Revise sanity checks on AudioWorkletProcessor.process() method #1773

hoch · 2018-09-27T20:28:40Z

Removes the process function check in AudioWorkletProcessor instantiation.
Introduce the new callable process internal slot to AudioWorkletProcessor.
Revise "Rendering an Audio Graph" section to add a new check on process function.
Minor editorial fixes. (numbering ordered list)

rtoy · 2018-09-27T22:15:05Z

index.bs

 		<var>processor</var>.

-	9. <a>Queue a task</a> the <a>control thread</a> to set the associated
+	1. <a>Queue a task</a> the <a>control thread</a> to set the associated


Since you're modifying this anyway: "Queue a task the" -> "Queue a task to the".

rtoy · 2018-09-27T22:15:24Z

index.bs

-				to [=Computing a block of audio|compute a block of audio=] with
-				the argument of [=input buffer=], output buffer and
-				[=input AudioParam buffer=].
+			4. If this {{AudioNode}} is an {{AudioWorkletNode}}, execute these step:


"step:" -> "steps:"

rtoy · 2018-09-27T22:17:22Z

index.bs

+
+				3. If <code><a href="http://www.ecma-international.org/ecma-262/6.0/#sec-iscallable">
+					IsCallable</a>(argument=<i><var>processFunction</var></i>)</code> is
+					false, throw a {{TypeError}}.


Not ReferenceError if the function doesn't exist?

I am no expert on ES6 abstract operation, but technically IsCallable checks if the target is function type. So I think TypeError is correct here.

Example: https://www.ecma-international.org/ecma-262/7.0/#sec-getmethod

rtoy · 2018-10-05T18:22:55Z

index.bs

@@ -9848,7 +9848,8 @@ custom audio node. {{AudioWorkletProcessor}} can
 only be instantiated by the construction of an
 {{AudioWorkletNode}} instance. Every
 {{AudioWorkletProcessor}} has an associated <dfn>node
-reference</dfn>, initially null.
+reference</dfn> and <dfn>callable process</dfn> that are


Are these internal slots (conceptually)? If so, we should probably use the [[foo]] notation to indicate that. If not, then these dfn's define the terms without really defining the what the terms mean.

Isn't the [[foo]] notation for the internal slot that are used by ECMAScript operation? We don't keep node reference in that way because it's not used by the operation itself. It is only used by the algorithm defined in the spec. I think callable process is same as well.

With that said, I am no expert on this issue and not sure what's the right way.

I don't know. But we use it all over some for slots like [[buffer set]] or [[internal reduction]] and these aren't all ECMAScript operations.

Nevertheless, the dfn don't define anything really, so a little bit of text about what it is would be useful. Or maybe you really just want them to be <var>s for the algorithm. This probably requires a bit more work.

rtoy · 2018-10-05T18:24:30Z

index.bs


-		1. The associated {{AudioWorkletProcessor}}'s
-			<a>active source</a> flag is equal to `true`.
+		1. <a>callable process</a> is `true`.


Maybe this should not use numeric items since ordering is not needed?

rtoy · 2018-10-05T19:04:06Z

index.bs

-				[=input AudioParam buffer=].
+			4. If this {{AudioNode}} is an {{AudioWorkletNode}}, execute these steps:
+
+			  1. If <a>callable process</a> is `false`, abort the following sub steps.


I think you need to indent this some more. I see that this step is labeled 5.

Also, when the following steps are aborted, where do we go from here? What's the next step in the algorithm?

rtoy · 2018-10-05T19:07:35Z

index.bs

+					IsCallable</a>(argument=<i><var>processFunction</var></i>)</code> is
+					false, execute the following:
+
+					1. Queue a control message to fire `onprocessorerror` event on the


It would be nice (but not necessary) if onprocessorerror were a link to the attribute.

rtoy · 2018-10-05T19:08:56Z

index.bs

+					1. Queue a control message to fire `onprocessorerror` event on the
+						associated AudioWorkletNode.
+
+					1. Set <a>callable process</a> to `false`.


The algorithm flow is to continue after setting this to false. Is that what you want? I think not since IsCallable returned false, you shouldn't call the process method in the next step.

rtoy · 2018-10-09T23:37:14Z

index.bs

+
+	: <dfn>[[callable process]]</dfn>
+	::
+		A boolean flag represents whether {{AudioWorkletProcessor/process()}} is


Probably s/represents/representing/

rtoy · 2018-10-09T23:37:17Z

index.bs

+	: <dfn>[[callable process]]</dfn>
+	::
+		A boolean flag represents whether {{AudioWorkletProcessor/process()}} is
+		a valid function that can ba invoked.


s/ba invoked/be invoked/

