Handling of output parameter for worklet process()? about web-audio-api-v2 HOT 28 CLOSED

An additional note from today's teleconf: We'd like to gather up all enhancements suggested for an AudioWorklet and consider them all in a unified way in the next version. We want to have a relatively stable definition of AudioWorklet now since we're nearing recommendation status.

from web-audio-api-v2.

I'm kind of thinking this should cause an error to be thrown. I guess that also implies that if output isn't exactly sequence<sequence> an error is thrown. Similarly, if the length of the Float32Array isn't 128, it's an error.

I agree.

from web-audio-api-v2.

Agreed - this is a spec oversight.

from web-audio-api-v2.

To clarify a bit:

1. Everything has to match for sequence<sequence<Float32Array>>.
2. Otherwise it throws an exception and deactivates the processor resulting the silence in the subsequent render quanta.
3. The corresponding AudioWorkletNode's onprocessorerror event handler will be fired.

from web-audio-api-v2.

Yes, and the sequence lengths and Float32Array lengths must match.

from web-audio-api-v2.

See: w3c/css-houdini-drafts#433

The problem is the error handling of WorkletGlobalScope is not well-defined yet.

from web-audio-api-v2.

Actually, as pointed out to me by @sletz, I think we can solve this
immediate problem using Object.freeze() (See
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze)

Before calling process(), we can do something like:

inputs.forEach(in => Object.freeze(in))
Object.freeze(inputs)

This makes it so that the length of inputs can't be changed; the
elements of inputs can't be changed, and inputs[k] has fixed
length and the elements of inputs[k] are fixed to be Float32Arrays
of length 128. You can, however, modify the elements of the
Float32Arrays.

And do the same thing with outputs.

If this is correct, then we don't have a problem with the user
trashing these parameters by accident---it just can't happen.

from web-audio-api-v2.

I like this idea a lot. The Object.freeze is well-defined, so we can use it in our spec or implementation. Validating and throwing exception would be also handled by the underlying JS engine, so that's a plus too.

from web-audio-api-v2.

@chrisguttandin mentioned on the webaudio slack channel that Object.freeze isn't used commonly and the "general assumption in the JS community is that it is very slow and therefore should not be used in production code whenever possible"

I don't quite understand why it should be slow for our use case, unless it makes accessing the underlying Float32Arrays slow.

from web-audio-api-v2.

We can do the experiments and see how it actually performs. Just saw the guts of Object.freeze and the code is pretty lengthy.

from web-audio-api-v2.

While we're waiting for the experiments, and assuming they work, we could say something like

1. Apply operation Object.freeze to outputs
2. For each n, apply freeze to outputs[n]

Simple experiments shows that this works. outputs is always sequence<sequence<Float32Array>>. (Well, for my test Array<Array<Float32Array>>.)

from web-audio-api-v2.

At some point, it would be nice to let the AudioWorkletProcessor determine the
number of output channels. This would be useful for nodes that have a tail
time but wish to adjust the output channel count according to the
computedNumberOfChannels from the input. Consider a DelayNode implementation,
for example.

Could we permit process() to modify the length of each outputs[n]?

I guess freeze could still be applied to outputs, but it would kind of lose its appeal somewhat.

from web-audio-api-v2.

Teleconf: Just say changing it is undefined and move @karlt comment https://github.com/WebAudio/web-audio-api/issues/1515#issuecomment-406173462 to a new issue, for v.next.

from web-audio-api-v2.

To clarify a bit from the teleconf, for V1, we will leave the behavior undefined if you modify the output parameter. In v.next, we can take up the issue about allowing the user to change the length of the array. Leaving it undefined now allows us to define the behavior in a compatible way for v.next.

from web-audio-api-v2.

Summary

The previous conclusion was to leave it undefined so we can define in v.next.

I would like to clarify what we want from this. First, we have two options:

1. Leave it v.next and do nothing.
2. Find an agreeable solution.

Obviously we don't want the option 1, so that will bring us to the following options:

