[AUDIO_WORKLET] Added API for getting the buffer's quantum size #22681

cwoffenden · 2024-10-04T13:40:02Z

The Web Audio API defines the processed sample size as always being 128, which is hardcoded in both the test code and docs. The upcoming Web Audio API has the option to set this to a user defined setting or request the machine's preference, so in preparation the Audio Worklet is extended with a function to query the context's quantum at creation time (and before the worklet is created), and also the processing callback contains a field with the same value.

For the simplest uses transitioning to the processing callback's field value will mean future changes will simply work.

Once the 1.1 version of the Web Audio API is supported, the context creation can be amended to accept a quantum hint, and any code written again these PR's changes will still work.

juj · 2024-10-08T09:08:35Z

I recall that Web Audio has gained a feature to allow a custom render quantum, though I actually never got around to tending to this:

so I wonder instead of promoting a define (which will be hardcoded in size), we should look into how to expand the api to read the actual quantum in a configurable way? I haven't looked into the details yet, on how to pull this off in a manner that doesn't break existing users (or I wonder if we might even care about that).

cwoffenden · 2024-10-08T09:30:40Z

I wonder instead of promoting a define (which will be hardcoded in size), we should look into how to expand the api to read the actual quantum in a configurable way?

I did read through most of the proposals but I think I zoned out after a while due to the multi-year nature of it. But you're right, the safest would be to have an API call to get the value. The details are in the draft:

https://webaudio.github.io/web-audio-api/#BaseAudioContext

I can test for renderQuantumSize, and if it exists great, otherwise return the default.

cwoffenden · 2024-10-08T17:28:51Z

@juj The context's renderQuantumSize doesn't look to be implemented anywhere yet, so I'll come back to this. I did however add a quantumSize field to the AudioSampleFrame, as well as a standalone emscripten_audio_context_quantum_size(). Both are hardcoded to 128 for now. Both have their uses, the function call for deciding upon buffer/stack sizes, the struct field for the callback.

Whilst making changes to the audio_worklet.js I used the struct_info to get the sizes, so for example:

stackMemoryNeeded = (numInputs + numOutputs) * 8

Becomes:

stackMemoryNeeded = (numInputs + numOutputs) * {{{ C_STRUCTS.AudioSampleFrame.__size__ }}}

