AudioWorklet backend: Fix bug when Wasm memory resizes. Avoid JS allocations. #1050
Conversation
…id JS allocations.
|
|
||
| for (let i = 0, j = ch; i < frame_size; i++, j += channels_count) { | ||
| channel[i] = interleaved[j]; | ||
| for (let i = 0; i < frame_size; i++) { |
There was a problem hiding this comment.
What is the risk of interleaved_start + (frame_size * channels_count) being out of bounds? For memory safety would it benefit from an assertion like:
if (interleaved_start + (frame_size * channels_count) > this.wasm_memory.length) {
console.error("Audio buffer out of bounds!");
return false; // Stop processing
}There was a problem hiding this comment.
The risk should be very low but if it were out of bounds Javascript would effectively write NaNs into the output audio buffer, which is not ideal.
I'll add the check because it's low cost and more plainly fails if somehow such a bug did ever occur.
| process(inputs, outputs) { | ||
| // Check if memory grew and update view | ||
| if (this.wasm_memory.buffer !== this.memory.buffer) { | ||
| this.wasm_memory = new Float32Array(this.memory.buffer); |
There was a problem hiding this comment.
Does this fail silently? Would this be more prudent?
try {
if (this.wasm_memory.buffer !== this.memory.buffer) {
this.wasm_memory = new Float32Array(this.memory.buffer);
}
} catch (e) {
console.error("Failed to update Wasm memory view:", e);
return false;
}There was a problem hiding this comment.
I don't see a reason this code is much more likely to fail than other parts of code. I also don't think it would fail silently if it were to fail. So I would propose leaving it as is.
src/host/audioworklet/worklet.js
Outdated
| for (let ch = 0; ch < channels_count; ch++) { | ||
| const channel = channels[ch]; | ||
| let src = interleaved_start + ch; |
There was a problem hiding this comment.
Maybe interleaved_idx or read_pos instead of src for clarity?
😁 no worries, thank you for your commitment! While at it, would you:
|
|
I think we're close to releasing v0.17... would you have time to look at the last bits? |
|
Sorry for the lack of response to the last comments, it's been a busy time of year. I'll take a look at this tomorrow! |
|
OK, I responded to the code review feedback and renamed "web_audio_worklet" which I admit is a mouthful to just "audioworklet" throughout the codebase. Previously I felt the need to prepend "Web" to "Audio Worklet" clarify this was a web specific feature but really none of the other backends append things like "Android" or "Mac" so that was probably unnecessary. Switching to just "audioworklet" is simply the name of the Web API being used and is shorter. I'm also happy to change it back to something else if there's differing opinions. |
2 participants
OK, this is my last commit for this PR for real this time!
I realized that there's a potential subtle bug that was unaddressed. When a Wasm program resizes the Javascript program's 'view' into the Wasm memory only points to the initial Wasm memory from before the resize.
It's unlikely to be hit often but if the interleaved buffer were resized and reallocated within Rust and given a new memory location outside of the Wasm program's initial range of memory then the AudioWorklet would crash.
This PR adds a check that prevents that.
Additionally while I was looking at this I also tested if other deinterleaving strategies are faster. Deinterleaving within Rust / Wasm (I didn't try SIMD) introduces an additional copy (interleaved -> deinterleaved and then deinterleaved -> JS buffers). That was ~20% to 30% slower. Special casing stereo deinterleaving on the JS side was negligibly faster if at all.
I also did realize that there was a way to tweak the loop to remove the call to
subarray, which is good because that call didn't really make the code simpler and created a tiny amount of JS garbage which could trigger periodic garbage collections on the audio thread.After this I think I'm finally done tweaking the AudioWorklet backend.