prevent audio playing between mic record and utt handling #2621
Conversation
devops-mycroft
added
the
CLA: Yes
|
Voight Kampff Integration Test Failed (Results) |
|
Would there be an issue with skill handlers that don't produce a spoken sentence, for example just using the skill.acknowledge()? |
|
I've been trying to think of the best way to know that "a Skill has finished". It's also possible that Skills do something and don't respond at all, even an acknowledge tone. Certainly not recommended but still very possible. So perhaps we can re-use the VK then_wait method with a timeout of ~4 seconds (?), and if no speak command was detected then we restore the volume anyway. This should give most Skills plenty of time to do something. That way even if an expected action isn't taken, it falls back to restoring audio playback. |
|
Sounds like a good solution. An alternative is to add something to the on_end() skill handler callback, but the timeout should probably be there first step since it'll be pretty safe. |
|
Hello @krisgesling! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-06-29 05:01:08 UTC |
|
Have now modified so that the volume will restore after 8 seconds regardless of what happens. So if a Skill consumes an intent and does absolutely nothing or just plays an acknowledgement beep, the playing audio will be restored. Had to push it to a higher 8 seconds as unknown intents take longer to respond. This should be the minority of cases so I don't see it as a problem that the audio stops playing for slightly longer. |
krisgesling
force-pushed
the
bugfix/audio-after-stop
branch
from
06c5b0d
to
c4478e8
Compare
|
Voight Kampff Integration Test Failed (Results) |
2 similar comments
|
Voight Kampff Integration Test Failed (Results) |
|
Voight Kampff Integration Test Failed (Results) |
mycroft/audio/audioservice.py
Outdated
| LOG.debug('restoring volume') | ||
| self.current.restore_volume() | ||
|
|
||
| def wait_for_speak(bus, timeout=8): |
There was a problem hiding this comment.
bus doesn't seem to be used (instead the one associated with the class is used)? can the argument be removed?
mycroft/audio/audioservice.py
Outdated
| Args: | ||
| message: message bus message, not used but required | ||
| """ | ||
| def __restore_volume(): |
There was a problem hiding this comment.
does this need the __ it's local so it won't need to be made anonymous?
mycroft/audio/audioservice.py
Outdated
| bus (Mycroft MessageBus): Bus instance to listen on | ||
| timeout (int): how long to wait, defaults to 8 sec | ||
| """ | ||
| self.speak_msg_detected = False |
There was a problem hiding this comment.
seems like this can be a local variable instead of an object variable to keep things encapsulated.
speak_msg_detected = False here
and in detected_speak have a line
nonlocal speak_message_detected
feels like otherwise there could be issues with re-entrancy.
mycroft/audio/audioservice.py
Outdated
| self.bus.remove('speak', detected_speak) | ||
| return self.speak_msg_detected | ||
|
|
||
| if self.current is None: |
There was a problem hiding this comment.
This is probably a personal opinion but perhaps invert this logic for clarity
if self.current:
self.bus.on('recognizer_loop:speech.recognition.unknown',
__restore_volume)
speak_msg_detected = wait_for_speak(self.bus)
if not speak_msg_detected:
__restore_volume()
self.bus.remove('recognizer_loop:speech.recognition.unknown',
__restore_volume)
else:
LOG.debug("No audio service to restore volume of")
|
Great feedback, thanks Ake :) Have changed each of them. |
krisgesling
force-pushed
the
bugfix/audio-after-stop
branch
from
4799bd0
to
e39fe64
Compare
|
Voight Kampff Integration Test Failed (Results) |
|
Voight Kampff Integration Test Succeeded (Results) |
Description
This creates a separate
restore_volumehandler for the end of mic recording. Previously the same handler was used for speech and recording.When the recording finishes, there should either be an utterance to be handled, or a
speech.recognition.unknownmessage meaning the transcription failed because the user didn't say anything (or it was a false activation).If the latter and no utterance was detected, then we immediately restore the volume. Otherwise we do not restore the volume, and the utterance is passed onto the intent handlers. Once Mycroft responds to that utterance, the
_restore_volumehandler attached to theaudio_output_endmessage is triggered.This means that if audio is playing and:
Once the response has finished, audio resumes.
Need to confirm
Are there other paths that I'm not considering?
Is there a chance that neither a failed transcription, nor Mycroft replying would occur and therefore leave the audio in an undesired state?
How to test
Ensure that no snippets of news audio are played during interactions. Should still play between questions.
Contributor license agreement signed?