Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why is the chunk_size set to 20kB? #48

Open
mpoullet opened this issue Oct 23, 2018 · 6 comments
Open

Why is the chunk_size set to 20kB? #48

mpoullet opened this issue Oct 23, 2018 · 6 comments

Comments

@mpoullet
Copy link
Contributor

in audio_input_file.cc the chunk_size is set to 20kB. Why?
Is there any official documentation/guidelines explaining this?

@Fleker
Copy link
Collaborator

Fleker commented Oct 23, 2018

I don't think there's a specific reason why it has to be 20KB.

@mpoullet
Copy link
Contributor Author

It's possible to push it to 31kB but after that I get

assistant_sdk failed, error: Invalid 'audio_in': audio frame length is too long.

see e.g. the comment in this Node.js fork too: https://github.com/albertreed/google-assistant/blob/master/components/conversation.js#L28

At the moment run_assistant_file is pretty lame compared to run_assistant_audio.
I'm wondering if it'd be ok to switch to a higher value for chunk_size.

@Fleker
Copy link
Collaborator

Fleker commented Oct 24, 2018

Looking at the proto definition for the field it's of type bytes.

To speculate, 31KB is very similar to 2^15 (32768 bytes), so it may be an encoding limitation and may provide the reason for a ceiling of chunk_size.

@mpoullet
Copy link
Contributor Author

mpoullet commented Nov 2, 2018

I obtain better results when the chunk_size and the delay match, e.g. 16kB (=500ms worth of audio input) and 500ms delay. As it is now it doesn't make much sense: 20kB (=625ms worth of audio) and 1s=1000ms delay? My first choice was to choose a chunk_size of 32kB to match the 1s delay but it's not possible, so I think a choice of 16kB/500ms would make much more sense in this example. Could I open a PR for this?

@Fleker
Copy link
Collaborator

Fleker commented Nov 2, 2018

Yeah you can open a PR for this. I'm not entirely sure what you mean by 'better results', but it may make sense to match the delay.

@mpoullet
Copy link
Contributor Author

mpoullet commented Nov 5, 2018

By better results I mean the overall execution time of run_assistant_file, e.g. (very rough measurements):

  • original commit (20kB/1000ms):
time ./run_assistant_file --input resources/weather_in_mountain_view.raw --output resources/response.raw --credentials ./credentials.json 
assistant_sdk response:
Looks like a pleasant day
Currently in Mountain View it's 55 and clear. Today, it'll be sunny, with a forecast high of 72 and a low of 49.
---
( More on weather.com )

real	0m3.480s
user	0m0.080s
sys	0m0.016s
  • by matching size and delay (e.g. 16kB/500ms or 31kB/1000ms):
time ./run_assistant_file --input resources/weather_in_mountain_view.raw --output resources/response.raw --credentials ./credentials.json 
assistant_sdk response:
Looks pleasant outside
Right now in Mountain View it's 55 and clear. Today, it'll be sunny, with a forecast high of 72 and a low of 49.
---
( More on weather.com )

real	0m2.027s
user	0m0.092s
sys	0m0.020s

so about 1s better which is noticeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants