-
Notifications
You must be signed in to change notification settings - Fork 75
Break audio up into chunks before sending to google #51
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
base: main
Are you sure you want to change the base?
Conversation
|
@albertreed @endoplasmic Even though I like the effort in helping the community by solving this issue, I don't think we should implement this within the library itself but should be done outside of the library, or make it an optional route limit the frame length. Like we're discussing here in the referenced issue, google is probably talking about sample-frames instead of actual bytes in size that you're sending through to Google. (I'm not sure if this is the same or not) -- As of this PR, there's a million way to process audio and whole libraries to do that for you so I think we should limit or at least separate audio processing from this library. Other applications using this library, who do their own processing, might get into problems if this update PR gets merged. It's always good to do research on how google exactly wants their audio so let's continue this discussion and see what other people think and what else we can figure out to optimize this! |
|
This PR doesn't process the audio - it just formats it to be compatible with google's API. |
endoplasmic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions and some styling updates.
| // highly responsive. | ||
| let chunks = []; | ||
| while(chunks.length < data.length / MAX_FRAME_LENGTH) { | ||
| chunks.push(data.slice(chunks.length * MAX_FRAME_LENGTH, Math.min(data.length, (chunks.length + 1) * MAX_FRAME_LENGTH))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I've never done reviewing in github, so forgive my noobness)
Can you break your Math.min into a const and give it a name explaining what it's doing?
| while(chunks.length < data.length / MAX_FRAME_LENGTH) { | ||
| chunks.push(data.slice(chunks.length * MAX_FRAME_LENGTH, Math.min(data.length, (chunks.length + 1) * MAX_FRAME_LENGTH))); | ||
| } | ||
| chunks.forEach((chunk) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the curly braces to wrap the for each, or put it all on 1 line?
| // The max number of bytes of audio that can be sent in a single chunk. | ||
| // Not documented anywhere by google, but sending more than this amount | ||
| // throws an INVALID_REQUEST error stating that audio_in is too long. | ||
| const MAX_FRAME_LENGTH = 31 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this just figured out by process of elimination? Love me the prime number :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, yes. I'm very unfamiliar with the gRPC stuff so if you know a better way, let me know. It's susceptible to changes on their end which is obviously bad.
This change addresses the error:
Conversation Error: { Error: 3 INVALID_ARGUMENT: Invalid 'audio_in': audio frame length is too long.that results from trying to send chunks of audio that are too long.