1. Use Object.freeze() on outputs and its contents recursively.
2. Allow AudioWorkletProcessor mutate outputs dynamically.

Thoughts

AWP being able to mutate the content of a given outputs object feels natural, but I am not sure it is compatible with the behavior of other AudioNodes. When you need to change the channel count of an AudioNode, changing channelCount property should be your way.

My preference is to use the channelCount property for the change, and to freeze outputs object to block the internal mutation. That way it is consistent with the rest of API (even though they are not visible) and easier to reason about. Another angle to look at: we can block the internal mutation, but we can't get rid of channel count property from the node.

Note: There is some overlap with WebAudio/web-audio-api-v2#37. Perhaps we should merge them into one.

from web-audio-api-v2.

AudioNode.channelCount specifies the number of input channels, and so would not be appropriate for specifying the number of output channels.

from web-audio-api-v2.

I have some doubts on what channelCount means for AudioNode. For the sake of simplicity, the spec says it is about the input, but you can see it in many different ways. From my perspective, the channel count means a number of channels being processed by a "kernel" inside of an AudioNode container.

For example, when you set a channelCount of 5 for a GainNode, the node configures the kernel so it can process 5 channels. The input and the output of the node will be configured accordingly afterwards. So you'll end up with a 5-channel output. Then does this channelCount really mean "input channels"?

Also this is why I don't like the idea of having a separate outputChannelCount property on AudioWorkletNode. It feels like adding more confusion to a thing that's already ambiguous.

from web-audio-api-v2.

from web-audio-api-v2.

I think the mutable output array will be a significant architectural change for any browser.

I believe we (as in all browsers) configure AudioNode's input and output first and then process the audio. For Audio Worklet, this will change such model so we can 1) configure input, 2) process audio and then 3) configure output. I'll have to go back to take a look when the downstream propagation happens, but as @rtoy suggested the change will not be simple and might have other implication we did not expect. If that's the case, I am not sure if we can address this in the scope of V1 work.

@karlt @padenot WDYT?

from web-audio-api-v2.

I think this is easy for us, @karlt will know for sure.

from web-audio-api-v2.

Yes, Gecko already has a model where output is configured after audio input is received for processing, and so no major changes would be required. IOW Gecko can easily set the output channel count after process() returns.

from web-audio-api-v2.

Unfortunately Blink does the channel count configuration at the beginning/end of the render quantum. So I think we'll have a different behavior on this one. To accommodate both cases, we can say something like "the channel count will change immediately or at the next render quantum."

from web-audio-api-v2.

This is technically a sub issue of WebAudio/web-audio-api-v2#37. WG decided to punt this to v.next.

from web-audio-api-v2.

A, perhaps iterable, interface type with an indexed property getter is an option for at least the inner objects, of length number of channels, to control which modifications are permitted. This could permit modifications to the length, but exclude modifications to the type of the properties, if that is desired, for example.

FrozenArray would be an option for the outer object outputs, of length number of outputs, assuming we don't want the inner objects replaced with other objects.

There seem to be varying opinions on whether frozen objects are desirable or not, but they are in WebIDL in support of APIs that provide the same object each time. See for example https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682#c34 or discussion in https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004

from web-audio-api-v2.

WebAudio/web-audio-api#25 (comment) describes the DelayNode use case where the output channel count depends on the inputs and parameters passed to process().

from web-audio-api-v2.

F2F summary:

• This is important
• Not talked about at length during the f2f because key people were missing from the discussion
• The arrays are FrozenArray now
• This also ties into https://github.com/WebAudio/web-audio-api-v2/issues/4

from web-audio-api-v2.

I think we should close this issue since the original question in #42 (comment) can't happen anymore since the arrays are now Frozen. Anything else that needs to be done can be handled in #37, which is where lots of the comments here were headed anyway.

from web-audio-api-v2.

TPAC: Yes, modifications as mentioned in #42 (comment) can't happen anymore. Closing this. Changes will be handled in #37.

from web-audio-api-v2.