Open JohnWeisz opened 5 years ago
max(desiredInputChannelCount, desiredOutputChannelCount)
as this.channelCount
would suffice, having separately adjustable input/output channel count can facilitate reducing the number of Float32Array
objects allocated, thus reducing GC and improving performanceComparing the API proposed here with the mutation of outputs suggested in https://github.com/WebAudio/web-audio-api/issues/1515#issuecomment-406173462 for similar purposes:
Mutation of outputs
allows process()
to choose and output channel count based on inputs. With the API proposed here, such a change would not be possible (at least not without a delay of one rendering quantum).
From the abstraction point of view, using a property rather than internal mutation makes more sense to me. It is possible that the actual user (e.g. plug-in user) might not have access to the code of AudioWorkletProcessor (e.g. developed by third-party). Hiding the channel mutation logic inside of the processor will confuse users with the processor's mysterious behavior.
Both solution have merit: the algorithm can certainly do some optimizations based on input channel count and internal state, to output a specific number of channels.
However it can be useful to set it from outside, as a control parameter to the algorithm.
Both make sense, maybe both are needed in fact.
To track it properly, I am putting this to the V1 bucket.
If both ideas make make sense, then we have a conflict on our hands: what if both happen at the roughly same time? Each operation happens on a different thread. The property change happens on the main thread and will be posted to the rendering thread later, and the internal array mutation happens on the rendering thread. In short, this will be a flaky situation, which we don't want to have more of it.
After more thoughts, now I am actually in favor of the internal array mutation. We already have some native AudioNodes that have opaque channel behavior. It is not ideal, but justifiable.
An example:
const getNewOutputChannels = (channelCount) => {
const output = [];
for (let i = 0; i < channelCount; ++i)
output.push(new Float32Array(128));
return output;
};
...
/* AudioWorkletProcessor */
process(inputs, outputs) {
const input = inputs[0];
const output = getNewOutputChannels(4);
for (let channel = 0; channel < output.length; ++channel) {
// do something with |output|.
}
outputs[0] = output;
return true;
}
If this is what we want, the spec change can be minimal actually. We simply can say that the output array will be mutable, and the output channel count will be changed accordingly. FWIW, this will trigger the downstream channel count change as well.
@padenot @JohnWeisz WDYT about the idea above?
cc @rtoy
I agree with changing the output channel count by mutating the output array.
I believe it's a question of taste whether it should behave like input channel count (or not), which works by assigning to this.channelCount
in the AudioWorkletNode instance.
One factor I can think of (although this is a long shot) is that mutating the output array in the worklet processor might make particular future optimizations more difficult. A little more on this: IMO, implementations have the potential to optimize process
calls by detecting whether the outputs
(or inputs
) argument is used outside of the process
method in any way, and re-use the already allocated Float32Array[][]
instead of allocating new ones before each call when possible, massively reducing garbage collection in the process (remotely similarly to how the arguments
object is specially optimized, or used to be optimized in v8).
If we expect to mutate the output array, the above mentioned optimization might be difficult or downright impossible, although I don't quite see how.
Thanks for your feedback, @JohnWeisz!
The WG has decided to push this issue to v.next. Until then we'll be collecting more field data from the developers to figure out the right design for this feature. It could be either using a node side property, dynamic mutation of output array or something else.
Per https://github.com/WebAudio/web-audio-api/issues/1780#issuecomment-459434942 really make it v.next.
F2F summary:
Teleconf: Mutating the output array is no longer possible since the output is now FrozenArray<FrozenArray<Float32Array>>
. See https://github.com/WebAudio/web-audio-api/issues/1933
TPAC summary:
https://github.com/WebAudio/web-audio-api-v2/issues/42#issuecomment-610010388 shows that my first point above is wrong.
As mentioned in https://github.com/WebAudio/web-audio-api-v2/issues/37#issuecomment-433383527, one use case is a convolver, implemented in a worklet. With a mono input and mono response, the output is mono. But if the response is changed to stereo, the convolver would produce stereo output even for the mono input. This can't currently be done in a worklet. In this case, an API for the control (main) thread would work. When the response is updated from the main thread, the number of output channels could also be specified.
The case for the delay node is more complicated. As @padenot mentioned in the TPAC meeting today, consider a delay node with a delay of 4 s and a mono source. The output is mono, of course. But then connect a stereo source to the delay node. The output should still be mono for 4 s and then switch to stereo. This would be very hard to do from the control thread.
With FrozenArray
, there are no ways out of this with the current API.
With WebAudio/web-audio-api-v2#4, then if we have a way to change (mostly, extend, to go from say stereo to quad, the opposite being straightforward) the memory will be used by the engine as the output of the node, we can sort this out.
Pseudocode:
function process(input,output,param) {
if (input[0].length == 2) {
// now need to output 4 channels instead of whatever we have -- placeholder function name
setOutputBuffers(4, arraybufferview);
}
// ... regular processing
}
where arraybufferview
is a buffer that exists with 4 4 render quantum size
bytes at least.
F2F Meeting: Allowing the channel count change to happen would allow an AudioWorkletNode to faithfully polyfill nodes like a DelayNode (because channel count changes should occur after the delay of the node). This isn't possible now. Add support is fairly complex. But the change would not happen immediately; it would only happen sometime later, perhaps the next call, but could be later. Would it be ok if the channel count change happened via messages from the main thread?
In any case, leaving this open, but changing priority, per meeting discussion.
Would it be ok if the channel count change happened via messages from the main thread?
Yeah it would probably work fine. This is already possible to do BTW using a worklet processor that acts as a channel detector, reinstantiating and reconnecting the audio worklet that does the actual processing... or something similar along these lines.
This bug seem to be resolved, except for some edge cases. Look here: https://bugs.chromium.org/p/chromium/issues/detail?id=1428291#makechanges
The workaround suggested by https://github.com/WebAudio/web-audio-api/issues/2438#issuecomment-837375979 and https://github.com/WebAudio/web-audio-api/issues/2438#issuecomment-1120217706 does seem reasonable.
Describe the feature
Allow changing outputChannelCount dynamically, after and separately from instantiation.
The following example briefly describes a node which always takes a single stereo input, has a single stereo output (by default), and can switch between stereo output, quad output, and auto output on demand, dynamically, at any time:
Is there a prototype?
Can be done in a limited fashion using a wrapper object (and augmenting the
AudioNode.connect
method to accept the wrapper object), as currently a reinstatiation of the AudioWorkletNode is required to change output channel count.For 1-1 input/output count, I strongly believe this idea can also be done by setting
channelCount
to the max of input/output channel count, and adding JS-implemented channel upmixing/downmixing beforeprocess
calls.Describe the feature in more detail
Note: below each reference to
this
refers to an AudioWorkletNode instance.General behavior
Assigning to
this.outputChannelCount
will result in the exact same behavior as if the AudioWorkletNode was initialized with the specified output channel configuration in the first place, beginning with the "next"process
call on the corresponding AudioWorkletProcessor instance.Here, the "next" process call can be interpreted essentially the same as the next process call after setting
this.channelCount
.Immutability
Assigning to
this.outputChannelCount
is consistent with assigning tothis.channelCount
, although it is up to question how exactly assigning to a single specific output is handled, i.e. what the following does:My vote here is to freeze
this.outputChannelCount
, and always require a full array assignment.Clearing an explicitly defined value
In a case where the developer wants to clear any values set previously, and instead use an automatically determined outputChannelCount,
this.outputChannelCount
can be assignednull
. Personally, I feel this part of the proposal needs some more discussion.This also implies the default value of
this.outputChannelCount
isnull
if not set to a specific configuration in the constructor.