Web-audio-api: Improve description of parameters for process() calls on client objects extending AudioWorkletProcessor

[karlt]

https://webaudio.github.io/web-audio-api/#rendering-loop says 'Let processFunction be the result of a Get(O=processor, P="process")', and so processFunction is an ECMAScript entity.

The process() method is not defined as WebIDL callback type, but its parameters appear to have WebIDL types.
https://webaudio.github.io/web-audio-api/#dom-audioworkletprocessor-process-inputs-outputs-parameters-inputs says that inputs has type sequence<sequence<Float32Array>> and links to the WebIDL type https://heycam.github.io/webidl/#idl-Float32Array

https://webaudio.github.io/web-audio-api/#dom-audioworkletprocessor-process-inputs-outputs-parameters-parameters says parameters is "A map of string keys and associated Float32Arrays", also linking to https://heycam.github.io/webidl/#idl-Float32Array.
Is this https://heycam.github.io/webidl/#idl-record ?

[karlt]

https://drafts.css-houdini.org/css-paint-api-1/#dom-paintworkletglobalscope-registerpaint has

1. Let paint be the result of converting paintValue to the Function callback function type. Rethrow any exceptions from the conversion.

and uses https://heycam.github.io/webidl/#invoke-a-callback-function to invoke a paint callback. Is that what is intended here?

[karlt]

The best design to support efficient implementations would allow all objects to be maintained for reuse across process() calls.

There are some steps in https://heycam.github.io/webidl/#invoke-a-callback-function that look useful. If using WebIDL to describe the call, then an efficient implementation means using WebIDL types that pass by reference instead of copy in the conversions. The object type would be suitable, and could be combined with a description of the underlying objects.

A, perhaps iterable, interface type with an indexed property getter would also be more efficient than sequence. I assume it could even be declared to behave in entirely the same way as Array, if desired, but perhaps this would be very verbose in WebIDL.

FrozenArray would be another efficient option, suitable for inputs, inputs[n], and the outer object outputs. See https://github.com/WebAudio/web-audio-api/issues/1515

An interface with a named property getter would be a possible alternative to object for the parameters argument, to support the specified parameters["name"] usage.

[hoch]

The best design to support efficient implementations would allow all objects to be maintained for reuse across process() calls.

This already has been ruled out. Because we can't keep these objects from being transferred to a different thread. (We discussed it one or two years ago, but I have to look through the find the right issue.)

