Skip to content
This repository was archived by the owner on Oct 19, 2023. It is now read-only.

fix-pushtotalk #206

Merged
merged 1 commit into from
Mar 19, 2018
Merged

fix-pushtotalk #206

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions google-assistant-sdk/googlesamples/assistant/grpc/pushtotalk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ def assist(self):
def iter_assist_requests():
for c in self.gen_assist_requests():
assistant_helpers.log_assist_request_without_audio(c)
yield c
self.conversation_stream.start_playback()
yield c

# This generator yields AssistResponse proto messages
# received from the gRPC Google Assistant API.
Expand All @@ -138,6 +137,7 @@ def iter_assist_requests():
logging.info('Transcript of user request: "%s".',
' '.join(r.transcript
for r in resp.speech_results))
self.conversation_stream.start_playback()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep doing this in the same grpc thread from where the stream reading is happening (i.e: at the end of the generator).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole problem is that I get data for playback in assist response before the request generator ends, so the playback is not started yet and writing to playback stream gets stuck while waiting to start the playback...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PizlaTheDeveloper yep, but I don't think this will be an issue if we use two different stream as you introduced with d0eb6a3

Can you give a try to 2c63ebc and let me know if it works for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but it doesn't work :(
The generator 'iter_assist_requests' will never stop iterating because it iterates while conversation_stream has data on its input, but there will be always some data if the source stream is running. So the recording will never stop.

I think that the events '_stop_recording' and '_start_playback ' should stay in ConversationStream to be able to signalize stop recording and return empty data to read calling what will end the iteration. Stop recording must be signaled when END_OF_UTTERANCE occurs.

I don't know if I described it correctly :)

I think my previous code was correct :)

Thank you for your interest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generator 'iter_assist_requests' will never stop iterating because it iterates while conversation_stream has data on its input, buend_of_utterancet there will be always some data if the source stream is running. So the recording will never stop.

Makes total sense!

I added an explicit end_of_utterance event in 187b485 (which I believe is cleaner that tying everything to stop_recording) let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PizlaTheDeveloper Thanks for digging this out.

Because of https://github.com/grpc/grpc/blob/16316a9fec3a78baf83d96dbdb392e90226bb013/src/python/grpcio/grpc/_channel.py#L682, I was making the assumption that grpc would always exhaust the request generator before ending the iteration of the response, and thought this would always be called:
187b485#diff-7262ae17d2cea4f6cb6c2b5934aaf939L127

But it looks like it's not the case! and that the iteration on self.assistant.Assist responses can finish before iter_assist_requests is exhausted. (Maybe @nathanielmanistaatgoogle can shed some light on the expected behavior).

@PizlaTheDeveloper can you give a try at 2dfc868 I added explicit locking to make sure we can safely close the recording stream from the main thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's working like a charm now !!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you review/approve #188 ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without reading this whole conversation, so I may be missing some context: yes, it is not the case that exhaustion of the request iterator is guaranteed. If an RPC ends (for whatever reason, successfully or unsuccessfully) before exhaustion of the request iterator, iteration of the request iterator may be stopped. (Or it may be continued! Don't depend on either being the case.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathanielmanistaatgoogle Thanks for the confirmation! So we really shouldn't perform any cleanup/teardown logic at the end of the iterator :)

logging.info('Playing assistant response.')
if len(resp.audio_out.audio_data) > 0:
self.conversation_stream.write(resp.audio_out.audio_data)
Expand Down Expand Up @@ -317,21 +317,18 @@ def main(api_endpoint, credentials, project_id,
logging.info('Connecting to %s', api_endpoint)

# Configure audio source and sink.
audio_device = None
if input_audio_file:
audio_source = audio_helpers.WaveSource(
open(input_audio_file, 'rb'),
sample_rate=audio_sample_rate,
sample_width=audio_sample_width
)
else:
audio_source = audio_device = (
audio_device or audio_helpers.SoundDeviceStream(
audio_source = audio_helpers.SoundDeviceStream(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't reuse the same stream for input/output, I think we might be able to get rid of some of the locking happening in conversationstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reusing the stream is possible, I didn't understand how it could be reused, but now it is more clear for me :) Sorry, I am Python beginner :)

sample_rate=audio_sample_rate,
sample_width=audio_sample_width,
block_size=audio_block_size,
flush_size=audio_flush_size
)
)
if output_audio_file:
audio_sink = audio_helpers.WaveSink(
Expand All @@ -340,13 +337,11 @@ def main(api_endpoint, credentials, project_id,
sample_width=audio_sample_width
)
else:
audio_sink = audio_device = (
audio_device or audio_helpers.SoundDeviceStream(
audio_sink = audio_helpers.SoundDeviceStream(
sample_rate=audio_sample_rate,
sample_width=audio_sample_width,
block_size=audio_block_size,
flush_size=audio_flush_size
)
)
# Create conversation stream with the given audio source and sink.
conversation_stream = audio_helpers.ConversationStream(
Expand Down