-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
Add ring buffers to AudioWorklet processors to support variable buffer sizes #376
Conversation
Very cool @oshoham! In reviewing this and testing it out locally, I found an issue that was introduced in the previous PR, that I think we should address before merging this PR: #373 (comment) - sorry I missed it before! Also an issue with soundFile pause on fallback browsers that was introduced in the same PR #373, so it's probably related to keeping track of the current time in the sound file using the Worklet/ScriptProcessor. Need to investigate that one a bit more, but the example at |
@therewasaguy I've opened PR #380 with fixes for the issues that you mentioned. |
a06e397
to
a615bb1
Compare
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.
It seems the worklet polyfill doesn't set the ScriptProcessorNode bufferSize https://github.com/GoogleChromeLabs/audioworklet-polyfill/blob/aea68c7e0da196a240d6c321c47b8e6a6819b657/src/index.js#L26
This is causing an issue in the SoundRecorder processor for fallback browsers like Firefox, which seems to use a default bufferSize of 4096. If we set the buffer size to 1024, it generates a recording that only has the first 1024 samples of each buffer.
We might need to hold off on the switch to Ring Buffer until we fix that (in the polyfill?)
|
||
this.inputRingBuffer.push(input); | ||
|
||
if (this.inputRingBuffer.framesAvailable >= this.bufferSize) { |
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.
the switch to Ring Buffer is causing an issue in Firefox for "/examples/record". It seems that the bufferSize for Firefox ScriptProcessorNode is 4096, regardless of what we pass in here.
src/soundRecorder.js
Outdated
processorOptions: { numInputChannels: this._inputChannels } | ||
processorOptions: { | ||
numInputChannels: this._inputChannels, | ||
bufferSize: 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.
if we set this to 4096, it matches the buffer size of the ScriptProcessor in Firefox. Otherwise, we get choppy recordings
…size than the one chosen by the polyfill
@therewasaguy Wow, thanks for catching that! I just pushed a fix for this issue - in fallback browsers, See here for an example. I've tested this on Chrome, Firefox, and Safari, and it seems to fix the problem with recording that you encountered. Let me know what you think of this. |
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.
This does the trick, and I think we could consolidate a bit
src/soundRecorder.js
Outdated
@@ -83,11 +83,13 @@ define(function (require) { | |||
this._inputChannels = 2; | |||
this._outputChannels = 2; // stereo output, even if input is mono | |||
|
|||
const workletBufferSize = 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.
I think we could determine the proper buffer size here, and abstract the code to do it into a shared method. Maybe something like
safeBufferSize(idealBufferSize) {
if (<AudioWorklet is not supported>) return idealBufferSize
else createScriptProcessorNode --> return its bufferSize
}
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.
@therewasaguy I've extracted this logic into a shared method in helpers.js
: 2014fbc#diff-b38f7254be1a9f817fd2e33c294481a1R305
src/soundfile.js
Outdated
if (self._workletNode instanceof ScriptProcessorNode) { | ||
self._workletNode.port.postMessage({ | ||
name: 'bufferSize', | ||
bufferSize: Math.max(workletBufferSize, self._workletNode.bufferSize) |
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.
is it better to pick the max, or—in cases where AudioWorklet is not supported—to always go with the script processor node buffer size?
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.
yeah, I agree that it's probably better to always go with the script processor node buffer size - I've used that approach in my latest commit
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.
👍 awesome work, and thanks for incorporating my feedback!
AudioWorklet processors use a fixed buffer size of 128 frames, unlike ScriptProcessorNodes which take a buffer size as an argument. A lower buffer size will generally result in lower latency but also higher CPU usage. If we want our AudioWorklet nodes to use the same buffer sizes as the ScriptProcessorNodes that they replaced, one solution is to use a ring buffer in each AudioWorklet processor to hold on to frames of audio until the desired buffer size has been reached. For more details on this approach, see the "Handling Buffer Size Mismatch" section of the article Audio Worklet Design Patterns.
In this PR, I've added a
RingBuffer
helper class adapted from some of Google's sample code, and I've updated the AudioWorklet processors for p5.SoundRecorder, p5.SoundFile, and p5.Amplitude to each use the buffer size specified by their previous ScriptProcessorNode implementations.