WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.04k stars 166 forks source link

[audioworklet] Passing AudioWorkletNodeOptions to AudioWorkletProcessor constructor #1459

Closed hoch closed 6 years ago

hoch commented 6 years ago

I think it makes sense to contain the processor-related data in a dedicated field of processorOptions.

dictionary AudioWorkletNodeOptions : AudioNodeOptions {
  unsigned long numberOfInputs = 1;
  unsigned long numberOfOutputs = 1;
  sequence<unsigned long> outputChannelCount;
  record<DOMString, double> parameterData;
  any processorOptions = null;
};

The entire dictionary is serialized/passed into AudioWorkletProcessor constructor and any processorOptions can be used for the user-defined initialization in the processor. Also I believe this is the proper way of allowing arbitrary data within dictionary. (Similar example: https://notifications.spec.whatwg.org/#dictdef-notificationoptions)

sletz commented 6 years ago

One typical use-case is moving an entire wasm instance from Node to Processor context, which would simplify a lot the complex model we are currently using in the Faust dynamic wasm code generation done here: https://faust.grame.fr/dynamic/webaudio-wasm-wrapper.js, and used in the this page for instance: https://faust.grame.fr/dynamic/faustlive-wasm.html

joeberkovitz commented 6 years ago

This would destroy the simplicity of API users passing options straight into a new AudioWorkletNode just like they would for a native node.

Also, I don't see that this change either helps or hurts wasm, since under the current arrangement we are already supposed to be serializing all data in an AudioWorkletNodeOptions. (I'm familiar with what Faust is doing already and the WG's current spec language was supposed to address that situation.)

sletz commented 6 years ago

So what do you suggest? That the complete "AudioNodeOptions" option is moved on the processor size, even non needed fields?

joeberkovitz commented 6 years ago

@sletz yes, that's what I'm suggesting. At least right now, I don't see that the cost of potentially transferring more data than needed offsets the additional complexity in using the API.

Also, this change would require the consumer of a custom AudioWorkletNode to be aware of which options are going to be serialized to the audio thread, and which options are going to stay on the main thread. That seems like an inappropriate burden given that the consumer may be using a custom node library and may not be aware of the internal implementation.

sletz commented 6 years ago

Fine for me. One remaining question : is serializing wasm instances or modules already possible ?

hoch commented 6 years ago

@domenic What happens there are arbitrary fields in the dictionary and it gets serialized for the cross-thread message passing? I found that V8 actually strips out all the arbitrary fields after the serialization/deserialization process.

hoch commented 6 years ago

which options are going to stay on the main thread.

@joeberkovitz This sort of defeats the purpose of passing |options|. Then why are we passing this |options| object when it is only used for the main thread? From my perspective, the fields in AudioWorkletNodeOptions dictionary are only relevant to the node.

I believed we wanted to be able to pass some arbitrary information to initialize the processor instance in its constructor. Am I missing something?

hoch commented 6 years ago

is serializing wasm instances or modules already possible ?

@sletz @domenic can answer this question better than I do, but I believe it is possible. It's either implemented or planned at least in Chrome.

joeberkovitz commented 6 years ago

@hoch I am confused. Options is not only used on the main thread. We just went to the trouble of amending the spec precisely so that all options would become visible to the audio thread.

My point was that for custom nodes, we can get into situations where some options are intended for the main thread to consume (say in a custom AWN subclass) while others are intended for the audio thread, and neither the consumer of a custom node nor the AWN subclass constructor should have to care about this.

hoch commented 6 years ago

where some options are intended for the main thread to consume while others are intended for the audio thread

Thanks for the clarification, but I am not really sure that's how IDLDictionary type works. I assume that you're referring "arbitrary user-supplied fields" in the dictionary. I need to confirm the correct behavior of serialization/deserialization first - it might be supposed to strip out random fields that are not defined in AudioWorkletNodeOptions IDL. (See this comment)

AudioWorkletNode is sending AudioWorkletNodeOptions and AudioWorkletProcessor's subclass constructor also accepts AudioWorkletNodeOptions. Perhaps somewhere in there IDL type checking might kick in. To workaround this problem, I was proposing to add any processorData field.

domenic commented 6 years ago

What happens there are arbitrary fields in the dictionary and it gets serialized for the cross-thread message passing?

Yes, dictionaries are not appropriate for this. Their fields are all known ahead of time. If you want arbitrary data, you'll need to use object or any in place of a dictionary. (Or, as you mention, get both, by nesting an any member inside the dictionary.)

is serializing wasm instances or modules already possible ?

Yes; see https://github.com/WebAssembly/design/pull/1074

hoch commented 6 years ago

@joeberkovitz As @domenic confirmed above, throwing random fields to an options dictionary for the cross-thread message passing will not work. So my proposal on adding any processorData can address the issue that we're having.

Thanks so much for the quick response, @domenic! :)

