Revise instantiation process of AudioWorkletNode and AudioWorkletProcessor #1265

hoch · 2017-06-21T21:10:16Z

PTAL.

hoch · 2017-06-26T23:11:30Z

@annevk It would be great to have your feedback here as well. @domenic helped a lot when I started working on this.

joeberkovitz · 2017-06-27T19:22:09Z

index.html

+            <dd>
+              Belongs to <a>BaseAudioContext</a>. This contains a collection of
+              <a>AudioParamDescriptor</a>s with an associated string key of
+              node name. This internal storage is populated from <a>node name


I would reference the preceding definition of the string key so that it is clearer that the keys are the same in both internal maps.

joeberkovitz · 2017-06-27T19:22:23Z

index.html

-            When <a>AudioWorkletNode</a>(<var>context</var>, <var>name</var>,
-            <var>options</var>) constructor is called, the user agent must run
-            the following steps <a data-lt="atomic">atomically</a>.
+            Note that the instantiation of these two objects spans with the


typo - remove "with"

joeberkovitz · 2017-06-27T19:27:24Z

index.html

-            <li>Return <var>this</var> as a result of the construction.
+            <li>Queue a task the <a>control thread</a> to fire
+            <code>running</code> event to the associated
+            <a>AudioWorkletNode</a>.


Should this be described as "setting the node's state to running and firing a state change event"?

Also, should the communication of the parameter descriptors list to the control thread be mentioned here?

I think it is correct that queueing a task 1) to change the node state and 2) to fire a state change event. In the rendering thread, the node's state cannot be changed.

Also the communication of parameter descriptors does not happen at the individual regsiterProcessor call. The synchronization of registered definitions should happen at the end of addModule() call. I filed an issue for this step.

I'm still not seeing a clarification of the nature of the event -- the point is that 1) the change in the node state is not an event, but an action by the control thread task, and 2) the event is astatechange event not a running event/

You're correct. I will make the fix.

annevk · 2017-06-27T23:31:17Z

In the preview "Queue a control message" is a dead link.

"If processorConstructor is undefined, throw a NotSupportedError DOMException." this happens in the rendering thread, but where would this exception end up? There's no global.onerror to catch them, right? Might as well terminate these steps then? Or maybe there's other ways to create these objects where the exception would be observable?

hoch · 2017-06-30T22:26:12Z

@annevk A good point. The PaintWorklet spec also throws when anything bad happens in PaintWorkletGlobalScope. (e.g. registerPaint() call) I am not sure how this behavior is explained in that spec. Perhaps it has the same issue?

WorkletGlobalScope.onerror actually makes sense. That way we can delegate user agents with how to handle a thrown exception.

hoch · 2017-06-30T23:29:01Z

Sorry for the messy commits. Please take a look at the last one: 0f41913

padenot · 2017-07-04T06:36:56Z

This really needs to be rebased cleanly on top of gh-pages, I can't make sense of it. Let me know if you need help.

joeberkovitz · 2017-07-06T15:21:20Z

@hoch I can't review this yet either, it includes a bunch of diffs from unmerged commits in gh-pages

hoch · 2017-07-06T15:25:12Z

Sorry, I think I have to recreate a PR. I failed to rebase it cleanly and now it is difficult to resolve it. I have to cherry-pick my own edits and replay them on top of the current gh-pages.

padenot · 2017-07-06T15:27:55Z

Yes, that's what you should do. You can then force-push to this branch and it will be clean. No need to close and re-open.

hoch · 2017-07-06T20:17:34Z

@padenot @joeberkovitz Sorry for the confusion and updated the PR with the force-push.

Preview

joeberkovitz · 2017-07-12T15:48:50Z

index.html

+              populated when a promise from <a href=
+              "https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule">addModule()</a>
+              on <a href="#widl-Window-audioWorklet">audioWorklet</a> gets
+              resolevd.


Typo: resolevd

joeberkovitz · 2017-07-12T15:50:40Z

index.html

            <a>AudioWorkletGlobalScope</a>’s <a>node name to processor
            definition map</a>, throw a <code>NotSupportedError</code>
