-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: added missing options to EmscriptenAudioWorkletNodeCreateOptions closes #23982 #25497
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
@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
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
@juj i think it's done. Is this fine?
src/lib/libwebaudio.js
Outdated
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
could you please lmk if any changes are required other than adding comments?
I'm out sick, I should be able to take a look next week.
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.Closes #23982