Do no enqueue any audio payloads larger than 1000 bytes #2791
Conversation
| * 1000 bytes is enqueued. Any controller that attempts to enqueue an audio payload larger than 1000 bytes will | ||
| * encounter this error and will permanently stop sending or receiving audio. | ||
| */ | ||
| private maybeEnqueueAudioFrame( |
There was a problem hiding this comment.
Since its such a short function we can probably make the name explicit, maybe enqueueAudioFrameIfPayloadSizeIsValid? Borderline too verbose, up to you. I myself have felt a bit mid towards the prefix 'maybe' after using it a bit over the years.
There was a problem hiding this comment.
We can go the verbose route as this is a private function anyway. I only used maybe since it looked like an existing pattern but I'm not strongly for that pattern either.
| const enqueueSpy = sinon.spy(controller, 'enqueue'); | ||
|
|
||
| expect(enqueueSpy.calledOnce).to.be.false; | ||
| frame.data = new ArrayBuffer(encoder['maxAudioPayloadSizeBytes'] - 1); |
There was a problem hiding this comment.
No way to test this without these private function accesses? Also, i'm pretty sure standard practice is to avoid using constants from non test code in tests. Not a big deal though.
There was a problem hiding this comment.
I can try to change this to test these cases without the need for private function accesses.
There was a problem hiding this comment.
I didn't avoid the private function accesses completely since it would have required a lot more mocking that seemed overkill for this small change, but I did update the tests to more closely resemble the actual edge cases that this change would protect against.
In Chromium-based browsers, writing audio payloads larger than 1000 bytes using the WebRTC Insertable Streams API (which is used to enable dynamic audio redundancy) will cause an error to be thrown and cause audio flow to permanently stop. See https://crbug.com/1248479.
Issue #: n/a
Description of changes:
In Chromium-based browsers, writing audio payloads larger than 1000 bytes using the WebRTC Insertable Streams API (which is used to enable dynamic audio redundancy) will cause an error to be thrown and cause audio flow to permanently stop. See https://crbug.com/1248479.
Testing:
Can these tested using a demo application? Please provide reproducible step-by-step instructions.
Set fullband music (stereo) qualityselected (this will cause the audio frames to be especially large due to the higher quality)Also share tab audiooptionUncaught (in promise) DOMException: Failed to execute 'write' on 'UnderlyingSinkBase': Frame too largeerror and Attendee B should receive audio normallyChecklist:
Have you successfully run
npm run build:releaselocally?y
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
n
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
n
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.