-            exception and abort these steps.
+            exception and abort these steps. <a>BaseAudioContext</a>’s <a>node


It looks like you had a merge problem here with duplicated language. I assume that the control thread only checks the BaseAudioContext map, not the AWGS map.

You're right. It was a mistake.

joeberkovitz · 2017-07-12T15:56:48Z

index.html

+            an exception (either explicitly or implicitly), abort the rest of
+            steps and queue a task on the control thread to fire <a href=
+            "#widl-AudioWorkletNode-onprocessorstatechange"><code>processorstatechange</code></a>
+            event to <var>node</var>.


I think we need to say what the resulting state of the node is (i.e. "error"), yes?

rtoy · 2017-07-21T16:01:15Z

index.html

-            exception and abort these steps.
+            <li>If <var>nodeName</var> does not exists as a key in the
+            <a>BaseAudioContext</a>’s <a>node name to parameter descriptor
+            map</a>, throw a <code>NotSupportedError</code> exception and abort


Need to wrap the "throw" part in a <span synchronous> tag.

What is <span synchronous>?

I think that it's a way of explicitly saying "this operation is synchronous". However, I am not sure why that is necessary here. This section of algorithm happens in the main thread, so everything is sequential and synchronous.

Yeah, and it's actually <span class="synchronous">. And not really sure it's needed, but most of the spec has this wherever we throw an error.

That seems like a really weird convention. Where did it come from? In general I'd recommend following the conventions established by https://infra.spec.whatwg.org/ and HTML.

In any case it seems moot if we are in the main thread and not employing any contexts belonging to other threads. In the past, we had been looking at data belonging to the AWP.

Seems like we digressed. Is there anything to be fixed in this PR? I think the current spec text here has a clear distinction between the main thread and the worklet thread. So adding a synchronous icon seems redundant to me.

rtoy · 2017-07-21T16:01:22Z

index.html

+              <dfn>node name to parameter descriptor map</dfn>
+            </dt>
+            <dd>
+              Belongs to <a>BaseAudioContext</a>. This map contains the


Nit: "the" -> "an"

rtoy · 2017-07-21T16:02:51Z

index.html

+            "#widl-AudioWorkletNode-port">port</a> to <var>nodePort</var>.
+            </li>
+            <li>Let <var>parameterDescriptors</var> be the result of retrieval
+            by <var>nodeName</var> from <a>node name to parameter descriptor


"by" or "of"?

rtoy · 2017-07-21T16:05:07Z

index.html

-                "value", <a href="https://tc39.github.io/ecma262/#sec-get-o-p">
-                  Get</a>(O=<var>options</var>, P=<var>paramName</var>),
-                  false).
+                <li>For each <var>paramNameInOption</var> → <var>value</var> of


