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

Revise the initialization of AudioWorkletProcessor.port #2000

Closed
wants to merge 4 commits into from

Conversation

hoch
Copy link
Member

@hoch hoch commented Jul 17, 2019

Fixes #1973.

Break down the processor constructor into two parts:

  1. Describe some implicit initialization in super() constructor. (port, node reference and etc)
  2. Then invoke the user-supplied constructor if it exists.

Preview | Diff

@hoch hoch requested a review from padenot July 17, 2019 16:55

1. Set <var>processor</var>'s {{[[node reference]]}}to
<var>node</var>.
1. During the execution of the AudioWorkletProcessor super class

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these steps actually go inside that constructor? That would be a lot more clear in terms of what needs to happen. The way this is written right now, someone implementing that constructor needs to know about these steps here that monkeypatch it...

If the steps were there, that would also make it a lot more clear how things should work if multiple AudioWorkletProcessor constructors run under the Construct call here, etc.

This does require making it explicit how the information propagates from here to the relevant constructor, but that's a feature in this case, I would think, because it enables implementations to actually do this interoperably.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these steps actually go inside that constructor?

That's why I wrote "During the execution of...". The substeps are supposed to be the implicit part of super() call. After the super() constructor, these fields are ready to use.

The way this is written right now, someone implementing that constructor needs to know about these steps here that monkeypatch it...

I think of it as a contract of using AudioWorkletProcessor class. These are features provided by UA. Not sure what you meant by monkeypatching. Some of the references are hidden and only used internally.

how things should work if multiple AudioWorkletProcessor constructors run under the Construct call here

What do you mean by "multiple AudioWorkletProcessor constructors"?

because it enables implementations to actually do this interoperably.

Do you see that these substeps can be the source of interoperability issues?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I wrote "During the execution of...". The substeps are supposed to be the implicit part of super() call.

I don't understand why they are implicit. Why not just make them explicit somewhere in https://webaudio.github.io/web-audio-api/#dom-audioworkletprocessor-audioworkletprocessor?

I think of it as a contract of using AudioWorkletProcessor class. These are features provided by UA. Not sure what you meant by monkeypatching. Some of the references are hidden and only used internally.

I mean how is the UA engineer implementing https://webaudio.github.io/web-audio-api/#dom-audioworkletprocessor-audioworkletprocessor supposed to know that in addition to the 3 steps it lists these other steps also need to happen at that same point?

What do you mean by "multiple AudioWorkletProcessor constructors"?

  class A extends AudioWorkletProcessor {};
  class B extends AudioWorkletProcessor {
    constructor() {
      let x = new A();
      super();
      let y = new A();
    }
  };

If B is now registered as a processor, when we do the Construct call for it, we will get three executions of the AudioWorkletProcessor constructor: one under the new A() call that sets x, one for the super() call, and another for the new A() call that sets y. Which, if any, of those AudioWorkletProcessor instances should have .port set to the given port and why? The proposed spec text does not make that clear at all.

because it enables implementations to actually do this interoperably.

Do you see that these substeps can be the source of interoperability issues?

Yes, of course. See my example above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why they are implicit. Why not just make them explicit somewhere in https://webaudio.github.io/web-audio-api/#dom-audioworkletprocessor-audioworkletprocessor?

By adding port as an explicit argument of AudioWorkletProcessor constructor? I am really reluctant to change the constructor interface at this point. If you want me to move those substeps to the AWP constructor algorithm, I can try that.

If B is now registered as a processor, when we do the Construct call for it, we will get three executions of the AudioWorkletProcessor constructor: one under the new A() call that sets x...

The AWP constructor step 1 does not allow the direction invocation of AWP constructor. I understand that you do not agree with that design, but the current spec does not allow such construction.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By adding port as an explicit argument of AudioWorkletProcessor constructor?

No, by being explicit in the AudioWorkletProcessor constructor about how it determines the port.

If you want me to move those substeps to the AWP constructor algorithm, I can try that.

Yes, that is what I want you to do. That will force you to think about how the constructor will actually get this data, and what happens if multiple constructors run.

The AWP constructor step 1 does not allow the direction invocation of AWP constructor.

My test code doesn't do direct invocation; it only constructs subclasses. Everything it does is perfectly allowed by the spec as it is written right this second.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your help! I'll follow up on your suggestion with a different PR.

@hoch
Copy link
Member Author

hoch commented Jul 26, 2019

This approach doesn't seem to work. Closing.

@hoch hoch closed this Jul 26, 2019
@hoch hoch deleted the 1973-port-initialization branch March 10, 2021 16:03
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 this pull request may close these issues.

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