Also freezing the input/output array also punted to the V.next. (#1515, and you already commented on it)

[hoch]

With that said, the confusion between WebIDL and ECMAScript seems like an editorial issue. We'll try to address that in the next WG meeting.

[karlt]

https://heycam.github.io/webidl/#invoke-a-callback-function would "perform a microtask checkpoint" in "clean up after running script", but that would be in conflict with "Any Promise resolved within the execution of process method will be queued into the microtask queue in the AudioWorkletGlobalScope" in https://webaudio.github.io/web-audio-api/#rendering-loop

If we can't use "invoke a callback function", then WebIDL may not be of any benefit here and it may be simpler to describe the parameters as ECMAScript Arrays and an Object. (The parameters parameter is not a Map.)

[hoch]

I am a bit confused what problems we want to solve here.

1. The confusing mixture of ECMAScript and WebIDL
2. The conflict between invoke-a-callback-function and Web Audio API's rendering loop algorithm.
3. Reusing objects for multiple process() calls.

All of these are quite different. @karlt Can you pinpoint and change the issue title accordingly?

[karlt]

The key issue is the description of the parameters. Part of the lack of clarity comes from the mix of ECMAScript and WebIDL types. The rest is a discussion of the implications of various options here.

This issue is about the Arrays and the "map" object (which contain the Float32Arrays). These do not have a buffer to detach, and they are not Transferable. https://github.com/WebAudio/web-audio-api/issues/1934 covers the Float32Arrays, whose types are clearly defined, but whose behavior is not completely described.

Some of the lack of clarity has come from process() changing from a WebIDL method to an ECMAScript method.

It appears that the consequence of sequence requiring new object creation each time may not have been fully understood. I thought the general understanding was that garbage creation and memory allocation are best avoided on the rendering thread, and so I would be surprised if the intention was to unnecessarily require creation of single-use objects. If sequence had been understood to imply a copy then https://github.com/WebAudio/web-audio-api/issues/1515 would not exist, because there is no way for the caller to know whether a copy of its argument is modified.

Prior to the introduction of sequence<sequence<T>>, inputs and outputs were declared as Float32Array[][]. sequence<sequence<T>> appears to have been somewhat randomly chosen based on https://github.com/WebAudio/web-audio-api/pull/869#issuecomment-235754363
However, the same comment pointed out the "new sequence every time" problem with sequence in a different part of the API. It also stated that process() is "supposed to implemented by the subclass, in which case it should not be in the IDL at all." Since that comment, IDL language was introduced to describe its parameters (https://github.com/WebAudio/web-audio-api/commit/75ca59fc82e3fca5530e0a17eb81c98fc5ab2afa#diff-eacf331f0ffc35d4b482f1d15a887d3bR5342) and then process() was moved out of IDL (https://github.com/WebAudio/web-audio-api/pull/963/files#diff-eacf331f0ffc35d4b482f1d15a887d3bL4920).

An associated comment explained https://github.com/WebAudio/web-audio-api/pull/869#issuecomment-238444178

The IDL is just instructions for how bindings generators in implementations should perform conversions and create APIs. It doesn't have anything to do with the web developer-facing interface and requirements for using the API. That is better documented in, well, documentation, not in normative interface definition language."

I don't feel as strongly as that comment. While it is possible to describe process() entirely in ECMAScript concepts, consistent use of WebIDL might be a simpler way to clarify some things. It might also be useful if we wish a WebAssembly binding to also be applicable without separate specification. We may even be able to find a way to make https://heycam.github.io/webidl/#invoke-a-callback-function work, but, if so, we should seriously consider whether sequence is what we really want.

The issue with the parameters parameter is not so much about WebIDL. The information that this is an Object (which is an ECMAScript concept) appears to have been accidentally removed in https://github.com/WebAudio/web-audio-api/pull/1618/files#diff-ec9cfa5f3f35ec1f84feb2e59686c34dL9897

[padenot]

F2F resolution: Performance is important and this will lead to changes to the spec.

1. Move inputs and outputs back to Float32Array[][] which are more performant (in particular because this means that implementations can reuse the objects and generate minimal garbage).
2. For parameters, the goal is the same. We need to figure out whether using an Object allows for efficient reuse.

[hoch]

I am assigning this to @padenot because the issue came up from FireFox's AW implementation.

[hoch]

F2F conclusion from TPAC 2019:

• Use a proper WebIDL callback type for AudioWorkletProcessor.process() function.
• The function signature will be process(FrozenArray<FrozenArray<Float32Array>>, object, object).

Action items:

• Ask the spec/IDL experts if reusing object ID type is feasible.

[murillo128]

One question, if the outputs is an Object, would the javascript code be able to modify the contents of the output array passed to process()?

For example, is the following code which replaces the outputs[0] array with a new one expected to work?

  process(inputs, outputs) {
    // By default, the node has single input and output.
    const input = inputs[0];
    const output = output[0]

    for (let channel = 0; channel < output.length; ++channel) {
      //Create a new Float32Array instead of using the provided one
      output[channel] = new Float32Array(input[channel]);
    }

    return true;
  }

[karlt]

Currently, replacing outputs[0] is not expected to lead to the new FloatArray32 being read by the implementation for output data. https://github.com/WebAudio/web-audio-api-v2/issues/42 raises the question of what should happen in the presence of modifications. There are situations where it would be useful if the process() method can determine the number of channels of output. See also https://github.com/WebAudio/web-audio-api-v2/issues/37.

Those have been pushed to v2, so the preferred direction here would be one that permitted such extension in the future.

There is no intention to allow numberOfOutputs to be dynamically changed and so FrozenArray<object> outputs would be fine.

Whether a future design would allow the process method to provide Float32Arrays or merely to set the length on output in a way that output provided the new Float32Array has not been decided.

[padenot]

I talked with Boris about all this today.

We can (should) use frozen object for the 3rd parameter. The parameter name and value don't ever change, what changes is only the values of the value (i.e. the values of the Float32Array). I don't see a use-case for mutating the parameter arrays.

We have to use a plain object for the second one, if we decide that a Processor can change its output channel count for a particular output, dynamically. We could imagine another API to do this, that would allow optimizations for real-time safety (in particular, only changing the channel count in between render quantum, and calling the _next_ process with the new channel count next time, to avoid allocating explicitly in JS, so that the implementation can use a pool of pre-allocated objects). We need to spec properly what happens when the object is mutated.

We can make the first object FrozenArray<FrozenArray<Float32Array>>, because I don't see a use-case for mutating the inputs: authors that want to do this need to change the graph topology or maybe the various input mixing settings on the AudioNodes, rather than doing it inside of the process callbacks.

We can use a WebIDL callback for process, and have it perform the microtask checkpoints at the time we want (after all the process methods have been called for the current render quantum), if we still have a script on the stack (https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script), which we should do.

This way, the amount of garbage that is created by the browser API is minimal: authors that need the highest performance can have zero-garbage processing, and everything is specced properly.

[padenot]

In addition, it would be nice to be able to use [[ArrayBufferDetachKey]], as explained in https://html.spec.whatwg.org/#structuredserializewithtransfer and https://tc39.es/ecma262/#sec-detacharraybuffer. Because it is the implementation that creates those buffers, we can create them with a detach key, which means that content won't be able to detach them.

[hoch]

"frozen object"; does such thing exist in WebIDL? If it doesn't, it means that we have to develop a new IDL data type.

We have to use a plain object for the second one, if we decide that a Processor can change its output channel count for a particular output, dynamically.

I agree with using a plain object for the second arg, but I will have to think about being able to change the output channel dynamically. This implicates a large-scale change in Chromium code. Also I have never seen any audio APIs that allow dynamic buffer size change within the audio processing code itself. Do we have any AudioNode that changes the output channel count while it's processing the DSP kernel?

We can make the first object FrozenArray>, because I don't see a use-case for mutating the inputs: authors that want to do this need to change the graph topology or maybe the various input mixing settings on the AudioNodes, rather than doing it inside of the process callbacks.

100%. But it seems like the construction of FrozenArray type is involved with the usage of sequence. By design, the sequence type generates a new container, so we might want to double check that.

This way, the amount of garbage that is created by the browser API is minimal: authors that need the highest performance can have zero-garbage processing, and everything is specced properly.

From real-world data points - In many cases, the GC hiccups that devs are having come from the other sides of the web platform. We should definitely try to minimize the garbage from AWP, but I also want to point out there are bigger obstacles.

[rtoy]

On Thu, Mar 26, 2020 at 11:41 AM Hongchan Choi [e-mail]
wrote:

"frozen object"; does such thing exist in WebIDL? If it doesn't, it means
that we have to develop a new IDL data type.

We have to use a plain object for the second one, if we decide that a
Processor can change its output channel count for a particular output,
dynamically.

I agree with using a plain object for the second arg, but I will have to
think about being able to change the output channel dynamically. This
implicates a large-scale change in Chromium code. Also I have never seen
any audio APIs that allow dynamic buffer size change within the audio
processing code itself. Do we have any AudioNode that changes the output
channel count while it's processing the DSP kernel?

Of course we do, but they're all in response to the input changing in some
way. LIke a gain node with a mono source and then a 5.1 ABSN is connected.
The output (eventually) has 6 channels.

So perhaps you're thinking of something else?

We can make the first object FrozenArray, because I don't see
a use-case for mutating the inputs: authors that want to do this need to
change the graph topology or maybe the various input mixing settings on the
AudioNodes, rather than doing it inside of the process callbacks.

100%. But it seems like the construction of FrozenArray type is involved
with https://heycam.github.io/webidl/#es-frozen-array the usage of
sequence. By design, the sequence type generates a new container, so we
might want to double check that.

Also, from Ecmascript, if you freeze an array, you can't change the
elements as suggested by the example in
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze.
That's right from the user's viewpoint, but not quite so from the
implementation. Unless the implementation can change the frozen feature and
write new values and freeze it again before sending it to process.

[karlt]

"frozen object"; does such thing exist in WebIDL? If it doesn't, it means that we have to develop a new IDL data type.

"frozen object" does not exist in WebIDL, but object does. There is no need to introduce a new IDL type because object can be used for the WebIDL declaration and prose could describe how to set up and freeze that object.

But it seems like the construction of FrozenArray type is involved with the usage of sequence. By design, the sequence type generates a new container, so we might want to double check that.

There would be a new JS Array container on construction of the FrozenArray, but the same FrozenArray can usually be re-used on the next call. There are exceptions where a new FrozenArray would need to be created, as implied by @rtoy.

"The result of converting an IDL FrozenArray value to an ECMAScript value is the Object value that represents a reference to the same object that the IDL FrozenArray represents" so there is no copy on passing a FrozenArray to JS.

[karlt]

Also, from Ecmascript, if you freeze an array, you can't change the elements as suggested by the example in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze. That's right from the user's viewpoint, but not quite so from the implementation. Unless the implementation can change the frozen feature and write new values and freeze it again before sending it to process.

The implementation cannot unfreeze and refreeze.

With a FrozenArray created from a sequence, the implementation would need to create a new container when one of its properties changes. When the number of channels of an input changes, the container FrozenArray of the Float32Arrays would need to be replaced with a new container. That would also require replacing the outer container of FrozenArrays.

At least with a frozen object, there is an option of way around this. The property still cannot change but the property can be defined to be a getter which can return a different inner object. e.g a different Float32Array when then length changes between 1 and 128 according to whether the parameter is constant for the render quantum. There is a question as to whether this would be better spec'ed as a WebIDL interface with a named property getter.

For FrozenArray, instead of creating from a sequence, I guess the Web Audio API could specify how the FrozenArray properties are defined and so do something similar, but I can't think of a good way that the length of the Array could be allowed to change. So a new container would still be required to hold a new number of Float32Arrays (for a channel count change), but the outer container of FrozenArrays would not need to be recreated if its property getters return the new inner objects.

A, perhaps iterable, WebIDL interface with an indexed property getter is an alternative to FrozenArray. There would be no need to recreate the object when the number of channels changes. It would behave similarly to a JS Array in many ways, but it would not be a JS Array.

[hoch]

rtoy@

Of course we do, but they're all in response to the input changing in some
way. LIke a gain node with a mono source and then a 5.1 ABSN is connected.
The output (eventually) has 6 channels.

No. IIRC Chrome does configure the input/output channel first, then run the processing code. So the processing code itself cannot change the channel configuration while it is running.

@karlt

Thanks for the explanation. Boris is already involved, so I will follow how the discussion goes. I just want to be careful so we don't make another mistake like using sequence.

[padenot]

Also I have never seen any audio APIs that allow dynamic buffer size change within the audio processing code itself. Do we have any AudioNode that changes the output channel count while it's processing the DSP kernel?

I don't know of any myself either, but I think we left this open as an option during the F2F in Seattle. The alternative design was to have a control thread API to change it.

Again, it's useful to change the channel count from within the callback (this is does very often in the implementation of native nodes, in between render quantum). Perhaps it would be wiser to devise an API in the AudioWorkletGlobalScope to change the channel input and output configuration, in which case we can Freeze all arrays here, and design this API later, if we are OK with the fact that changing the output layout will not happen via mangling the output parameter.

The alternative design is a bit more in line with what authors familiar with other audio APIs would expect. In the VST world (to take an example I know), the plugin initially tells the DAW the input and output channel layout. Then DAW can tell the plugin the set of input/output channels the DAW would like to have processed by the plugin. The plugin can say that it cannot process a particular input/output channel setup. Depending on the version of the SDK and the particular flavour of implementation, the plugin can be notified of a change of input/output layout.

The DAW/Plugin model is less dynamic than the Web Audio API however, and the Web Audio API has good reasons to do it like it does, so we need to be careful not to adopt a particular solution because other audio software do it that way.

Having two APIs for this seems inevitable. Useless duplication of APIs is something we should strive to avoid, but having two ways of controlling the channel layout is very much _not_ useless, here.

[padenot]

There is two part to this discussion:

1. Whether or not the arrays are frozen. I'm leaning towards freezing all arrays. This means a bit of work on the render thread when the input/output layout change. This is inevitable, even in native (pre-allocation and pointer tricks make it cheap, however, JavaScript doesn't make this particularly easy). Using a getter to allow switching between arrays of length 1 and 128 seems like a good solution.
2. Whether or not Float32Arrays have a non-default ArrayBufferDetachKey. This seem like it would help reducing the amount of checks implementations have to do before calling process, and this translates to improve performances for users. Additionally, it would clearly signal the fact that attempting to detach any Float32Array that have been passed down to authors via the parameters of the process function is a bad idea, and people should no do that. We can prevent it, and we should.

[hoch]

Perhaps it would be wiser to devise an API in the AudioWorkletGlobalScope to change the channel input and output configuration, in which case we can Freeze all arrays here, and design this API later, if we are OK with the fact that changing the output layout will not happen via mangling the output parameter.

I agree with this idea given the phase that we're in. I would like to defer the output channel mutation to V2.

The DAW/Plugin model is less dynamic than the Web Audio API however, and the Web Audio API has good reasons to do it like it does, so we need to be careful not to adopt a particular solution because other audio software do it that way.

Also agreed. But we should still prioritize the performance over the dynamism, at least from my perspective.

Having two APIs for this seems inevitable.

Can you clarify what two APIs we are talking about?

Whether or not the arrays are frozen. I'm leaning towards freezing all arrays. This means a bit of work on the render thread when the input/output layout change.

I am also leaning toward making everything immutable. If the output layout change is needed for some reasons, I think we should utilize something like AudioNode.channelCounts or AudioWorkletNode.outputChannelCounts as other AudioNodes do. That said, this should be in the V2 scope.

I guess the buffer transferability will be the issue when we go down this path (several production apps use AW for audio capturing/transferring), but that can be solve by user-provided buffering in many flavors.

[padenot]

Can you clarify what two APIs we are talking about?

An api to change the output channel count from within the rendering thread, and an API to change the channel count from the control thread.

Both are needed to re-implement native nodes efficiently.

A DelayNode or any other node that has a tail changes its channel count automatically depending on its inputs. This happens in between render quantum. This capability is easily added in V2.

Authors probably also want to dynamically reconfigure AudioWorkletNodes, with an API similar to the native nodes, as you note. Similarly this can happen in V2.

If we agree on this we can freeze everything, and use getters for the parameters and possibly input buffers, to signal respectively a constant parameter, and silence (this has been discussed, we agreed that it would be a useful capability, but we didn't do it). We can use Float32Array directly for the output buffers.

I guess the buffer transferability will be the issue when we go down this path (several production apps use AW for audio capturing/transferring), but that can be solve by user-provided buffering in many flavors.

The problem of transfer-ability goes away if we're using ArrayBufferDetachKey. Any application transferring buffers provided by the process method will be broken, and that's a good thing, they shouldn't have done that in the first place. Those details should have been locked down before any implementation shipping, but this boat has sailed. I'm not willing to compromise performances in the long run for everybody because of this.

[hoch]

A DelayNode or any other node that has a tail changes its channel count automatically depending on its inputs. This happens in between render quantum. This capability is easily added in V2.

How important is this? What else do we have to consider except for DelayNode's tail time? Do we really want to do this so developers can reimplement DelayNode's tail time? Anyhow, this should be a V2 issue, so I am not going to delve in too much.

If we agree on this we can freeze everything, and use getters for the parameters and possibly input buffers, to signal respectively a constant parameter, and silence (this has been discussed, we agreed that it would be a useful capability, but we didn't do it). We can use Float32Array directly for the output buffers.

All these for V2. Please correct me if I am mistaken.

The problem of transfer-ability goes away if we're using ArrayBufferDetachKey.

Is this specced somewhere? I found this, but it seems like it is still in the works.

Any application transferring buffers provided by the process method will be broken, and that's a good thing, they shouldn't have done that in the first place.

I can't agree with this. Some applications accumulate and send buffers the other thread for capturing purpose. They don't do that in process() method directly, but keep buffers and transfer when needed/requested. Why is that wrong and why do they need to be broken?

Those details should have been locked down before any implementation shipping, but this boat has sailed.

From my perspective, leaving the spec/implementation in limbo for more than several years without a proper replacement of ScriptProcessorNode is even worse. In Chrome, we have learned a lot from the launch in 2017 and made improvements from it.

I'm not willing to compromise performances in the long run for everybody because of this.

I mentioned this already before, the bottleneck in the real-world application is not the garbage created by AWP in most cases. Also I don't think Chrome is willing to compromise performance. No browser implementer wants to do such things.

[padenot]

A DelayNode or any other node that has a tail changes its channel count automatically depending on its inputs. This happens in between render quantum. This capability is easily added in V2.

How important is this? What else do we have to consider except for DelayNode's tail time? Do we really want to do this so developers can reimplement DelayNode's tail time? Anyhow, this should be a V2 issue, so I am not going to delve in too much.

Any AudioNode that can have a tail: DelayNode, ConvolverNode, DynamicsCompressor, PannerNode in HRTF mode, BiquadFilterNode, IIRFilterNode. It's a V2 issue indeed, if we agree that mangling the arrays is not the way to go (and I think we agree).

If we agree on this we can freeze everything, and use getters for the parameters and possibly input buffers, to signal respectively a constant parameter, and silence (this has been discussed, we agreed that it would be a useful capability, but we didn't do it). We can use Float32Array directly for the output buffers.

All these for V2. Please correct me if I am mistaken.

This is for V1 (see below). The shape of the design is important to allow improvements in V2.

The problem of transfer-ability goes away if we're using ArrayBufferDetachKey.

Is this specced somewhere? I found this, but it seems like it is still in the works.

This is in current ECMAScript: https://tc39.es/ecma262/#sec-properties-of-the-arraybuffer-instances. It's in use currently in browsers that ship a WASM impl to make it impossible to detach bits of the WASM heap as your link notes.

Any application transferring buffers provided by the process method will be broken, and that's a good thing, they shouldn't have done that in the first place.

I can't agree with this. Some applications accumulate and send buffers the other thread for capturing purpose. They don't do that in process() method directly, but keep buffers and transfer when needed/requested. Why is that wrong and why do they need to be broken?

Detaching Float32Array by post messaging means that the browser has to re-allocate those arrays next time process is called, for each AudioWorkletProcessor that has its arrays detached.

This means we're forcing implementations to allocate from inside the real-time thread, which is a big mistake in real-time programming. It's the rule #1, and always has been.

http://www.rossbencina.com/code/real-time-audio-programming-101-time-waits-for-nothing is the classic blog post that explains the problem, and it begins with direct quotes from the audio programmers behind the system API of the OS on which our browsers run. [0] is a talk from the Audio Developer Conference 2016 that also explains the problems, but any literature on the matter is pretty clear about this.

process is being called 344 times a second _at minimum_ (if we have a single AudioWorkletProcessor and we're running at 44.1kHz), from a real-time thread, we can't afford breaking the rules, or exposing an API that encourages breaking the rules.

Getting data out of a real-time thread always has been done with a ring buffer or other similar mechanism (buffer ping-ponging, etc.), copying them out, for this reason.

Those details should have been locked down before any implementation shipping, but this boat has sailed.

From my perspective, leaving the spec/implementation in limbo for more than several years without a proper replacement of ScriptProcessorNode is even worse. In Chrome, we have learned a lot from the launch in 2017 and made improvements from it.

This is not something that can be improved on unfortunately, or I wouldn't be stressing it, it has to be right.

I'm not willing to compromise performances in the long run for everybody because of this.

I mentioned this already before, the bottleneck in the real-world application is not the garbage created by AWP in most cases. Also I don't think Chrome is willing to compromise performance. No browser implementer wants to do such things.

I'm not talking about garbage collector, which is one problem, but can be mitigated: it's a non problem in our implementation, and I expect the GC in V8 and others to be excellent for prototyping as well. It's still a problem for professional apps, but there are ways around it, such as using WASM.

I'm talking about allowing and encouraging patterns that result in the browser making unnecessary allocations with a system allocator, which can take locks and make system calls, from real-time threads. This is not something that can be mitigated after the fact regardless of the number of optimizations we do. Not doing this means we're compromising on performance, this is not a theoretical concern.

[jackschaedler]

I'm going to jump in here as a community group member and web developer with a huge interest in the success of these APIs, and an even greater appreciation for the work that you all are doing to make the web a great platform for music and art. I obviously don't have the requisite background as an implementer to speak insightfully about the very detailed particulars of the specification or implementation, but maybe it'll be useful/helpful/amusing if I chime in here from an outsider's perspective. Please tell me to buzz off if these comments aren't helpful!

The conversation around buffer allocation and process caught me eye:

First, one thing is unclear to me: Do implementations allocate the buffers for process on every single call, or only under certain circumstances? In other words, when I run this example (https://github.com/GoogleChromeLabs/web-audio-samples/blob/master/audio-worklet/basic/noise-generator/noise-generator.js) are buffers being allocated for each call to process, or only if I start maintaining references to those buffers and/or posting them back to the main thread? I'm assuming/hoping it's the latter?

More generally, I tend to agree with @padenot's statement, for the reasons that he outlined (this leads to allocations in the render thread):

it would clearly signal the fact that attempting to detach any Float32Array that have been passed down to authors via the parameters of the process function is a bad idea, and people should not do that. We can prevent it, and we should.

I would never advocate for breaking existing applications, but I wonder if it would be possible to get the spec changed for this case so FF could implement it as intended, and then folks with apps that currently use this method of record/capture in Chromium could slowly transition over to an implementation that's more real-time safe. If example code were provided (like Paul did for the ring buffer: https://github.com/padenot/ringbuf.js) it seems like this might be acceptable for many developers.

It's not a great example, but apps using the Web Audio API have broken in the past (autoplay restrictions). That's hugely saddening, but at least this would be a breaking change that would ultimately be better for everyone who's doing audio development in the long run.

[padenot]

First, one thing is unclear to me: Do implementations allocate the buffers for process on every single call, or only under certain circumstances? In other words, when I run this example (https://github.com/GoogleChromeLabs/web-audio-samples/blob/master/audio-worklet/basic/noise-generator/noise-generator.js) are buffers being allocated for each call to process, or only if I start maintaining references to those buffers and/or posting them back to the main thread? I'm assuming/hoping it's the latter?

Speaking with my Firefox developer hat: Firefox doesn't allocate on every process, but has to check that the arrays haven't been mangled/detached on every process. This is a small overhead, but certainly measurable (low single digit percent at 100% load). If it finds that those arrays are not of the right shape, of that the memory has been detached, it needs to allocate, and this is unfortunately done on the real-time thread. This is on the other hand very very clearly measurable and results in glitches under load. Maintaining references to the input/output/parameter arrays doesn't require us to re-allocate, as long as the arrays are intact and the memory is not detached. The usefulness of this is close to zero, because the array values are zeroed-out (for the output buffers), or overwritten (for the input and parameter arrays) because process is called.

[hoch]

Re https://github.com/WebAudio/web-audio-api/issues/1933#issuecomment-613640613:

This means we're forcing implementations to allocate from inside the real-time thread, which is a big mistake in real-time programming. It's the rule #1, and always has been.

Let us remind that this was a mistake from our understanding on WebIDL sequence. No one intended this in the first place and we are here to fix it.

Any application transferring buffers provided by the process method will be broken, and that's a good thing, they shouldn't have done that in the first place.

This is the specific comment I cannot agree with. An application might be using a bad pattern, but we should not punish it by rendering it nonfunctional. It might cause glitches, and we can direct devs to follow up with a better pattern.

This is in current ECMAScript: https://tc39.es/ecma262/#sec-properties-of-the-arraybuffer-instances.

I'll have to consult our WASM team about this. I can't find any instance of this property in Chromium code base.

I'm talking about allowing and encouraging patterns that result in the browser making unnecessary allocations with a system allocator

Yes. I'll take the path of encouraging instead of punishing apps with a complete shutdown. It seems like ArrayBufferDetachKey will break some of applications that are already out there.

If it finds that those arrays are not of the right shape, of that the memory has been detached, it needs to allocate, and this is unfortunately done on the real-time thread.

Yes. Why can't in be done this way? Why do we have to completely break the existing apps?

[hoch]

Re https://github.com/WebAudio/web-audio-api/issues/1933#issuecomment-615110652:

First, one thing is unclear to me: Do implementations allocate the buffers for process on every single call, or only under certain circumstances?

The spec is currently using sequence which does memory allocation every render quantum. We all are agreeing that we need a resolution for this.

If example code were provided (like Paul did for the ring buffer: https://github.com/padenot/ringbuf.js) it seems like this might be acceptable for many developers.

Chrome published 2 articles about Audio Worklet with demo codes. Perhaps it was not useful to many folks, but I know some of our partners followed the guidance.

That's hugely saddening, but at least this would be a breaking change that would ultimately be better for everyone who's doing audio development in the long run.

But I believe there's a way of not breaking existing apps while bringing improvement. So my question is: why can't we do that instead?

[hoch]

My opinions on this issue are:

1. Use FrozenArray<FrozenArray<Float32Array>>> instead of sequence for both input and output.
2. Transferring these buffer to the other threads should not be completely banned. The implementation will reuse buffer for every render quantum, and allocate new memory if needed.
3. Changing the buffer configuration within process() call should be V2 feature.

[jackschaedler]

Thanks for the clarification @padenot & @hoch!

Transferring these buffer to the other threads should not be completely banned. The implementation will reuse buffer for every render quantum, and allocate new memory if needed.

I guess that seems reasonable so long as the check for whether new memory is needed isn't costly.

[padenot]

Re #1933 (comment):

This means we're forcing implementations to allocate from inside the real-time thread, which is a big mistake in real-time programming. It's the rule #1, and always has been.

Let us remind that this was a mistake from our understanding on WebIDL
sequence. No one intended this in the first place and we are here to fix it.

sequence only is a problem because it forces having new outer arrays each time (quoting step 2 here):

1. Let A be a new Array object created as if by the expression [].

This is a problem because it makes it impossible for authors to have a guaranteed zero-gc path, which is necessary to be able to reach high load without glitching because of under-runs. In practice as we said previously generational GC makes this very very fast, but it's still best to not do it.

Float32Array are passed by reference so it was always allowed to reuse the memory, quoting the important bit below because I can't find a way to link to it:

The result of converting an IDL value of any buffer source type to an
ECMAScript value is the Object value that represents a reference to the same
object that the IDL value represents.

The problem would have been sequence<sequence<sequence<float>>> but we've decided early on to not do this.

If it finds that those arrays are not of the right shape, of that the memory
has been detached, it needs to allocate, and this is unfortunately done on the
real-time thread.

Yes. Why can't in be done this way? Why do we have to completely break the
existing apps?

Hitting the system allocator on a real-time thread cannot be made to work reliably, because system allocators can and do take locks and make system calls.

Other solutions exist, but require things like pre-allocation or pooling, which means increased memory usage, and a lot of AudioWorkletNode/Processor pairs can be instantiated. Modern allocators are extremely fast and it will work most of the time without glitching (if the app load is not too high), but it will glitch at some stage, and we're talking around 750 allocations per second (two buffers of 128 floats at 48kHz, 2 * 48000 / 128. = 750) for a single AudioWorkletProcessor that is receiving a stereo input, for _each_ side (input/output).

It seems like a path forward would be the following (echoing the points @hoch made in https://github.com/WebAudio/web-audio-api/issues/1933#issuecomment-616632754):

1. Agreed
2. We can live with this. In Firefox, this check is cheap enough to not matter (essentially a couple loads and a branch, on memory that is hot, in the case where the memory hasn't been detached, and all bets are off if the memory has been detached, but authors can decide to not do this). I'll open an issue for v2 for a backwards compatible way to make those non-detachable, which we'll need to have regardless for better WASM integration (which I don't think is a controversial feature, it's just that the group hasn't worked on it yet).
3. Agreed

So I think we can proceed and write the prose for this. @hoch, I can take this if that's ok with you.

[hoch]

Hitting the system allocator on a real-time thread cannot be made to work reliably, because system allocators can and do take locks and make system calls.

We all are aware of the consequence of the mistake. My question was more about the strategy of WG. We should do our best so the change like this does not punish API users. Punishing many developers because they were using the original design provided by WG using doesn't seem completely fair to me.

in the case where the memory hasn't been detached, and all bets are off if the memory has been detached, but authors can decide to not do this

Yeap. I think this is way more forgiving and this will eventually be another opportunity to highlight the best practice.

[hoch]

@padenot Is there something else to be discussed here? Changing <sequence<sequence<Float32Array>>> to <FrozenArray<FrozenArray<Float32Array>>> here should be enough.

[hoch]

We missed one thing. The last argument object also needs to be frozen right after the construction. If the length changes for any Float32Arrays inside, the object needs to be recreated.

[hoch]

Also we decided to change the length of arrays dynamically to signal the change of incoming connections or automation rate.

Now I am not sure if this is a good idea because any changes in the shape of underlying arrays will cause memory allocation. All data containers are a FrozenArray or a frozen object, so there's no way to modify its content after the integrity level change. So we have to throw it away and create a new one.

Checking the detached status of an array is less of an issue here, but the dynamic change of array shape leads to reallocation is a potential problem.

[rtoy]

I think a reasonable option here is to use the automation rate to gate this. If it's k -rate, then the arrays are always length 1. If it's a-rate, it's always full length even if we know a priori that the values are constant. I doubt people toggle back and forth between automation rates[1], so the array length is pretty much fixed.

[1] I'm speculating here; I have no evidence one way or another. And if they do, it would be relatively infrequent. If it causes too much garbage, it's pretty easy to make it fixed and still work well. (Always a-rate and do your own checking for constant values if it matters enough to check.)

[hoch]

I doubt people toggle back and forth between automation rates[1], so the array length is pretty much fixed.

Although that assumption is sensible, the impl still needs to prepare the corner case. Freezing an object makes this problem even worse.

[karlt]

I agree that using the automation rate to determine the length of the Float32Arrays in parameters would be reasonable. Such an implementation would be consistent with what is permitted by the current spec. Specifying that behavior would have the appeal of predictability for the client.

[karlt]

Something not yet discussed here is what should happen after output Float32Arrays are detached in process() calls.

I don't want to make available for reading the float32 values from an ArrayBuffer that is now mutable by another thread. That would be conceptually strange and racy in behavior.

I assume there's no good reason for a client to transfer ArrayBuffers from outputs, so we can reset [[callable process]] to false, queue a "processorerror", and make silence available for reading.

[hoch]

Eventually we have to specify an algorithm for process() and it needs to be a proper WebIDL.

Chrome's current impl does not touch ArrayBuffer if its shape is mutated (i.e. transferred) and will be recreated (re-allocation) in the next call. Crippling a processor into an unusable state is basically a breaking change, and that's what I wanted to avoid as I mentioned above.

[rtoy]

So, there's a couple of things that need to be done here, right?

1. Replace sequence<sequence<Float32Array>> with FrozenArray<FrozenArray<Float32Array>>
2. Make process() a callback.

[hoch]

It's been a while since this issue was assigned (https://github.com/WebAudio/web-audio-api/issues/1933#issuecomment-625422795). Perhaps we need to escalate this? @padenot I can assign this to myself again to get the ball rolling, if that's better.

[rtoy]

@padenot is out for a bit. Assigning to you. I think we've all agreed on the approach.

[karlt]

There is an option of providing the frozen behavior for the container arrays/objects through a proxy. That can prevent modification of the containers by content but allow the implementation to change properties on a object. This avoids the need to re-create an array/object and all its ancestors when the implementation needs to dynamically change a container.
Dynamic changes would then be considerably simplified, saving allocations, but I don't know whether or not a proxy would introduce additional overhead in access efficiency.