Should the little arrow character be some html character entity instead? (Can't remember how html works with non-ASCII stuff.)

rtoy · 2017-07-21T16:06:17Z

index.html

+                <li>For each <var>paramNameInOption</var> → <var>value</var> of
+                <var>options</var>:
+                  <ol>
+                    <li>If there exists an entry with name member equal to


Shouldn't there be some markup for "name" or "name member" so that it denotes a variable or something?

name nor name member is not defined anywhere. We can fully expand this section but I wanted avoid too much clutter. Do you think this is too confusing?

rtoy · 2017-07-21T16:07:20Z

index.html

+                    <var>paramNameInOption</var> inside
+                    <var>audioParamMap</var>, set that <a>AudioParam</a>'s
+                    value to <var>value</var>.
+                    </li>


Maybe describe what happens if the entry doesn't exist? (In code, it's obvious, but with text and lists, it's not so clear.)

rtoy · 2017-07-21T16:10:00Z

index.html

+            <a>AudioWorkletProcessor</a>, given a string <var>nodeName</var>, a
+            serialization record <var>processorPortSerialization</var>, and an
+            <a>AudioWorkletNode</a> <var>node</var>, perform the following
+            steps on the <a>rendering thread</a>. If any of these steps throws


Can the rendering thread throw an exception? And if it can, where does it go? I think what you're saying is if something bad happens on the rendering thread, the rendering thread stops processing and queues a task to the control thread. We're not throwing an exception in the JS sense?

@bfgeek What should happen when JS code throws in WorkletGlobalScope?

According to @bfgeek, WorkletGlobalScope does not have any error handling. So any exception that is being thrown here will be silently ignored. I think this issue should be handled by the worklet infra.

Don't audio worklets introduce a subclass of that class? Couldn't you add something there? Since you have message passing it seems good to be able to catch uncaught exceptions as well. (Maybe best discussed as a new issue?)

IMO adding its own error handling whereas the parent class does not one is likely to cause the layering issue. What if the error handling will be added into WorkletGlobalScope in the future and it is different from what we do in AudioWorkletGlobalScope?

By the way, I filed an issue to Worklet folks.

rtoy · 2017-07-21T16:14:42Z

index.html

+            name to processor definition map</a>.
+            </li>
+            <li>If <var>processorConstructor</var> is <code>undefined</code>,
+            throw a <code>NotSupportedError</code> DOMException.


NotSupportedError or InvalidStateError?

PaintWorklet throws TypeError. According to ES spec, it "Indicates the actual type of an operand is different than the expected type."

Makes sense to me. WDYT?

In some Chrome CLs, haraken@ said that TypeError should be handled at the JS binding layer. If the binding handles this (which is seems it could?), then TypeError is fine.

This is actually a part of Worklet infra, so we should follow what it does.

rtoy · 2017-07-25T19:49:24Z

@padenot did this change quite some time ago. I no longer remember the reason or the issue/pull request for this.

…

On Tue, Jul 25, 2017 at 12:42 AM, Anne van Kesteren < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In index.html <#1265 (comment)> : > </p> <ol> <li>Let <var>this</var> be the instance being created by constructor of <a>AudioWorkletNode</a> or its subclass. </li> - <li>If <var>name</var> does not exists as a key in the - <a>AudioWorkletGlobalScope</a>’s <a>node name to processor - definition map</a>, throw a <code>NotSupportedError</code> - exception and abort these steps. + <li>If <var>nodeName</var> does not exists as a key in the + <a>BaseAudioContext</a>’s <a>node name to parameter descriptor + map</a>, throw a <code>NotSupportedError</code> exception and abort That seems like a really weird convention. Where did it come from? In general I'd recommend following the conventions established by https://infra.spec.whatwg.org/ and HTML. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1265 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAofPNfztdI779VFW1oGDg0KGwoFlWJyks5sRZx-gaJpZM4OBhgJ> .

-- Ray

padenot · 2017-07-28T15:33:09Z

That seems like a really weird convention. Where did it come from? In general I'd recommend following the conventions established by https://infra.spec.whatwg.org/ and HTML.

infra didn't exist back then ! I followed a pattern I found in HTML, for example in the media resource selection algorithm.

In practice it's useful because most of what the Web Audio API needs to do is async.

hoch · 2017-08-04T15:56:10Z

Here's where I am on this:

This section will not use <span synchronous> because here we clearly define synchronous/asynchronous operations.
The error handling on AudioWorkletGlobalScope should be discussed/defined in the Worklet spec first, then we can downstream the fix to AudioWorklet. I want to handle this in a different PR.

@rtoy @annevk @padenot @joeberkovitz WDYT?

annevk · 2017-08-04T16:40:21Z

It's reasonable to treat error handling separately. A tracking issue would be good.

hoch · 2017-08-04T17:00:44Z

Tracking issue is here: #1282

@annevk Can you approve this PR? You're one of reviewers.

padenot · 2017-08-08T09:43:21Z

This section will not use because here we clearly define synchronous/asynchronous operations.

I prefer consistency, but I don't care that much.

hoch · 2017-08-08T17:39:52Z

@padenot Your last comment doesn't sound like an LGTM. I would like to have another approval before I actually merge this change.

padenot · 2017-08-09T10:09:18Z

index.html

+            </li>
+            <li>If <var>processor</var> does not implement the
+            <a>AudioWorkletProcessor</a> interface, throw an
+            <code>"InvalidStateError"</code> DOMException. NOTE: this can occur


For notes, use <div class=note></div>.

padenot · 2017-08-09T10:15:48Z

index.html

+            <li>If <var>processor</var> does not implement the
+            <a>AudioWorkletProcessor</a> interface, throw an
+            <code>"InvalidStateError"</code> DOMException. NOTE: this can occur
+            if the author-supplied constructor uses JavaScript's


This should be in a non normative section or an example section or something like that.

padenot · 2017-08-09T10:19:43Z

index.html

+          <p>
+            When <a>AudioWorkletNode</a>(<var>context</var>,
+            <var>nodeName</var>, <var>options</var>) constructor is invoked,
+            the user agent must perform the following steps on the control


must or MUST ?

- Rewrite the algorithm. - Add `node name to parameter descriptor map`.

hoch · 2017-08-23T15:44:47Z

Still waiting for the @joeberkovitz's approval.

joeberkovitz · 2017-08-31T15:00:33Z

I was on vacation at the time but the changes looked fine to me.

hoch requested review from joeberkovitz, padenot and rtoy June 21, 2017 21:10

hoch mentioned this pull request Jun 26, 2017

[audioworklet] Ensure MessagePort is exposed in audio worklets #1266

Closed

joeberkovitz reviewed Jun 27, 2017

View reviewed changes

hoch force-pushed the revise-awn-awp-instantiation branch 2 times, most recently from 0f41913 to 20b5f76 Compare June 30, 2017 23:17

hoch closed this Jul 6, 2017

padenot reopened this Jul 6, 2017

hoch force-pushed the revise-awn-awp-instantiation branch from ea6f033 to 4340858 Compare July 6, 2017 20:10

joeberkovitz suggested changes Jul 12, 2017

View reviewed changes

hoch force-pushed the revise-awn-awp-instantiation branch from a234a5e to 593949c Compare July 12, 2017 21:28

rtoy requested changes Jul 21, 2017

View reviewed changes

hoch mentioned this pull request Jul 27, 2017

[worklets] Error handling in WorkletGlobalScope w3c/css-houdini-drafts#433

Open

rtoy approved these changes Aug 7, 2017

View reviewed changes

padenot reviewed Aug 9, 2017

View reviewed changes

padenot approved these changes Aug 9, 2017

View reviewed changes

hoch added 8 commits August 23, 2017 08:32

Revise instantiation process of AWN/AWP

8bf921b

- Rewrite the algorithm. - Add `node name to parameter descriptor map`.

Fix processorConstructor link

2407420

Apply tidy

5ffa1a4

Addressing feedback from @joeberkovitz

bb6ee07

Fix the event type to statechange

f87704c

Reordering

98a9df0

Addresing feedback from @rtoy

690b9a9

Addressing feedback from padenot@

71c1f3a

hoch force-pushed the revise-awn-awp-instantiation branch from 6abadb0 to 71c1f3a Compare August 23, 2017 15:35

hoch merged commit 354c298 into WebAudio:gh-pages Aug 24, 2017

joeberkovitz mentioned this pull request Sep 7, 2017

[audioworklet] Revise AWN/AWP instantiation algorithm: structured serialization and abstract operation #1193

Closed

hoch deleted the revise-awn-awp-instantiation branch March 10, 2021 16:03

Revise instantiation process of AudioWorkletNode and AudioWorkletProcessor #1265

Revise instantiation process of AudioWorkletNode and AudioWorkletProcessor #1265

Uh oh!

Conversation

hoch commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoch commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk commented Jun 27, 2017

Uh oh!

hoch commented Jun 30, 2017

Uh oh!

hoch commented Jun 30, 2017

Uh oh!

padenot commented Jul 4, 2017

Uh oh!

joeberkovitz commented Jul 6, 2017

Uh oh!

hoch commented Jul 6, 2017

Uh oh!

padenot commented Jul 6, 2017

Uh oh!

hoch commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hoch commented Jun 21, 2017 •

edited

Loading