Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VUMeter example uses AudioWorkletProcessor's port in a way that can't work per spec #1973

Closed
bzbarsky opened this issue Jul 5, 2019 · 5 comments
Assignees

Comments

@bzbarsky
Copy link

bzbarsky commented Jul 5, 2019

https://webaudio.github.io/web-audio-api/#vu-meter-mode has this bit:

registerProcessor('VUMeter', class extends AudioWorkletProcessor {
...
  constructor (options) {    
    super(options);
    this.port.onmessage = event => {
...

but this can't possibly work. Indeed, let's consider what happens when new VUMeterNode is called on the control thread. That lands in https://webaudio.github.io/web-audio-api/#instantiation-of-AudioWorkletNode-and-AudioWorkletProcessor the "In order to process a control message for the construction of an AudioWorkletProcessor" steps, right?

Now step 4 does Construct(processorCtor, options), where processorCtor is in this case the class declaration above. That will invoke the constructor function defined above. The super(options) call lands in https://webaudio.github.io/web-audio-api/#dom-audioworkletprocessor-audioworkletprocessor which does not initialize the "port" member in any way as far as I can tell. That initialization happens in step 5 of the "process a control message for the construction of an AudioWorkletProcessor" steps, which is after the scripted constructor has already returned.

So what happens when the script then does this.port? https://webaudio.github.io/web-audio-api/#AudioWorkletProcessor-attributes seems to assume that the port is always initialized when the getter can be called, but that's clearly not the case.

The only way I see to make this (that is, being able to use the port in constructors of subclasses of AudioWorkletProcessor) work is to make the port a required argument to the AudioWorkletProcessor constructor, either directly or via the existing options object. Then the constructor could set up the port, and there would be no issue here.

@padenot @karlt

P.S. I looked at what Chrome implements, and it simply looks nothing like the spec. What they do is effectively store the port in a slot on the global right before step 4 of "process a control message for the construction of an AudioWorkletProcessor" and putt it out of there in the AudioWorkletProcessor constructor, so it's initialized by the time the super() call returns. If the expectation is that UAs need to do that, then the spec should say so instead of saying something totally different.

@hoch hoch self-assigned this Jul 11, 2019
@hoch
Copy link
Member

hoch commented Jul 11, 2019

so it's initialized by the time the super() call returns. If the expectation is that UAs need to do that, then the spec should say so instead of saying something totally different.

I believe that's our original intention. We don't want to expose the port as an argument of the constructor. So we need to fix:

  1. AudioWorkletProcessor constructor algorithm needs to have a step that initializes the port passed from AudioWorkletNode.
  2. Remove the step 5 in the algorithm above.

@hoch
Copy link
Member

hoch commented Jul 17, 2019

For the reference, this is what Worker spec does (link):

Worker objects act as if they had an implicit MessagePort associated with them. This port is part of a channel that is set up when the worker is created, but it is not exposed. This object must never be garbage collected before the Worker object.

Perhaps we should completely remove the notion of port from the instantiation algorithm, and use a prose to describe this implicit setup/association.

@hoch
Copy link
Member

hoch commented Jul 17, 2019

I tackled it from a bit different angle: we can have a step that describes some implicit initialization in the super() constructor. PTAL at the PR above.

@hoch
Copy link
Member

hoch commented Aug 8, 2019

@hoch hoch closed this as completed Aug 8, 2019
@bzbarsky
Copy link
Author

bzbarsky commented Aug 8, 2019

Please see #2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants