Skip to content
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

[AUDIO_WORKLET] Add new emscripten_audio_node_connect API #22667

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

cwoffenden
Copy link
Contributor

@cwoffenden cwoffenden commented Oct 2, 2024

In all the examples and in the docs, connecting the context and worklet is done with the following type of code:

EM_ASM({
  emscriptenGetAudioObject($0).connect(emscriptenGetAudioObject($1).destination)
}, wasmAudioWorklet, audioContext);

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:

emscripten_audio_worklet_node_connect(audioContext, wasmAudioWorklet);

See here for how it's done in the examples:

EM_JS(void, InitHtmlUi, (EMSCRIPTEN_WEBAUDIO_T audioContext, EMSCRIPTEN_AUDIO_WORKLET_NODE_T audioWorkletNode), {

I updated the examples and the documentation.

@juj Comments or ideas?

Copy link
Collaborator

@sbc100 sbc100 left a 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"?

@cwoffenden
Copy link
Contributor Author

Perhaps the title should something like "[AUDIO_WORKLET] Add new emscripten_audio_worklet_node_connect API"?

Good point. I’ll take a look tomorrow, plus I might roll it in with some other quality of life changes.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 2, 2024

Perhaps the title should something like "[AUDIO_WORKLET] Add new emscripten_audio_worklet_node_connect API"?

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.

@juj
Copy link
Collaborator

juj commented Oct 2, 2024

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?

@cwoffenden cwoffenden force-pushed the cw-audio-api-changes branch 2 times, most recently from f5f46b6 to bbded28 Compare October 3, 2024 10:54
@cwoffenden cwoffenden changed the title AudioWorklet: move the oft called context/worklet connect to native code [AUDIO_WORKLET] Add new emscripten_audio_node_connect API Oct 3, 2024
@cwoffenden
Copy link
Contributor Author

If we do this, I think it would be good to be forward-looking and generic, so instead of having a function

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.

@cwoffenden cwoffenden force-pushed the cw-audio-api-changes branch 2 times, most recently from 6520ad6 to c72221f Compare October 4, 2024 13:29
Copy link
Collaborator

@sbc100 sbc100 left a 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.

@cwoffenden
Copy link
Contributor Author

I'll wait for @juj to chime in again before landing.

This PR also has the much smaller #22681 as a sibling for the public API parts I’d like to add to. Anything else will be internal.

Copy link
Collaborator

@juj juj left a 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!

@juj juj merged commit 4db36c1 into emscripten-core:main Oct 8, 2024
28 checks passed
@cwoffenden cwoffenden deleted the cw-audio-api-changes branch October 8, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants