ES6 classes that inherits from AudioNode stopped working when the extension is enabled about audion HOT 26 CLOSED

@tchakabam, good question. I think #73 (comment) should shed some light: Overriding constructors lets us find orphan nodes.

Writing a tracer for the toneJS library sounds useful. :)

from audion.

Hi! Any news about this bug? Its quite enoying, as we're subclassing AudioNode instances a lot in our code (mainly AudioWorklet but not only this one) and this breaks the extension each time, making our webapps very difficult to debug. If I can help... ?

from audion.

I think the tracing logic is basically failing on the derived constructor based on "class extends" syntax. Not sure how to address this until we have a native communication via DevTool API.

Perhaps there is a set of workarounds to snatch the derived construction like this, it won't be pretty.

from audion.

This is what happens in M57:

const _gainNodeConstructor = GainNode.prototype.constructor;

GainNode.prototype.constructor = function () {
  console.log('GainNode constructor called.');
  _gainNodeConstructor(...args);
};

class VolumeNode extends GainNode {
  constructor(context) {
    console.log('VolumeNode constructor called.');
    super(context);
  }
}

let context = new AudioContext();
let vnode = new VolumeNode(context);

console.log(vnode.constructor.name);
console.log(Object.prototype.toString.call(vnode));

And the console says:

"VolumeNode constructor called."
"VolumeNode"
"[object GainNode]"

So the parent constructor is being invoked implicitly, basically bypassing the traced constructor. ("GainNode constructor called." is missing.) This might be the IDL/implementation bug in our constructor, or perhaps this is how it is specced. It needs further investigation.

from audion.

Ok, sorry to give you more work guys ;-)

from audion.

Per Hongchan, the current tracing logic seems to break with ES6 subclassing. The problem seems to be how the constructor overrider returns an AudioNode instead of setting properties on this:
https://github.com/google/audion/blob/795545d1692d11a0348556827622c997dbc9502e/js/entry-points/tracing.js#L926

At the outset, I see no fixes within ES5. One solution I think would be to make closure compilation use ES6 for tracing.js. And then, make the tracing logic extend native AudioNode types:

class OverridenAnalyserNode extends AnalyserNode {
  constructor() {
    super(...args);
    // Add tracking logic here.
  }
}
AnalyserNode = OverridenAnalyserNode;

More investigation is indeed needed. Thank you for noticing this! More folks will start using ES6 features, so this issue is important.

from audion.

@micbuffa Thanks for catching this!
@chihuahua I think we should say something about this in README.

from audion.

I did some digging on how to trace ES6 classes inheritance and so far I only have more bad news.

1. The pattern in #73 (comment) actually changes the prototype chain of the node, so there might be side effect. (e.g. hasOwnProperty() will return a different value.)
2. class ... extends ... cannot be done programmatically, meaning that we have to write the code by hand for all the constructors. Not fun.

from audion.

Both 1 and 2 indeed seem difficult. :(

I took your snippet above and played with it a lot. It seems like we can still supporting extending with ES5 if we use the "new" keyword to create the node. For instance, this works I think:

var originalAnalyserNodeConstructor = AnalyserNode;
AnalyserNode = function(audioContext) {
  var node = new originalAnalyserNodeConstructor(audioContext);
  console.log('We tracked AnalyserNode.');
  return node;
};
AnalyserNode.prototype = originalAnalyserNodeConstructor.prototype;
AnalyserNode.prototype.constructor = AnalyserNode;

class FrequencyAnalyserNode extends AnalyserNode {
  constructor(audioContext) {
    super(...arguments);
  }
}

let node = new FrequencyAnalyserNode(new AudioContext());
console.log(node);

After that code runs, getByteFrequencyData is still defined on AnalyserNode.prototype.

However, this still succumbs to how we must deal with each node type 1 by 1, which is manual, albeit I don't think I can come up with a more concise solution. Maybe it's worth it?

from audion.

In short, the problem is that this schmorgasborg of a hack isn't working.
https://github.com/google/audion/blob/795545d1692d11a0348556827622c997dbc9502e/js/entry-points/tracing.js#L906

We got to use the new keyword.

from audion.

Never mind - the above won't work because calling super doesn't define the other method of the child class ... it sounds like we may indeed an ES6 solution.

from audion.

Oh wait wait wait, I think using Function.prototype.bind.apply without the new operator works:

var originalAnalyserNodeConstructor = AnalyserNode;
AnalyserNode = function(audioContext) {
  Function.prototype.bind.apply(
      originalAnalyserNodeConstructor, [this].concat(audioContext));
  console.log('We tracked AnalyserNode.');
};
AnalyserNode.prototype = originalAnalyserNodeConstructor.prototype;
AnalyserNode.prototype.constructor = AnalyserNode;

class FrequencyAnalyserNode extends AnalyserNode {
  constructor(audioContext) {
    super(...arguments);
  }
}

let node = new FrequencyAnalyserNode(new AudioContext());
console.log(node); 

from audion.

Oh no, it doesn't work. None of the properties are actually defined on the node:

AnalyserNode {}
channelCount
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]
channelCountMode
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]
channelInterpretation
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]
context
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]
fftSize
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]
frequencyBinCount
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]
maxDecibels
:
[Exception: TypeError: Illegal invocation at AnalyserNode.remoteFunction (<anonymous>:2:14)]

