google-assistant-sdk/pushtotalk: fix conversation_stream handling #188
Conversation
- close stream on stop_recording - restart stream on start_playback - flush stream on stop_playback The previous behaviour (keeping the stream open the whole time) exposed a race condition where we tried to flush (write to) a stream that was still recording (read-only mode). Change-Id: I629bee9267744d5080bcdaff61c6da241ba8fe7f
| @@ -212,13 +215,12 @@ def flush(self): | |||
|
|
|||
| def start(self): | |||
| """Start the underlying stream.""" | |||
| if not self._audio_stream.active: | |||
| if self._audio_stream.active and not self._audio_stream.active: | |||
There was a problem hiding this comment.
The condition will never be met so audio stream will never start
There was a problem hiding this comment.
oh yep, this is a typo, this should go in flush :)
|
When returning back the original condition mentioned in my review
the assistant will run but will fail when trying to play assistant response pushtotalk.py: audio_helpers.py: Maybe this is not the right way how to report these issues, but I am beginner to GitHub :) |
Change-Id: Iaff6e3f25c57e82f10073812fcbea7b165ef044a
|
@PizlaTheDeveloper PTAL I fixed the typo :) |
|
Hi, thank you for your fix, but as I mentioned already, with this fix pushtotalk.py gets stuck when trying to play assistant's response for any query
|
- remove conversation stream threading.Event based locks - call stop_recording from the thread reading from the audio stream - fix tests According to: https://app.assembla.com/spaces/portaudio/wiki/Tips_Threading """ In general, calls to ReadStream and WriteStream on different streams in different threads are probably safe. That means that if you are doing Blocking I/O, one thread may read and write to one stream, and another thread may read and write to another. """ Change-Id: I1355986600453e9d1d932ebfc3e094359407b60d
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@PizlaTheDeveloper I merged your change from #206 in this PR, and added more fixes and top: PTAL! |
| if resp.speech_results: | ||
| logging.info('Transcript of user request: "%s".', | ||
| ' '.join(r.transcript | ||
| for r in resp.speech_results)) | ||
| self.conversation_stream.start_playback() |
There was a problem hiding this comment.
Should playback start when we get text speech results? I think it makes sense to do it when we get audio data.
There was a problem hiding this comment.
Do you think to start playback in 'if len(resp.audio_out.audio_data) > 0:' ?
Yes, it might be better way...
There was a problem hiding this comment.
Yes, but we also don't want to call start_playback more than one time.
There was a problem hiding this comment.
We should be able to check if start_playback has started already. Right now it feels like it is in the wrong place.
There was a problem hiding this comment.
@Fleker do you want me to change this in this PR?
There was a problem hiding this comment.
we should probably change it in this PR, but I'll leave it up to you.
|
@PizlaTheDeveloper can you confirm it fixes the issue for you? |
Change-Id: I14440b02745aad6ac67494bd4f7c7833d9dce1d6
- remove end_of_utterance() - stop recording from main thread with explicit locking Change-Id: I07b24b8193560783945acde9f2b55e5533578ebd
…io data Change-Id: Icfd2e3a01ddbb35e17e54b53f26091580bf31f0f
|
@Fleker PTAL |
|
@proppy I still have issues with this fix. While if I don't use these fixes I can run the sample with no errors and i can interact with the assistant but sometimes I get the usual audio errors: This error I get only on the raspberry pi, if I run the sample on my laptop I have no problem using your fixes Thanks. |
|
@moham96 we have not done a new release of code. In order to test, you will need to |
|
@moham96 can you confirm that you installed the package with: ? |
|
@Fleker I built the sdk from source (didn't use a release version) |
|
@proppy I cloned the repo and built a wheel for the google-assistant-sdk and installed it in a virtual environment. |
|
@proppy this is the error i get when running this issue happens beacuse it tries to run setup.py at the root of repository but there is no setup.py in the root, only in the google-assistant-sdk and google-assistant-grpc directories, maybe this needs a fix ?! |
|
@moham96 can you try to |
|
@moham96 sorry the right command is: |
|
@proppy when using this command it states that it installed correctly but when I try to run the sample a got a lot of |
|
@moham96 thanks for testing
I see, that must be because
This seems like a different error like the one you linked in #188 (comment) Can you open a separate issue with the full logs, there is also something wrong with |
|
@proppy sure, I will open a new issue. |
Fixes #186 #185 #155
The previous behaviour (keeping the stream open the whole time)
exposed a race condition where we tried to flush (write to) a stream
that was still recording (read-only mode).