Add missing options to EmscriptenAudioWorkletNodeCreateOptions #25497
Conversation
|
@juj how do i run test cases locally? I followed the docs but I'm facing some issues while running these commands |
|
Yeah, that is how you install a pre-compiled Emscripten. If installing To run browser audio-related tests, you can run If installing |
|
@juj i think it's done. Is this fine? |
src/lib/libwebaudio.js
Outdated
| outputChannelCount: readChannelCountArray({{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.outputChannelCounts, 'i32*') }}}, optionsOutputs), | ||
| channelCount: (function(){ var v = {{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.channelCount, 'u32') }}}; return v > 0 ? v : undefined; })(), | ||
| channelCountMode: (function(){ var v = {{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.channelCountMode, 'i32') }}}; var arr = ['max','clamped-max','explicit']; return arr[v] || undefined; })(), | ||
| channelInterpretation: (function(){ var v = {{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.channelInterpretation, 'i32') }}}; var arr = ['speakers','discrete']; return arr[v] || undefined; })(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did you have a chance to look at the above review comment?
How about this code:
channelCount: {{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.channelCount, 'u32') }}} || undefined,
channelCountMode: [/*'max'*/,'clamped-max','explicit'][{{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.channelCountMode, 'i32') }}}],
channelInterpretation: [/*'speakers'*/,'discrete'][{{{ makeGetValue('options', C_STRUCTS.EmscriptenAudioWorkletNodeCreateOptions.channelInterpretation, 'i32') }}}],It is shorter for the same effect? ('max' and 'speakers' are the default in the spec if I understand correctly)
|
@juj @cwoffenden could you please lmk if any changes are required other than adding comments? |
|
@juj This test case that failed doesn't look like its related to the webaudio related tests can you help me with this? |
|
@nikitha-m can you try merging/rebasing with main? |
| ], | ||
| "EmscriptenWebSocketCreateAttributes": [ | ||
| "protocols" | ||
| ] |
There was a problem hiding this comment.
Minor: formatting got lost.
cwoffenden
left a comment
cwoffenden
left a comment
There was a problem hiding this comment.
With the changes that @juj suggested, this all now looks compact and correct.
sbc100
changed the title
|
@juj @cwoffenden its done |
I can neither approve nor merge, I can just say that it looks correct and doesn't break anything. I've not personally used these audio node properties for mixing so I can't comment further (we perform all the mixing ourselves and to a regular stereo output). |
|
Hmm looks like CircleCI is stuck here on code size checks. I'll land this and re-roll the code size update action, I think those will change here. Thanks for the contribution @nikitha-m . |
4 participants
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:
Fix
system/include/emscripten/webaudio.h.src/lib/libwebaudio.jsto pass these options to theAudioWorkletNodeconstructor.Fixes: #23982