Then we never need to keep track of these manually with more magic numbers. And also same with the field offsets (playing nice with wasm64. All of these get resolved to constants.

system/include/emscripten/webaudio.h

test/webaudio/audio_worklet_tone_generator.c

src/audio_worklet.js

juj · 2024-10-09T08:04:41Z

src/audio_worklet.js

@@ -38,60 +38,69 @@ function createWasmAudioWorkletProcessor(audioParams) {
    }

    process(inputList, outputList, parameters) {
+      // hardcoding this until the Web Audio 1.1 API spec makes it into browsers and we can get the AudioWorkletProcessor's context quantum
+      const quantumSize = 128;


Reading the spec at https://webaudio.github.io/web-audio-api/ , I think we can (and should) already implement this, as it will be need to be implemented in a backwards compatible manner anyways.

I.e. something like const quantumSize = context.renderQuantumSize || 128;

The reason I say that we should implement this already now, because the data flow needs tending to, in order to get the renderQuantumSize posted into the worklet. So it would be good to implement it already pretending that the renderQuamtumSize parameter might exist in an implementation. Reading the spec, it seems fairly complete to assume that the quantum size will exist at some point.

Also, I wonder if we should expand the context creation function to allow taking in a quantum size?

I.e. something like const quantumSize = context.renderQuantumSize || 128;

Originally I did, but I chose not to in the end because it's [SecureContext] in the spec, and when it finally makes it to a browser I didn't want an exception. I thought of wrapping in a try/catch, but not knowing at all what the behaviour would be, I opted for the safer option.

Also, I wonder if we should expand the context creation function to allow taking in a quantum size?

I also considered this, in preparation, but since I didn't know how it would be treated in reality I chose the safer option.

The rationale behind both decisions were: if we expose setting it, we need to be able to read it otherwise things will break, and if we read it things might also break. We're itching to see the larger buffer sizes at work, so it's definitely something I or @tklajnscek will revisit as soon as.

juj · 2024-10-09T08:06:29Z

playing nice with wasm64

I see there are still pointer e.g. assignments to HEAPU32 that won't actually work in Wasm64 mode, which will need HEAPU64 assignments. But that is good work for a separate PR, to keep the wasm64 part orthogonal.

juj · 2024-10-09T08:08:39Z

Once the 1.1 version of the Web Audio API is supported

I'm getting an impression this might not happen any time soon, though maybe we should still implement the renderQuantumSize request/read flow in a way that is complete/forward-looking for the implementation that should appear.

juj

Overall looks good. Personally I would squeeze some code size bytes out of the JS code, though I think it's fine like this.

One quirk is that the quantumSize is not part of the AudioSampleFrame, i.e. there cannot be separate quantum sizes for separate sample inputs and outputs, it's always a constant.

But the only way to address that would probably be to change the signature of the audio process callback, which would be a breaking change, so this looks like a nice middle ground.

I'd like to see the setting and reading the renderQuantumSize flow to exist, just to make sure we can actually get the renderQuantumSize information into the worklet cleanly. (we probably need to postMessage it at audio worklet creation time?) But works either way.

cwoffenden · 2024-10-09T09:22:43Z

Personally I would squeeze some code size bytes out of the JS code, though I think it's fine like this.

I may just shorten some variable names for now. Ultimately, time permitting (which is wishful thinking), I want to look at what it would take to get the worklet code through Closure, the same as sbc100 did recently with general worklets (IIRC).

One quirk is that the quantumSize is not part of the AudioSampleFrame, i.e. there cannot be separate quantum sizes for separate sample inputs and outputs, it's always a constant.

This also irks me, but as you mention I didn't want to make a breaking change to the callback signature. It might be worth breaking though, since the API didn't work with Closure until #22472 a few months back, I wonder how much production pain this would cause for others?

I could mark emscripten_create_wasm_audio_worklet_node() as deprecated, add a new one that takes a different EmscriptenWorkletNodeProcessCallback signature, and let the two live side by side for a while? But let me do the minor tweaks here and land this first, I don't know when I'll have free time again.

I'd like to see the setting and reading the renderQuantumSize flow to exist, just to make sure we can actually get the renderQuantumSize information into the worklet cleanly.

I'd pass it as one of the processor opts, along with the callback, etc. We already know it at that point, so in the creation code I could read it from emscripten_audio_context_quantum_size() then we only have one hardcoded 128 and a single place to make the change. I'll try to look at this today.

cwoffenden · 2024-10-09T17:35:36Z

@juj The quantum size has a single getter now, used internally where needed:

emscripten/src/library_webaudio.js

Line 41 in 8e1115a

$emscriptenGetContextQuantumSize: (contextHandle) => {

The value is also passed in the options to the WasmAudioWorkletProcessor at construction, since it's fixed at context creation time:

emscripten/src/library_webaudio.js

Line 299 in 8e1115a

'qs': emscriptenGetContextQuantumSize(contextHandle)

If this value is expected to change, e.g. the worklet node gets reattached to another context (I don't think this possible) then we could simple read it in process() from the same function.

cwoffenden · 2024-10-09T18:01:11Z

(I see the filesize checks are failing in other PRs, it's not just me...)

sbc100 · 2024-10-09T18:28:44Z

(I see the filesize checks are failing in other PRs, it's not just me...)

Filesize tests rebaselined. Please rebase or merge

sbc100 · 2024-10-09T18:37:23Z

I may just shorten some variable names for now. Ultimately, time permitting (which is wishful thinking), I want to look at what it would take to get the worklet code through Closure, the same as sbc100 did recently with general worklets (IIRC).

You should not need to use short variable names in the source code. We can rely on closure/minifiers to shorten the names.

I think @juj was likely referring to other tricks when he said "I would squeeze some code size bytes out of the JS code"

cwoffenden · 2024-10-09T18:46:59Z

You should not need to use short variable names in the source code.

The worklet doesn’t get minified currently and I add a few extra lines in there to use the struct sizes/offsets instead of magic numbers.

sbc100 · 2024-10-09T18:51:15Z

You should not need to use short variable names in the source code.

The worklet doesn’t get minified currently and I add a few extra lines in there to use the struct sizes/offsets instead of magic numbers.

I'm not sure what you mean. The code running the worklet is the exact same code that runs on the main thread right? i.e. we only generate one single program from the JS library files, right? If you build with --closure it will be minified, right?

cwoffenden · 2024-10-09T18:56:53Z

If you build with --closure it will be minified, right?

I’ll need to recheck tomorrow but I’m pretty sure the property and variable names I added are staying the same (e.g. this.quantumSize doesn’t get shortened).

The library file certainly gets minified (and I’m not adding much that could be written more compact, AFAIK). It’s the worklet JS file I didn’t think was running through Closure?

cwoffenden · 2024-10-10T13:20:12Z

I'd like to see the setting and reading the renderQuantumSize flow to exist, just to make sure we can actually get the renderQuantumSize information into the worklet cleanly.

@juj I don't think we need to, but it wants more investigation. The typed arrays passed in the input and output lists here:

emscripten/src/audio_worklet.js

Line 40 in 429f70c

process(inputList, outputList, parameters) {

Each is the size of the quantum, so if all calculations were based on the lengths of these, then everything would work now and in the future. It shouldn't need the quantum passing in at all, but I'd like to see it in action.

sbc100 · 2024-10-10T15:25:54Z

We can rely on closure/minifiers to shorten the names.

I just verified to be sure, and the generated myfile.aw.js has no difference whether or not it's built with --closure=1. It has whitespace, etc., removed when building with -O2 for example, but Closure doesn't appear to be run.

Ah yes, but that is just the bootstrap code the audio worklet, that main program code (all the code that lives in library_xxx.js) goes in myfile.js which should be covered by closure, and I think that covers all the code in this PR.

cwoffenden · 2024-10-10T16:39:54Z

Ah yes, but that is just the bootstrap code the audio worklet

There are changes to both the bootstrap and library, the bootstrap is mostly the likes of {{{ C_STRUCTS.AudioSampleFrame.__size__ }}} due to changes to the offsets. I'd like to have used makeSetValue but it needs amending to work with how it accesses the worklet heap.

sbc100 · 2024-10-10T16:42:41Z

Ah yes, but that is just the bootstrap code the audio worklet

There are changes to both the bootstrap and library, the bootstrap is mostly the likes of {{{ C_STRUCTS.AudioSampleFrame.__size__ }}} due to changes to the offsets. I'd like to have used makeSetValue but it needs amending to work with how it accesses the worklet heap.

Ah, sorry I didn't see that. Still I don't think you want to be doing minification of variable names or other things that closure generatelly does in this code. A better path forward there would be to work on getting closure to run on that code

cwoffenden · 2024-10-10T17:04:18Z

A better path forward there would be to work on getting closure to run on that code

Unless there's anything hideous I believe this PR is done, then I plan on looking at getting it all through Closure and other tweaks. I need to first move code away from our own Audio Worklet implementation (#12502 which we've been maintaining and shipping since 2020!) beforehand though.

cwoffenden · 2024-10-14T14:49:19Z

Any feedback on this? I've more PRs to follow but I'm building each on the last since they're touching the same files.

I'm happy to mark the current API as deprecated and create a parallel API with the quantum, or go with this current PR and come back to this later. I'm itching to get to my next task of replacing this in the worklet:

for (i of outputList) {
  for (j of i) {
    for (k = 0; k < this.quantumSize; ++k) {
      j[k] = HEAPF32[outputDataPtr++];
    }
  }
}

Which I think is doable because the address never changes (given the worklet's stack is passed in externally) so we can create a view on the channel data valid for the life of the worklet, and thus can perform a garbage-free copy.

A single JS function exists for the quantum size, the value is also passed to the processor node on creation.

juj · 2024-10-15T08:33:51Z

src/library_webaudio.js

+    // It comes two caveats: it needs the hint when generating the context adding to
+    // emscripten_create_audio_context(), and altering the quantum requires a secure
+    // context and fallback implementing. Until then we simply use the 1.0 API value:
+    return 128;


Couldn't this code already today read

return EmAudio[contextHandle]['renderQuantumSize'] || 128;

That way the code would also be verifying that contextHandle is a valid context.

It could, but I was worried about its [SecureContext] marking in the spec and what that entails, and also since we can't yet set it then it doesn't make a difference.

But I'll happily change it and look at it again if/when it breaks, and when the API allows for the setting.

Ok, works either way.

[SecureContext] just means that the parameter won't be populated if the page is served via an insecure http:// handler. The value won't then be present, and will return undefined.

(http://localhost URLs get a special case and is always considered secure).

My concern was it'd throw an exception, not just silently return undefined.

(For localhost it still needs Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy headers for audio demos to run, BTW, since SharedArrayBuffer isn't available and throws a reference error)

My concern was it'd throw an exception, not just silently return undefined.

Ah, right. An exception won't happen, that is guaranteed by the IDL spec of [SecureContext]: https://www.w3.org/TR/secure-contexts/#integration-idl which states multiple times

But let's leave this later for when an implementation comes along.

juj · 2024-10-15T08:35:55Z

src/library_webaudio.js

+    assert(EmAudio[contextHandle] instanceof (window.AudioContext || window.webkitAudioContext), `Called emscripten_audio_context_quantum_size() on handle ${contextHandle} that is not an AudioContext, but of type ${EmAudio[contextHandle]}`);
+#endif
+    return emscriptenGetContextQuantumSize(contextHandle);
+  },


I would delete the second function $emscriptenGetContextQuantumSize and just have one function emscripten_audio_context_quantum_size() and use that - it would be simpler that way?

One was to expose it to JS for use in the worklet or in other code, the other native. Originally I didn't pass the value to the worklet, instead reading it from this JS function, but you'd said to demonstrate the value being passed.

I can re-look at this.

$emscriptenGetContextQuantumSize() gets called in two places from JS (one to pass to now pass to the worklet, the other to expose to native). I guess I could call emscripten_audio_context_quantum_size() from emscripten_create_wasm_audio_worklet_node() since both are JS in the end.

Both versions of the function receive the handle ID? So in that case, I think it would be fine to call _emscripten_audio_context_quantum_size() from JS code as well. (need to add the underscore at the call site though, since the function won't start with a $ prefix)

Both versions of the function receive the handle ID?

I can tidy this in another PR.

juj · 2024-10-15T08:43:40Z

test/webaudio/audio_worklet_tone_generator.c

    {
      float s = emscripten_math_sin(phase);
      phase += phaseIncrement;
      for(int ch = 0; ch < outputs[o].numberOfChannels; ++ch)
-        outputs[o].data[ch*128 + i] = s * currentVolume;
+        outputs[o].data[ch*outputs[o].quantumSize + i] = s * currentVolume;


Given that the quantum size is a constant, I wonder if it would be better to have all these examples use that global quantumSize integer, e.g.

int quantumSize; bool ProcessAudio(int numInputs, ...) { for(int o = 0; o < numOutputs; ++o) for(int i = 0; i < quantumSize; ++i) ... } int main() { quantumSize = emscripten_audio_context_quantum_size(context); }

This would communicate the invariant that the quantumSize cannot change between inputs and outputs in a single call to ProcessAudio(). That might help someone learning about AudioWorklets.

Alternatively, I wonder if the field AudioSampleFrame::quantumSize might be simpler to rename to AudioSampleFrame::length to signal that it just means the length of the data array. Then users could have

for(int i = 0; i < outputs[o].length; ++i) { outputs[o].data[i] = ...;

with very idiomatic reading C code. The "quantumSize" name in this context reads fancier than it necessarily needs to be (and doesn't match the Web Audio spec which calls it "renderQuantumSize").

Works either way, leaving it up to you to decide.

I don't like setting it as a global, and I agree, I also don't like the name quantumSize either (or renderQuantumSize or the use of quantum in general for this, or the way audio frame is used, etc.).

If I'm going to spend more time with this I'd rather that be spent marking the existing API as deprecated and creating a new callback signature.

Gotcha, that makes sense. 👍

I made #22741 since it was easy to do (for me) now this has landed, that calls it samplesPerChannel and bytesPerChannel, which I think is better wording. I only left in emscripten_audio_context_quantum_size() because that's what the API (almost) calls it which affects only advanced API usage.

juj · 2024-10-15T10:05:28Z

I could mark emscripten_create_wasm_audio_worklet_node() as deprecated, add a new one that takes a different EmscriptenWorkletNodeProcessCallback signature, and let the two live side by side for a while? But let me do the minor tweaks here and land this first, I don't know when I'll have free time again.

Yeh, maybe breaking the signature of the audio callback won't be worth it, as the information can be acquired already in two different ways.

I just verified to be sure, and the generated myfile.aw.js has no difference whether or not it's built with --closure=1. It has whitespace, etc., removed when building with -O2 for example, but Closure doesn't appear to be run.

Ohh, this looks like an oversight. It definitely will be good to be Closured when --closure is passed. Though that's definitely something for another PR.

Still I don't think you want to be doing minification of variable names or other things that closure generatelly does in this code.

Yeah, we don't need to manually minify any variable names (when Closure can do the minification for us). In cases where Closure cannot minify variable names (like the key names for postMessage boundaries), then we can do some custom tweaking (like we do with the _wsc things etc.)

juj · 2024-10-15T10:11:17Z

Thanks for working on this!

cwoffenden · 2024-10-15T10:25:31Z

Thanks for working on this!

It's actually fun, so it's not really work!

@juj

Following from @juj 's suggestion in #22681, the use of quantum has been reduced to only where it refers to the API. Internally it's named `samplesPerChannel` to match the `numberOfChannels` in the `AudioSampleFrame`. (This is entirely optional and doesn't change anything other than the naming.)

cwoffenden changed the title ~~[AudioWorklet] replace repeated use of 128 with defined constant~~ [AUDIO_WORKLET] replace repeated use of 128 with defined constant Oct 4, 2024

cwoffenden force-pushed the cw-audio-tweaks-1 branch from a275a20 to 0e9958e Compare October 4, 2024 19:43

cwoffenden mentioned this pull request Oct 8, 2024

[AUDIO_WORKLET] Add new emscripten_audio_node_connect API #22667

Merged

cwoffenden force-pushed the cw-audio-tweaks-1 branch from 0e9958e to e96adc5 Compare October 8, 2024 09:23

cwoffenden marked this pull request as draft October 8, 2024 16:16

cwoffenden force-pushed the cw-audio-tweaks-1 branch 2 times, most recently from c80eb2a to 9bab4e5 Compare October 8, 2024 16:53

cwoffenden marked this pull request as ready for review October 8, 2024 17:28

sbc100 reviewed Oct 8, 2024

View reviewed changes

system/include/emscripten/webaudio.h Show resolved Hide resolved

cwoffenden changed the title ~~[AUDIO_WORKLET] replace repeated use of 128 with defined constant~~ [AUDIO_WORKLET] Added API for getting the buffer's quantum size Oct 8, 2024

sbc100 reviewed Oct 8, 2024

View reviewed changes

test/webaudio/audio_worklet_tone_generator.c Outdated Show resolved Hide resolved

cwoffenden force-pushed the cw-audio-tweaks-1 branch from ff8620c to bc67d74 Compare October 8, 2024 21:46

sbc100 approved these changes Oct 8, 2024

View reviewed changes

src/audio_worklet.js Outdated Show resolved Hide resolved

cwoffenden force-pushed the cw-audio-tweaks-1 branch from 66dfe2f to 274cc56 Compare October 9, 2024 06:04

juj reviewed Oct 9, 2024

View reviewed changes

juj approved these changes Oct 9, 2024

View reviewed changes

cwoffenden force-pushed the cw-audio-tweaks-1 branch from 8e1115a to b9ce59c Compare October 9, 2024 18:30

cwoffenden force-pushed the cw-audio-tweaks-1 branch from b9ce59c to 1ecfb23 Compare October 10, 2024 12:35

cwoffenden added 10 commits October 15, 2024 07:20

Replace 'magic' 128 number with a define

ea8aa11

Work in progress (something broke...)

58422d0

Fixed copy and paste...

559d097

Updated docs

ab4671a

Added simple test to get the quantum size from the context

f0596cf

Simplified test further (printf the result)

7ff2a00

Formatting

24fb2af

Created a single source for querying the quantum size

6a26421

A single JS function exists for the quantum size, the value is also passed to the processor node on creation.

Minor: follow same JS style

8ed9588

Added note for future work

5249dbb

cwoffenden force-pushed the cw-audio-tweaks-1 branch from e6ad164 to 5249dbb Compare October 15, 2024 05:20

juj reviewed Oct 15, 2024

View reviewed changes

juj mentioned this pull request Oct 15, 2024

Fix audio worklet + memory64 #22731

Merged

juj approved these changes Oct 15, 2024

View reviewed changes

juj merged commit ac381b8 into emscripten-core:main Oct 15, 2024
28 checks passed

cwoffenden deleted the cw-audio-tweaks-1 branch October 15, 2024 10:25

cwoffenden mentioned this pull request Oct 15, 2024

[AUDIO_WORKLET] Reword API to make it clearer #22741

Merged

[AUDIO_WORKLET] Added API for getting the buffer's quantum size #22681

[AUDIO_WORKLET] Added API for getting the buffer's quantum size #22681

Conversation

cwoffenden commented Oct 4, 2024 • edited Loading

juj commented Oct 8, 2024

cwoffenden commented Oct 8, 2024

cwoffenden commented Oct 8, 2024

Choose a reason for hiding this comment

cwoffenden Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

juj commented Oct 9, 2024

juj commented Oct 9, 2024

juj left a comment

Choose a reason for hiding this comment

cwoffenden commented Oct 9, 2024 • edited Loading

cwoffenden commented Oct 9, 2024

cwoffenden commented Oct 9, 2024

sbc100 commented Oct 9, 2024

sbc100 commented Oct 9, 2024

cwoffenden commented Oct 9, 2024

sbc100 commented Oct 9, 2024

cwoffenden commented Oct 9, 2024 • edited Loading

cwoffenden commented Oct 10, 2024

sbc100 commented Oct 10, 2024

cwoffenden commented Oct 10, 2024

sbc100 commented Oct 10, 2024

cwoffenden commented Oct 10, 2024

cwoffenden commented Oct 14, 2024

juj Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwoffenden Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwoffenden Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

juj commented Oct 15, 2024

juj commented Oct 15, 2024

cwoffenden commented Oct 15, 2024

cwoffenden commented Oct 4, 2024 •

edited

Loading

cwoffenden Oct 9, 2024 •

edited

Loading

cwoffenden commented Oct 9, 2024 •

edited

Loading

cwoffenden commented Oct 9, 2024 •

edited

Loading

juj Oct 15, 2024 •

edited

Loading

cwoffenden Oct 15, 2024 •

edited

Loading

cwoffenden Oct 15, 2024 •

edited

Loading