-
Notifications
You must be signed in to change notification settings - Fork 435
AudioWorklet based Host for web when Wasm atomics are enabled #958
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: master
Are you sure you want to change the base?
Conversation
Very nice addition! This is a big one, so I'm gonna first trigger a Copilot review to walk through major points, if any, then do a further code review. Apologies for the late response - new collaborator here and grooming through the backlog now. |
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.
Pull Request Overview
This pull request adds AudioWorklet support for web audio in CPAL, providing a lower-latency, lower-overhead alternative to the existing WebAudio host when WebAssembly atomics are enabled. The implementation leverages shared memory between the main thread and audio worklet thread, allowing for more efficient audio processing similar to native platforms.
- Introduces a new
web_audio_worklet
feature flag and host implementation - Adds JavaScript worklet processor code that interfaces with WebAssembly
- Provides an example demonstrating the setup requirements and usage
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/platform/mod.rs | Adds conditional compilation for WebAudioWorklet host alongside existing WebAudio host |
src/host/web_audio_worklet/mod.rs | Core implementation of the AudioWorklet-based host with stream management |
src/host/web_audio_worklet/worklet.js | JavaScript AudioWorkletProcessor that interfaces with WASM audio processing |
src/host/web_audio_worklet/dependent_module.rs | Utility for dynamically loading JavaScript modules in AudioWorklet context |
src/host/mod.rs | Adds conditional module inclusion for web_audio_worklet |
examples/web-audio-worklet-beep/* | Complete example showing usage with required configuration |
Cargo.toml | Adds web_audio_worklet feature with necessary web-sys dependencies |
Comments suppressed due to low confidence (1)
src/host/web_audio_worklet/worklet.js:1
- [nitpick] The class name
WasmProcessor
is generic and could conflict with other WASM processors. Consider using a more specific name likeCpalWasmProcessor
to match the registered processor name.
registerProcessor("CpalProcessor", class WasmProcessor extends AudioWorkletProcessor {
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Thanks for following up! I applied some of Copilot's suggestions and also realized that some files needed to build the example were missing so I fixed that as well. Let me know if anything further is needed. |
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 looks very, very nice. Super to see such an exhaustive example too.
rustflags = ["-C", "target-feature=+atomics"] | ||
|
||
[unstable] | ||
build-std = ["std", "panic_abort"] |
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.
Does it have to abort? Could we leave that choice to the user?
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 right now when using multiple threads and atomics panic has to be set to abort. I can't remember the exact reason, but if I try to disable the flag there are cryptic build errors. Searching online doesn't turn up anything quickly. I think it's an area that's under documented right now because few people are using multithreaded WebAssembly with Rust.
For now I think leaving this as-is makes the most sense.
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.
OK, thanks for looking into it.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
* Update README to reference AudioWorklet usage * Attribute authorship of AudioWorklet example * Remove unwraps in example code * Choose Apache 2.0 license for copied code * Correctly report if the AudioWorklet host is available * Fix incorrect module reference
* Correct 2 errors introduced in previous commit
OK, I made a series of fixes to respond to comments and caught a few other things in the process. Thanks for the review! |
Thanks for the update! In the meantime a few conflicts came up because the platform implementation was changed (but simplified 😄 ) so if you could resolve those? Then I’ll be out in the coming days so hope to have more time to look into it again over the weekend. |
This pull request adds an optional feature-flagged
Host
that uses AudioWorklet on web when theatomics
feature is enabled. When theweb_audio_worklet
andwasm-bindgen
features are enabled and nightly Rust is compiled with theatomics
flag enabled the WebAudioWorklet Host is available.AudioWorklet
should be lower latency, lower overhead, and less prone to jank. This implementations works pretty much the same as a native audio thread, allowing the implementation to be simpler than the existing WebAudio Host implementation.To used shared memory it's also required (for security reasons) that the server hosting the Wasm to be configured with the correct CORS headers. I've added an example that shows how to build with the correct Rust flags and how to set the Cors headers in a test environment.
So far I've tested this in Chrome, Safari, and Firefox with the beep example and it seems to be working fine.
Edit: I'm not sure why a past merged PR shows up in Github's listed commits, but it seems to have no impact on this PR.