-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[AUDIO_WORKLET] Add new emscripten_audio_node_connect API #22667
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
Conversation
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.
Perhaps the title should something like "[AUDIO_WORKLET] Add new emscripten_audio_worklet_node_connect API"?
c1df5b5
to
2d0aa71
Compare
Good point. I’ll take a look tomorrow, plus I might roll it in with some other quality of life changes. |
As always, I'd love to see separate smaller PR that kind of thing rather than trying combine stuff. |
This sounds fine. Originally the connection was in JS since I did not want to start building a full Emscripten Web Audio API wrapper, but show developers in the examples how to do the JS side tasks on JS side. However I can see that users who don't have a JS side web audio engine, might want to have a pure C API without needing to dip to JS side at all. If we do this, I think it would be good to be forward-looking and generic, so instead of having a function emscripten_audio_worklet_node_connect(EMSCRIPTEN_WEBAUDIO_T audioContext, EMSCRIPTEN_AUDIO_WORKLET_NODE_T audioWorkletNode); let's instead have a function emscripten_audio_node_connect(EMSCRIPTEN_WEBAUDIO_T sourceNode, int outputIndex, EMSCRIPTEN_WEBAUDIO_T destinationNode, int inputIndex); and in the implementation of that function, do sourceNode.connect(destinationNode.destination || destinationNode, outputIndex, inputIndex); (https://developer.mozilla.org/en-US/docs/Web/API/AudioNode/connect) This way the same function can be used to connect to AudioContext destination, but also to connect any arbitrary audio nodes together. While we don't have capability to manipulate audio nodes in C side, maybe someone in the future might rationale to want to add that, so then the shape of the API would be generic to be compatible with that kind of expansion. Would that make sense? |
f5f46b6
to
bbded28
Compare
I made something more generic that takes any two nodes, and added the indices as params as suggested. I'll create a few smaller PRs with other proposed changes. |
6520ad6
to
c72221f
Compare
c72221f
to
5569cc9
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.
lgtm, but I'll wait for @juj to chime in again before landing.
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.
Perfect, this looks pretty clean!
In all the examples and in the docs, connecting the context and worklet is done with the following type of code:
Every example or usage needs this to play audio. Granted, more complex graphs will add more node types and
emscriptenGetAudioObject()
will have its uses, but for the general use case a simpler call without JS is preferred:See here for how it's done in the examples:
emscripten/test/webaudio/audioworklet.c
Line 50 in 7df48f6
I updated the examples and the documentation.
@juj Comments or ideas?