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

19. 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 #869 (comment)
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 (75ca59f#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 #869 (comment)

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.

[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 (#1933 (comment)). 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.