rtoy · 2018-10-09T23:39:12Z

index.bs

-		<dfn dfn>active processing conditions</dfn> are true:
+		every <a>render quantum</a>, if both conditions are met:
+
+		* {{{[[callable process]]}} is `true`.


rtoy · 2018-10-09T23:41:44Z

index.bs


-	9. <a>Queue a task</a> the <a>control thread</a> to set the associated


Don't need to queue anything anymore?

No. statechange event for AudioWorkletNode is removed. So we don't have to do anything here. It was simply an oversight.

rtoy

Looks fine except for one nit.

rtoy · 2018-10-10T18:02:46Z

index.bs

+						[=input AudioParam buffer=].
+
+					1. Else if {{[[callable process]]}} is `false`,
+						<a>Queue a control message</a> to fire `onprocessorerror` event on


Can we specify the type of error that occurred?

This will be an Event on the main thread, so it is not an actual ErrorEvent. You can't throw an error on the main thread, because technically it is an error from the other side.

What we can do instead is to throw a TypeError in AWGS before queueing a message, but it will halt the entire AWGS and that's not what we want here.

I think we have now is sufficient.

Was just basically wondering if we could provide some kind of info in the Event about what happened. This isn't required, but we know exactly why we're sending a message and it would be nice to let the user also know exactly why instead of just some random Event object.

I agree, but the Event object does not have a valid field to fill with the detailed error message. For the Event.type, you'll get processorerror.

Also defining a better error reporting for AWP should be a separate piece of work. I've been meaning to do that, but this becomes also useless when you have a proper error handling mechanism in WGS.

hoch added 4 commits September 27, 2018 13:22

initial commit

b764eb7

fixing Bikeshed errors

f584db0

fixing Bikeshed errors (var)

7a2ecfc

fix numbering

3462f82

rtoy reviewed Sep 27, 2018

View reviewed changes

hoch added 6 commits October 4, 2018 10:03

Addressing feedback from rtoy@

4d3dad6

Merging master

dfb1153

Update PR according to the conclusion in the issue

229bc03

fixing bikeshed error

dd6170f

fixing more bikeshed errors

d09aa36

fixing more bikeshed errors

e98f55b

rtoy requested changes Oct 5, 2018

View reviewed changes

hoch added 3 commits October 8, 2018 11:11

addressing comments from @rtoy

50438fb

Addressing comments from @rtoy

15ee056

fix bikeshed errors

13e46cf

rtoy reviewed Oct 9, 2018

View reviewed changes

Addressing feedback from @rtoy

6e98edd

hoch requested a review from padenot October 10, 2018 16:14

rtoy approved these changes Oct 10, 2018

View reviewed changes

padenot approved these changes Oct 18, 2018

View reviewed changes

hoch merged commit f840c41 into WebAudio:master Oct 18, 2018

hoch deleted the 1767-awp-process-check branch March 10, 2021 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise sanity checks on AudioWorkletProcessor.process() method #1773

Revise sanity checks on AudioWorkletProcessor.process() method #1773

hoch commented Sep 27, 2018 •

edited by pr-preview bot

Loading

rtoy Sep 27, 2018

hoch Oct 4, 2018

rtoy Sep 27, 2018

hoch Oct 4, 2018

rtoy Sep 27, 2018

hoch Oct 4, 2018

rtoy Oct 5, 2018

hoch Oct 8, 2018 •

edited

Loading

rtoy Oct 9, 2018 •

edited by hoch

Loading

rtoy Oct 5, 2018

rtoy Oct 5, 2018

rtoy Oct 5, 2018

rtoy Oct 5, 2018

rtoy Oct 9, 2018

rtoy Oct 9, 2018

rtoy Oct 9, 2018

rtoy Oct 9, 2018

hoch Oct 10, 2018 •

edited

Loading

rtoy left a comment

rtoy Oct 10, 2018

hoch Oct 10, 2018 •

edited

Loading

rtoy Oct 10, 2018

hoch Oct 10, 2018


		9. <a>Queue a task</a> the <a>control thread</a> to set the associated

Revise sanity checks on AudioWorkletProcessor.process() method #1773

Revise sanity checks on AudioWorkletProcessor.process() method #1773

Conversation

hoch commented Sep 27, 2018 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoch Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

rtoy Oct 9, 2018 • edited by hoch Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoch Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

rtoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoch Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoch commented Sep 27, 2018 •

edited by pr-preview bot

Loading

hoch Oct 8, 2018 •

edited

Loading

rtoy Oct 9, 2018 •

edited by hoch

Loading

hoch Oct 10, 2018 •

edited

Loading

hoch Oct 10, 2018 •

edited

Loading