fix: added missing options to EmscriptenAudioWorkletNodeCreateOptions closes #23982

[nikitha-m]

Problem Description

The EmscriptenAudioWorkletNodeCreateOptions struct currently only exposes options specific to AudioWorkletNode itself (numberOfInputs, numberOfOutputs, outputChannelCounts), but AudioWorkletNode inherits from AudioNode which has additional properties:

• channelCount
• channelCountMode
• channelInterpretation

Fix

• Extend the C struct in system/include/emscripten/webaudio.h.
• Update the JavaScript implementation in src/lib/libwebaudio.js to pass these options to the AudioWorkletNode constructor.

Closes #23982

[juj]

channelCountMode and channelInterpretation are strings, so an integer will not work.

Create an enum for those, and in the JS code, index the enum into an array of strings.

See e.g. https://github.com/juj/wasm_webgpu/blob/3ed529956d1ee3f14f64c07769385e1929908f68/lib/lib_webgpu.js#L615 and https://github.com/juj/wasm_webgpu/blob/3ed529956d1ee3f14f64c07769385e1929908f68/lib/lib_webgpu.js#L729 in another repo for examples.

[juj]

This comment documents the history of evolution of the codebase (i.e. "before this PR, we had a 'basic' set, after this PR, we now have 'extended' set"), but in the Web Audio specification, there is no distinction between basic and extended options - there are just options.

So instead of documenting history of evolution, maybe add short comments that describe the new parameters?

[nikitha-m]

@juj how do i run test cases locally? I followed the docs but I'm facing some issues while running these commands

emsdk install tot
emsdk activate tot

[juj]

Yeah, that is how you install a pre-compiled Emscripten. If installing tot doesn't work, you can also try emsdk install latest followed by emsdk activate latest.

To run browser audio-related tests, you can run test/runner browser.test_*audio* and test/runner interactive.test_*audio*. The first suite is automated, but the second suite requires an interactive user to prod the tests forward.

If installing tot or latest doesn't work out, you can also try these steps to build all from source: https://bugzilla.mozilla.org/show_bug.cgi?id=1992558#c2

[nikitha-m]

@juj i think it's done. Is this fine?

[juj]

The anonymous functions and || undefined should be unnecessary here, they take up code size for no benefit.

E.g. channelCountMode: ['max','clamped-max','explicit'][{{{ makeGetValue(...) }}}], looks like would be enough here?

One thing to verify here is how do older browsers behave (or Safari) that do not yet support these options? If you run in an older browser that does not support any of this, would that cause an error?

It would be good to have a short test of non-default channelCountMode and non-default channelInterpretation to ensure that it works.

[juj]

Can you add here a short comment describing the default values of each?

Hmm it seems that we get the struct initialization regression problem here.. that is, after this addition, all users must explicitly initialize these values to valid numbers; or their code will stop working? E.g. is existing users had code, say,

EmscriptenAudioWorkletNodeCreateOptions options = {
  .numberOfInputs = 2,
  .numberOfOutputs = 2,
};

then when they update Emscripten to recompile their code, they will get implicit

.channelCount = 0,
.channelCountMode = WEBAUDIO_CHANNEL_COUNT_MODE_MAX,
.channelInterpretation = WEBAUDIO_CHANNEL_INTERPRETATION_SPEAKERS

and the spec says, "If this value is set to zero [...] the implementation MUST throw a [NotSupportedError](https://webidl.spec.whatwg.org/#notsupportederror) exception."

So in this form, this update would cause all Web Audio utilizing code to break from running, unless they are updated.

That is a bit annoying, since there is necessarily not a good way for developers to figure that out, and they might raise a bug report "I updated Emscripten and now I am getting an NotSupportedError exception. It used to work in previous version of Emscripten, so what did you regress?"

[juj]

Oh now I see in the init code there was a v > 0 ? v : undefined added to work around this problem.

Hmm, I suppose that's ok. It would be good to have a comment, e.g.

// the number of channels used when up-mixing and down-mixing connections to any inputs to the node.
// Pass 0 to omit specifying this parameter altogether during context creation.
unsigned long channelCount;

[juj]

@cwoffenden what do you think about the struct init problem for the future?

Should we try to model the struct so that all-zero-init should be preserved as a valid struct init with defaults? E.g.

EmscriptenAudioWorkletNodeCreateOptions options = {};

would provide all zeros, and at JS side, we would always shoehorn the init to map to default values of an Audio Context?

In WebGPU I have been explicitly using global constant data default-initialized struct constants, e.g. here: https://github.com/juj/wasm_webgpu/blob/3ed529956d1ee3f14f64c07769385e1929908f68/lib/lib_webgpu_cpp20.cpp#L34-L45

which would mean we would enforce users to do

EmscriptenAudioWorkletNodeCreateOptions options = AUDIO_WORKLET_NODE_CREATE_DEFAULT_INITIALIZER;

if they want to ensure forward compatibility with default values into the future.

The default-zero-init for all fields would be preferred for code size and simplicity, though only if we can expect that any upcoming fields in the Web Audio spec would reasonably map from zero to the default. WDYT?

[cwoffenden]

I’m out for a week or so but jotting some quick thoughts down, I personally prefer (and use) the zero initialiser, with, for example, an enum UNDEFINED mapped to zero, and other special treatment of zeros. I find it cleaner than building a struct with all the defaults, and the constructor passed the struct is probably going to have all the sanity checks anyway, so using that to create sane defaults is a nice byproduct.

[juj]

I personally prefer [...] the zero initialiser

Yeah, of course using zeroes is nice, though this is not a question of (just) programming style or preference. Rather the risk there is whether we will always be technically able to do this.

For example, in this specific PR case we were just barely saved by a lucky happenstance that the new added param channelCount has zero be an invalid value in the upstream Web Audio spec.

If that were not the case, then the above v > 0 ? v : undefined workaround would not be possible, but user breakage would have been inevitable, unless we also added some extra bool channelCountSpecified; variable.

Though I suppose the resolution here is clear that we don't want to go the route of initializer structs (or functions, like pthread_attr_init() for example has), and deal with the problematic case when/if one arises in the future.