joeberkovitz commented 6 years ago

@hoch In this case I think we still need to at least discuss the possibility that a typed dictionary is not the right vehicle for the options, and that we should use object instead. Requiring coders to put things in processorData by hand is a pretty big step backwards in ease of use.

hoch commented 6 years ago

@joeberkovitz Having 'any' in the field is a decent option, I think. If AudioWorkletNode ends up not accepting AudioNodeOptions, there would be a lot of things to describe now. For example, how random fields in an object get parsed and mapped into "pseudo-pre-defined" properties like numberOfInputs, numberOfOutputs and etc.

I would like to avoid devising our own procedure/algorithm for the random property parsing.

Changing the POV, AudioWorkletNodeOptions already requires coders to put things like parameterData and outputChannelCount - which are AudioWorklet-speicific. I believe adding one more field doesn't hurt and the advantage is significant. I value @sletz's use case and AudioWorklet should support that.

rtoy commented 6 years ago

I think it's appropriate for AudioWorkletNodeOptions to allow passing other things to the processor. I would choose a name different from processorData, though. (Maybe processorOptions?)

Using any seems appropriate as well, so that we leave it up to the implementer to chose the appropriate data structure for telling the processor what to do.

sletz commented 6 years ago

Being able to pass data from Node to Processor constructor seems the only sane way to do. The processor will typically need to access Node/Processor specific parameters at creation time, that they both agree to deal with. So this is quite easy for the Node/Processor coder to handle that. processorOptions seems a good name.

Typical use cases we already have:

joeberkovitz commented 6 years ago

@hoch I'm hearing you that this is awkward either way, so maybe we should act now to fix the problem and create a new field that is guaranteed to be serialized. I prefer the name processorOptions because we have already established the idea that options is a constructor argument that passes a bunch of arbitrary data. And in that case, typing it as object feels a bit better than any: we're saying, these options are an open-ended map, but not a scalar.

What do people think?

@sletz Just to reassure, we're definitely providing a way to pass arbitrary data. The only debate is around the mechanism for doing it -- this use case will definitely be supported.

hoch commented 6 years ago

@joeberkovitz Glad we're getting close to the agreement. I like processorOptions too, but why object is better than any? any can be anything including object. (reference)

Also @rtoy's idea on passing the entire dictionary is sensible, so would love to know what other folks think.

rtoy commented 6 years ago

Passing the entire dictionary just makes it simple so we don't have to explain any magic that happens, or if we update later and add another dictionary member.

joeberkovitz commented 6 years ago

The reason to use object instead of any is that we constrain the options to take a similar form across all custom nodes, which seems valuable for the dev community.

rtoy commented 6 years ago

Use object.

And pass the entire dictionary in (not just the object slot)

hoch commented 6 years ago

Because we call all the constructor optional arguments as "options", I think processorOptions is better for the name of the field.

@joeberkovitz @padenot WDYT?

rtoy commented 6 years ago

processorOptions works for me, for the reasons given.

hoch commented 6 years ago

@mdjp @padenot @joeberkovitz

Can I get one more LGTM on processorOptions?

padenot commented 6 years ago

Yes, processorOptions looks good. I also agree that having Object rather than any is better, for the reason @joeberkovitz gave on dec. 14.