-
Notifications
You must be signed in to change notification settings - Fork 167
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
Split the instantiation algorithm of AWN/AWP into each constructor #2005
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general but I have a few questions.
index.bs
Outdated
following steps on the control thread, where the constructor was | ||
called. | ||
|
||
1. Perform the validity check on <var>options</var>. If the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "the validity check" refer to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of TODO items, based on #1969. I have to plan to address this with a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better, but some things are missing still.
<dl dfn-type="constructor" dfn-for="AudioWorkletProcessor"> | ||
: <dfn>AudioWorkletProcessor(options)</dfn> | ||
:: | ||
When the constructor for {{AudioWorkletProcessor}} is invoked, the following steps are performed: | ||
When the constructor for {{AudioWorkletProcessor}} is invoked, | ||
the following steps are performed on the <a>rendering thread</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mandatory ? It would be best if this was not done on an RT thread, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. AudioWorkletProcessor belongs to the rendering thread. How would you use this object if this is created in somewhere else? Are you creating this from the control and transfer it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to have the flexibility of "construction", you might want to create a follow-up PR after this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor points.
When the constructor for {{AudioWorkletProcessor}} is invoked, the following steps are performed: | ||
When the constructor for {{AudioWorkletProcessor}} is invoked, | ||
the following steps are performed on the <a>rendering thread</a>. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically a move of the existing algorithm from another section to this, right? No other changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on what your "this" means here. This PR has few more changes integrated. I summarized them in the description.
But if you're asking about this L.10147 specifically - Yes. It's a simple move.
PTAL - I would like to tackle this over multiple commits. Some changes are coupled tightly, so it's hard to break down.
DONE
AWP.port
explicitly (VUMeter example uses AudioWorkletProcessor's port in a way that can't work per spec #1973)NewTarget
andactive function object
bit (NewTarget check for AudioWorkletProcessor isn't actually possible with a Web IDL constructor #1963)parameterDescriptor
inAudioWorkletNode
constructor. (parameterDescriptors handling during AudioWorkletNode initialization is probably wrong #1972)Preview | Diff