from audion.

Another idea: We do not bother to trace AudioNode constructors. Instead, on connect or disconnect, we track any nodes we haven't seen before.

The downside to this idea is that we don't track nodes that connect to nothing and have nothing connecting to them. IMO, this is tolerable because isolated don't play a role in audio graphs anyway.

It might be worth doing that to support ES6, which come to think seems important.

from audion.

FWIW, Firefox's (excellent) web audio editor seems to err for
https://codepen.io/w3devcampus/pen/GWGdjK?editors=0010 as well.

The editor perpetually hangs on "Waiting for an audio context to be created ...", albeit the visualization runs correctly:

Per above, it might be OK to make a compromise and avoid tracking constructors (to avoid major ES6 side effects).

from audion.

Another idea: We do not bother to trace AudioNode constructors. Instead, on connect or disconnect, we track any nodes we haven't seen before. The downside to this idea is that we don't track nodes that connect to nothing and have nothing connecting to them. IMO, this is tolerable because isolated don't play a role in audio graphs anyway.

I disagree. It helps you find orphan nodes in your code. If you see something unconnected, that means the code needs to be fixed. I will bring these questions to ES6 experts.

from audion.

I can see the benefit of seeing orphan nodes. Overall, I'm not sure if we have a perfect solution. Any input from ES6 gurus would be great!

from audion.

Definitely want to see orphan nodes in the graph. Having orphans isn't an error (unless they stay that way forever).

from audion.

Got it. In that case, it seems like we ought to track AudioNode constructors. The question then becomes how we can make that jive with ES6.

from audion.

Any news?

from audion.

Unfortunately, not yet. I'm not sure how to quietly override ES6 class constructors. Maybe there is an imperfect way (with subtle side-effects), but we are still looking.

from audion.

Ok, thanks...

from audion.

@chihuahua Hey, this indeed seems to be a really tricky one.

Was just wondering, why do you need to override the constructor in the first place?

To be able to trace across connections, shouldn't it be sufficient to override the connect/disconnect methods with spy-ones that will track each patch and then just call the original implementation - and then keep all that state in a singleton outside of all the nodes? You shouldn't really have to deal with any instance of any AudioNode, except the original ones? :)

Probably I should first read up on your code here, but maybe you can give me a quicker insight? That'd be nice!

I am thinking of writing a webaudio graph tracer myself (not as a browser extension, but for the Tone lib) and maybe you can already tell me about my preconception mistakes ;)

Thanks!

from audion.

Hmm, Web Audio Inspector's been used for a while now. We override constructors since doing so lets us find orphan nodes. @hoch, have users found that feature useful? I wonder if it's worth avoiding overriding constructors and lazily identifying nodes during calls to connect.

from audion.

@micbuffa Surely you can contribute. Feel free to submit a PR!

@chihuahua I have no stat on the extension at all. Do you have any metric to follow? FWIW, I started to plan the native DevTool integration with WebAudio. Hopefully I will have something to demo soon.

from audion.

...was just passing through and noticed this thread. Is this what you're looking for?

function trackMe(ctor){
    return class extends ctor{
        constructor(...args){
            console.log(`${ctor.name} constructor called.`);
            super(...args);
        }
    }
}
// usage:
GainNode = trackMe(GainNode)
// -- or --
Reflect.ownKeys(window).map(p => window[p])
  .filter(p => p?.prototype?.constructor)
  .filter(p=> Reflect.getPrototypeOf(p) === AudioNode)
  .forEach(Node => {
      window[Node.name] = trackMe(Node);
  });

// test:
const context =new AudioContext();
const gain= new GainNode(context);
// GainNode constructor called 
const filter= new BiquadFilterNode(context);
// BiquadFilterNode constructor called 

Sadly this does not affect context.createGain() usage types, but it would be easy enough to monkey patch those methods.

from audion.