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

Webaudio: Consistent use of node handle vs audio handle #23153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

slowriot
Copy link
Contributor

@slowriot slowriot commented Dec 13, 2024

There's currently some confusion in webaudio.h about node handle types vs the audio handle type - EMSCRIPTEN_WEBAUDIO_T is sometimes used to refer to nodes, where it should be EMSCRIPTEN_AUDIO_WORKLET_NODE_T.

emscripten_create_wasm_audio_worklet_node returns EMSCRIPTEN_AUDIO_WORKLET_NODE_T but functions emscripten_destroy_web_audio_node and emscripten_audio_node_connect expect the node handle to be passed as a EMSCRIPTEN_WEBAUDIO_T. This PR straightens out those inconsistencies.

It presently doesn't make any material difference since both handles are typedef int, so this PR just makes things clearer without changing any behaviour. If the typedefs ever change in future, it will become important.

Changes

  • Move the EMSCRIPTEN_AUDIO_WORKLET_NODE_T node handle typedef to the top of the file alongside the EMSCRIPTEN_WEBAUDIO_T definition.
  • emscripten_destroy_web_audio_node accepts a node handle.
  • emscripten_audio_node_connect accepts a node handle as its first argument.

@@ -57,7 +58,7 @@ void emscripten_destroy_audio_context(EMSCRIPTEN_WEBAUDIO_T audioContext);
// Disconnects the given audio node from its audio graph, and then releases
// the JS object table reference to the given audio node. The specified handle
// is invalid after calling this function.
void emscripten_destroy_web_audio_node(EMSCRIPTEN_WEBAUDIO_T objectHandle);
void emscripten_destroy_web_audio_node(EMSCRIPTEN_AUDIO_WORKLET_NODE_T objectHandle);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this API not be used without audio worklets?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this API not be used without audio worklets?

Parts of it, but the it needs JS to create other node types.

(In general though, some of the functions need a node, others a worklet context, and often they’re interchangeable when creating a graph depending on the use.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the types should be interchangeable, then perhaps EMSCRIPTEN_AUDIO_WORKLET_NODE_T can just be removed entirely, and EMSCRIPTEN_WEBAUDIO_T used everywhere? I can update to do that if preferred.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some parts of the API only work with a single type, others have tests like this and work with node or context:

assert(dstNode instanceof (window.AudioContext || window.webkitAudioContext) || dstNode instanceof window.AudioNode, `Called emscripten_audio_node_connect() on handle ${destination} that is not an AudioContext or AudioNode, but of type ${dstNode}`);

I don't think there's a correct choice.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 14, 2024

Let see what @juj thinks

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