TAG issue: Subclassing / Node composition
chrislo opened this issue · 35 comments
The following issue was raised by the W3C TAG as part of their review of the Web Audio API
ISSUE: Subclassing
Related to a lack of constructors, but worth calling out independently, it's not currently possible to meaningfully compose node types, either through mixins or through subclassing. In JS, this sort of "is a" relationship is usually set up through the subclassing pattern:
var SubNodeType = function() {
SuperNodeType.call(this);
};
SubNodeType.prototype = Object.create(SuperNodeType.prototype);
SubNodeType.prototype.constructor = SubNodeType;
// ...There doesn't seem to be any way in the current design to enable this sort of composition. This is deeply unfortunate.
with sublassing capabilities writing custom nodes will be way easier and there won't be a fight of CustomNode(input) vs input.connect(customNode.node) etc.
For example, I have a multi channel analyzer node. If I were to expose a currently ideomatic webaudio api, it would be
sourceOne.connect(analyzer.node);
sourceTwo.connect(analyzer.node);
analyzer.on('data', function(data) {
console.log(data); // [[ 0, 1, ... ], [ 1, 1, ... ]]
});Having unnamed parameters sucks, and especially when you disconnect one node it becomes super confusing. So the api I have now is like this:
analyzer
.add('one', sourceOne)
.add('two', sourceTwo)
.on('data', function(data) {
console.log(data); // { one: [ 0, 1, ... ], two: [ 1, 1, ... ] }
});This works but isn't ideomatic webaudio style, i.e. it's not using .connect. What I'd ideally do is this:
sourceOne.connect(analyzer.input('one'));
sourceTwo.connect(analyzer.input('two'));
analyzer.on('data', function(data) {
console.log(data); // { one: [ 0, 1, ... ], two: [ 1, 1, ... ] }
});So I'd have be able to create custom audionodes.
This is likely a meta-issue: make sure we can compose nodes somehow.
Would it be useful to a factory method for creating custom nodes? Maybe something like this:
AudioContext.prototype.createCustomNode = function(CustomNode, ...options) {
var node = Object.create(CustomNode.prototype);
Object.defineProperty(node, "context", {
enumerable: true,
configurable: false,
writable: false,
value: this
});
CustomNode.apply(node, options);
return node;
}
It would be nice if we could do write source.connect(customNode, [output], [input]). This would necessitate customNode to define some sort of inputs array that contains one or more AudioNode instances which source would look at when making the connection. The could be polyfilled by doing something like this:
var connect = AudioNode.prototype.connect;
AudioNode.prototype.connect = function(destination, output = 0, input = 0) {
if (destination instanceof CustomNode) {
this.connect(destination.inputs[input], output);
} else if (destination instanceof AudioNode) {
this.connect(destination, output, input);
} else if (destination instanceof AudioParam) {
this.connect(destination, output);
} else {
throw "not a valid destination";
}
};
In the case of a custom node possessing multiple input nodes which have multiple inputs themselves a user would have to specify which of the inputs in the following way: node.connect(customNode.inputs[0], [output], [subInput])
Even having a custom "compositorNode" that could have a graph internally would be a fix for this. It really, really sucks to not be able to properly do this, since it means you can't create (e.g.) a "ChorusEffectNode" that can be both connected to and connected.
I would really love to see this happen. I've already gone extra miles to have something like this in my WAAX library. Having this feature on API level would be much more useful then JS polyfills and prototype override/inheritance. If this happens, I can see my plug-ins can be ported with very little effort.
Also another useful thing to have is a collective AudioParam. Some parameters can be abstracted on top of a set of AudioParams. Redirection/collective control on them makes much more sense inside of CompositorNode.
@chris Do you have any idea/proposal for this?
I'm not sure this should be v.next.
TPAC: @cwilso to verify that de facto subclassing/extension of native nodes can be done, then close.
I believe that there's no reason this would not be addressed by the existence of regular constructors as per #250, and the ability to delegate from a subclass to the prototype via Object.create(<superclass>.prototype) and <superclass>.call(this) in the subclass constructor. Does anyone disagree with this? If not I will close as a duplicate of #250.
Please don't, as they're not the same thing. Paul's assertion at TPAC was that this can work today; the TPAC assertion (above) was that I would test this, and close if I can write an example of how to create a composite node that works. That won't happen until after Chrome Dev Summit (and Thanksgiving), but this should stay on me.
Great, then I will assign to you and proceed with #250 separately.
I've been fiddling around and objects created using an existing AudioNode as a prototype do not seem to work today:
> ctx = new AudioContext()
AudioContext {}
> baseosc = ctx.createOscillator()
OscillatorNode {}
> baseosc.channelCount
2
> osc = Object.create(baseosc)
Object {}
> osc.channelCount
Uncaught TypeError: Illegal invocation(…)(anonymous function)
@cwilso This was marked as in the current sprint for me so it would be great if we could keep it that way. Perhaps someone else on the Chrome team could help if you're not available?
The new constructor makes this possible, as discussed in Atlanta.
Although it still lacks the support of AudioParam composition, here is the proof-of-concept of ES6 CompositeAudioNode polyfill:
https://github.com/GoogleChrome/web-audio-samples/wiki/CompositeAudioNode
@svgeesus what does the new constructor look like?
@kevinbarabash Currently what we have is a factory-style constructor.
var gainNode = context.createGain();On the other hand, the new constructor uses new keyword.
var gainNode = new GainNode(context);We might have to reconsider our decision: the new constructor pattern cannot achieve functional subclassing.
// This is cool.
var gainNode = new GainNode(context);
// But what does this mean?
MyGainNode extends GainNode {
/** stuff **/
}What kind of thing can you do in there except for overriding connect or disconnect methods? You cannot even create an instance AudioParam. The main problem is the new constructor does not expose process() method like AudioWorkletNode class, so you cannot alter how the node manipulates the audio stream.
For the v1, we might have to leave out subclassing for AudioNode. AudioWorkletNode is not a perfect alternative, but it does work. For the node composition, please take a look at my polyfill above.
On Wed, May 4, 2016 at 10:56 AM, Hongchan Choi notifications@github.com
wrote:
We might have to reconsider our decision: the new constructor pattern
cannot achieve functional subclassing.// This is cool.var gainNode = new GainNode(context);
// But what does this mean?
MyGainNode extends GainNode {
/** stuff **/
}What kind of thing can you do in there except for overriding connect or
disconnect methods? You cannot even create an instance AudioParam. The
main problem is the new constructor does not expose process() method like
AudioWorkletNode class, so you cannot alter how the node manipulates the
audio stream.For the v1, we might have to leave out subclassing for AudioNode.
AudioWorkletNode is not a perfect alternative, but it does work. For the
node composition, please take a look at my polyfill above.
FWIW, I'm perfectly happy with leaving all of this subclassing to
AudioWorklets. The existing nodes are what they are.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#251 (comment)
Ray
But aren't these benefits considerable even without modifying the node's underlying behavior on the audio-thread side? It's very useful to be able to adorn the subclass with convenience methods that call superclass node methods. Despite the limitations this seems like a nontrivial benefit for builtin nodes, and maybe a really big benefit for the main-thread side of AudioWorkletNode.
For a builtin example, one could imagine subclassing AudioBufferSourceNode in order to set up the buffer, loop points and start/stop time in a complex way appropriate to some synthesizer application.
For subclasses of AudioWorkletNode the goodies are even bigger. One would be able to create methods that wrap all those messy calls to postMessage(), and the subclass ctor could set up a custom onmessage handler for communication coming back from the audio side. I don't know, that seems like a really great benefit to me. It would allow subclasses of worklet nodes to expose a surface that starts looking an awful lot like native nodes.
BTW, if my observations about the value of subclassing AudioWorkletNode are correct, perhaps this argues against adding a new factory-style pseudo constructor for this node on BaseAudioContext as described here: https://github.com/WebAudio/web-audio-api/wiki/AudioWorklet-IDL-Proposal#idl-audioworkletnode
After giving more thoughts on this, I feel like we are arguing over nothing.
If we register all the constructors of built-in nodes under window name space, extending them becomes possible anyway. I was just simply pointing out the missing part - we won't be able to change the audio processing callback. This only can be done by AudioWorkletNode. Since we already decide this supports the constructor, so extending it shouldn't be an issue.
If we make an exception like "you have the constructor, but you cannot extend it" in the spec, that looks really weird as well.
To sum up,
- All the built-in nodes will have a constructor, which can be extended.
- Extend
AudioWorkletNode/AudioWorkletProcessorif the sample-level audio manipulation is required.
@domenic I think the working group has reached the agreement. Could you take a look at the comments above and let us know TAG's decision?
I'm having a hard time understanding what the concrete proposal is for changing the spec based on the above comments. Would you be able to summarize?
@domenic Sorry about the confusion. Here is the summary:
- We are introducing a new constructor pattern for all types of audio nodes, that can be subclassed into a custom node by developers. An example would be:
class CustomGainNode extends GainNode {
// Override an existing method.
connect () {
console.log(' connected!');
super.connect.apply(this, arguments);
}
// Adding a new method.
mute () {
this.gain.setValueAtTime(0, 0);
}
}- In terms of the composition of a custom node (by assembling multiple built-in audio nodes), I have a separate proposal. It is unfortunate that we still need to use the shim, but I could not find a better way to do this.
AudioWorkletNodeclass can be extended if you need to access the actual audio samples. Proposal 1 and 2 both does not provide this feature because the actual audio process method cannot be exposed to JS. The IDL proposal ofAudioWorkletNodeis here.
Hope this helps and let me know if you need more clarification.
That plans sounds pretty good!
A way to improve https://github.com/GoogleChrome/web-audio-samples/wiki/CompositeAudioNode to avoid having to override connect would be to add an overload to connect() that accepted something like a dictionary whose single member is input. Then, any subclass which exposed a public property named input (e.g. if you changed your example to use input instead of _input) would automatically work as an argument to connect().
(Alternately you could pick a name that is more specific to this adapter purpose like innerNode or connectableAudioNode or something, instead of input.)
@joeberkovitz @mdjp What would be the next step here?
Seems to me we can just proceed on cleaning up the constructor definitions
as per the other issue filed on this point, and clarify that they are in
the global window namespace.
I don't think we need to do anything special to address the
composite-node-construction case for v1 although it's nice to get a better
pictre of how this might be done.
...Joe
. . . . . ...Joe
Joe Berkovitz
President
Noteflight LLC
+1 978 314 6271
49R Day Street
Somerville MA 02144
USA
"Bring music to life"
www.noteflight.com
On Fri, May 20, 2016 at 10:49 AM, Hongchan Choi notifications@github.com
wrote:
@joeberkovitz https://github.com/joeberkovitz @mdjp
https://github.com/mdjp What would be the next step here?—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#251 (comment)
Then perhaps we can move this issue to the ready-for-editing status.
done!
Now we have constructors, this isn't an issue any more.
Is there an info was the extending implemented by any browser? Chrome 91 doesn't seem to allow that.
Chrome 91 doesn't seem to allow that.
Can you elaborate what you tried and what did not work?
@hoch I mean your example:
class CustomGainNode extends GainNode {
mute () {
this.gain.setValueAtTime(0, 0);
}
}
let ctx = new AudioContext
let g = new CustomGainNode(ctx)
g.mute()
// VM207:1 Uncaught TypeError: g.mute is not a function
// at <anonymous>:1:3Can that be considered a chromium bug? Is that expected to be implemented?
Firefox, Safari support that.
Filed a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1229154
I responded on the bug, but it seems working correctly for me. We can continue the discussion on the bug, not here.