Fix #1933: Use FrozenArray for AudioWorkletProcessor process() #2250

rtoy · 2020-09-22T21:53:02Z

Instead of sequence<sequence<Float32Array>>, use FrozenArray<FrozenArray<Float32Array>>.

Define a callback function as well.

Instead of `sequence<sequence<Float32Array>>`, use `FrozenArray<FrozenArray<Float32Array>>`. Define a callback function as well.

rtoy · 2020-09-24T23:07:19Z

Note to self: Making process() be a callback is easy. Just not sure how to integrate that into the spec appropriately.

rtoy · 2020-09-25T14:26:39Z

bikeshed: AudioWorkletProcessCallback would be a more specific name, which might be useful should any other AudioWorkletProcessor callbacks be added in the future.

That works for me.

rtoy · 2020-09-25T14:31:05Z

The third parameter has named properties rather than indexed properties and so is not an array. It is a frozen object, but there is no such pre-defined WebIDL type. It can be simply object here, with details described in the prose.

Yes, my mistake. Too much search-and-replace. I'll have to ask @hoch exactly how he did this.

Rename AudioWorkletProcessorCallback to AudioWorletProcessCallback The third parameter is an object, not frozen array.

Several changes here in no particular order: - Tell bikeshed that `object` is the webidl concept - Fix parameter names for the process callback (plurals, as used in the descriptions) - Rename the dfn for "Processing an input buffer" from "process" to "processing-input-buffer". - Fix up links to `process()` to point to the right place. - The old section on the process method is broken up into two sections. The main one is renaming it to "callback AudioWorkletProcessCallback". This contains most fo the main text. A subsection then describes the parameters to the callback.

rtoy · 2020-09-25T22:22:38Z

I think the latest revision is closer to what we want, but I'm not 100% sure. Still have to describe what happens with all of the FrozenArrays and the callback parameters object map if you do something bad. But I hope this is close enough that we can now concentrate on getting the right text for that.

The callback idea fixes the errors from defining the process method.

hoch

I am not sure how to make a connection between AudioWorkletProcessCallback to AudioWorkletProcessor.process().

hoch · 2020-09-30T17:51:47Z

index.bs

@@ -43,7 +43,10 @@ Markup Shorthands: markdown on, dfn on, css off
 spec: ECMAScript; url: https://tc39.github.io/ecma262/#sec-data-blocks; type: dfn; text: data block;
 url: https://www.w3.org/TR/mediacapture-streams/#dom-mediadevices-getusermedia; type: method; for: MediaDevices; text: getUserMedia()
 </pre>
-
+<!-- We want IDL object -->


What does this mean?

Oh, bikeshed was choosing something else when we did {{object}}. This makes it link to object in the webidl spec, which is want we want, I think.

hoch · 2020-09-30T17:53:25Z

index.bs

+callback AudioWorkletProcessCallback =
+  boolean (FrozenArray<FrozenArray<Float32Array>> inputs,
+           FrozenArray<FrozenArray<Float32Array>> outputs,
+           object  parameters);


nit: a redundant space

hoch · 2020-09-30T17:56:01Z

index.bs

@@ -10373,7 +10381,7 @@ interface AudioWorkletProcessor {

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


I think we're missing is?

hoch · 2020-09-30T19:19:25Z

index.bs

- 				The input audio buffer from the incoming connections provided by the user agent. It has type <code>sequence&lt;sequence&lt;Float32Array>></code>.<code class="nobreak">inputs[n][m]</code> is a {{Float32Array}} of audio samples for the \(m\)th channel of the \(n\)th input. While the number of inputs is fixed at construction, the number of channels can be changed dynamically based on [=computedNumberOfChannels=].
-
-				If there are no [=actively processing=] {{AudioNode}}s connected to the \(n\)th input of the {{AudioWorkletNode}} for the current render quantum, then the content of <code>inputs[n]</code> is an empty array, indicating that zero channels of input are available. This is the only circumstance under which the number of elements of <code>inputs[n]</code> can be zero.
+The [=process()=] callback function is called


Instead of explaining how this is invoked here, referring https://webaudio.github.io/web-audio-api/#rendering-loop directly might be clearer and more specific.

This was pretty much in the original, but I can reference the rendering loop.

Yeah, I think referencing the algorithm itself would be more thorough.

I will remove this and just reference the algorithm.

hoch · 2020-09-30T19:22:32Z

index.bs

+</h6>
+The {{AudioWorkletProcessCallback}} function has the following parameters:
+
+*Insert text here about how these FrozenArrays behave if they get transferred*.


It will get reallocated when the array topology changes or it is transferred.

Added some new text here. PTAL.

Add text to say inputs/outputs are recycled unless the topology changes (number of channels, array is transferred). Then new arrays are allocated.

hoch · 2020-10-02T15:23:52Z

index.bs

- 				The input audio buffer from the incoming connections provided by the user agent. It has type <code>sequence&lt;sequence&lt;Float32Array>></code>.<code class="nobreak">inputs[n][m]</code> is a {{Float32Array}} of audio samples for the \(m\)th channel of the \(n\)th input. While the number of inputs is fixed at construction, the number of channels can be changed dynamically based on [=computedNumberOfChannels=].
-
-				If there are no [=actively processing=] {{AudioNode}}s connected to the \(n\)th input of the {{AudioWorkletNode}} for the current render quantum, then the content of <code>inputs[n]</code> is an empty array, indicating that zero channels of input are available. This is the only circumstance under which the number of elements of <code>inputs[n]</code> can be zero.
+The [=process()=] callback function is called


Yeah, I think referencing the algorithm itself would be more thorough.

hoch · 2020-10-02T15:24:53Z

index.bs


-			parameters:
-				An [=ordered map=] of <var>name</var> → <var>parameterValues</var>. <code>parameters["<var>name</var>"]</code> returns <var>parameterValues</var>, which is a {{Float32Array}} with the automation values of the <var>name</var> {{AudioParam}}.
+This callback returns a {{boolean}}.


The IDL shows the return type. Not sure if this is needed.

Yes, this should be removed.

hoch · 2020-10-02T15:27:20Z

index.bs

+
+		For each array, the array contains the [=computedValue=] of the parameter for all frames in the [=render quantum=]. However, if no automation is scheduled during this render quantum, the array MAY have length 1 with the array element being the constant value of the {{AudioParam}} for the [=render quantum=].
+
+		*Insert text here about how these aren't freshly allocated each time, unless the object is transferred.*


This still says "Insert text here...".

A reminder that this still needs to be fixed. Either here or in the part about inputs/outputs.

hoch · 2020-10-02T15:34:00Z

index.bs

+reallocated if any part of the
+{{AudioWorkletProcessCallback/inputs!!argument}} or
+{{AudioWorkletProcessCallback/outputs!!argument}} arrays are
+transferred.


This doesn't include parameters object. We can (and Chrome does) freeze the object and reallocate if the topology changes.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/modules/webaudio/audio_worklet_processor.cc;l=460

IIRC, the comments in #1933 says we'd have to do much more work than just saying "Freeze" because there's no WebIDL concept of freeze other than FrozenArrays.

Correct. It's less ideal, but we can directly refer ECMAScript algorithm:
https://tc39.es/ecma262/#sec-object.freeze

Or start from WebIDL's FrozenArray's algorithm:
https://heycam.github.io/webidl/#es-frozen-array

Still need description of how parameters are frozen.

Update for Safari and for Edge

Update IDL results for newer browsers

Update summaries for new browsers. Fails = 6573 - passes

Date

This isn't really a typo but an issue with bikeshed not leaving a space at line breaks. See speced/bikeshed#1470.

…dio#2253) * Address WebAudio#2185: Duplicate IDs for Decode callbacks The decode callbacks were multiply defining their arguments. Instead, just have the descriptions of the arguments just link back to the IDL that defines the args. * Update expected errors

As a workaround, clone the bikeshed repo and install bikeshed from the repo. Ideally, want just want to use the version with pip3 instead of cloning the current version of bikeshed, but that's producing unexpected errors. We'll do this for now and recheck at some later date when bikeshed is updated and revert to the old version.

…ebAudio#2264) When using dark mode, use a different background color for the even rows of the channel ordering table so that the entries are visible. While we're at it, make the headings for the audio node properties table, the audio param values table, and the enumeration description table look a little better in dark mode. This is done by specifying a brighter green to make it a bit more legible.

rtoy · 2020-11-03T18:07:40Z

Question: Should we add a something like Example 38 in https://heycam.github.io/webidl/#idl-callback-functions? That is add a method, say, performOp(AudioWorkletProcessCallback process)?

This is a pretty big change in interface so perhaps that's too late for that.

karlt · 2020-11-04T20:45:41Z

Question: Should we add a something like Example 38 in https://heycam.github.io/webidl/#idl-callback-functions? That is add a method, say, performOp(AudioWorkletProcessCallback process)?

That shouldn't be necessary. performOperation in Example 38 identifies the callback, but for AudioWorkletProcessCallback, this is provided by the process property on the registered processor.

rtoy · 2020-11-06T22:32:33Z

Question: Should we add a something like Example 38 in https://heycam.github.io/webidl/#idl-callback-functions? That is add a method, say, performOp(AudioWorkletProcessCallback process)?

That shouldn't be necessary. performOperation in Example 38 identifies the callback, but for AudioWorkletProcessCallback, this is provided by the process property on the registered processor.

Works for me. Thanks!

rtoy · 2020-11-06T22:33:36Z

PTAL. I think I have all the pieces in place now. The preview diff is kind of hard to follow. Perhaps using that and the preview would make it easier to see what was changed.

hoch

LGTM

Fix WebAudio#1933: Use FrozeArray for AudioWorkletProcessor process()

607eb07

Instead of `sequence<sequence<Float32Array>>`, use `FrozenArray<FrozenArray<Float32Array>>`. Define a callback function as well.

rtoy marked this pull request as draft September 22, 2020 21:53

rtoy changed the title ~~Fix #1933: Use FrozeArray for AudioWorkletProcessor process()~~ Fix #1933: Use FrozenArray for AudioWorkletProcessor process() Sep 24, 2020

rtoy added 2 commits September 25, 2020 07:39

Address review comments from karlt.

de45916

Rename AudioWorkletProcessorCallback to AudioWorletProcessCallback The third parameter is an object, not frozen array.

Update expected errors

097a0ed

The callback idea fixes the errors from defining the process method.

hoch reviewed Sep 30, 2020

View reviewed changes

rtoy added 2 commits October 1, 2020 14:46

Address some review comments

f14cc12

Describe what happens to inputs/outputs if things change

e90a8ab

Add text to say inputs/outputs are recycled unless the topology changes (number of channels, array is transferred). Then new arrays are allocated.

hoch self-requested a review October 2, 2020 15:21

hoch reviewed Oct 2, 2020

View reviewed changes

rtoy and others added 13 commits October 2, 2020 09:42

Address some review comments

cc2bef4

Still need description of how parameters are frozen.

Update README.md

b2fdc7d

Fix typo in "messsage" (WebAudio#2255)

c0af014

chore: Fix markdown for bold (WebAudio#2258)

2b6dc31

Update implementation-report.html

443e3d5

Update for Safari and for Edge

Update implementation-report.html

2c4f0c2

Update IDL results for newer browsers

Update implementation-report.html

0af14fe

Update summaries for new browsers. Fails = 6573 - passes

Update implementation-report.html

df661ef

Date

Fix typo "atleast" (WebAudio#2259)

c7cac46

This isn't really a typo but an issue with bikeshed not leaving a space at line breaks. See speced/bikeshed#1470.

Merge branch 'main' into 1933-awn-frozen-arrays

b2c9791

Add ECMAScript algorithm to freeze parameters.

6135447

Some cleanups.

ae984bd

rtoy marked this pull request as ready for review November 6, 2020 22:32

rtoy requested review from padenot and hoch November 12, 2020 17:42

hoch approved these changes Nov 18, 2020

View reviewed changes

rtoy merged commit a331281 into WebAudio:main Nov 19, 2020

rtoy mentioned this pull request Nov 19, 2020

Many Bikeshed errors when building index.html (broken IDL?) #2185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1933: Use FrozenArray for AudioWorkletProcessor process() #2250

Fix #1933: Use FrozenArray for AudioWorkletProcessor process() #2250

rtoy commented Sep 22, 2020 •

edited by pr-preview bot

rtoy commented Sep 24, 2020

rtoy commented Sep 25, 2020

rtoy commented Sep 25, 2020

rtoy commented Sep 25, 2020

hoch left a comment

hoch Sep 30, 2020

rtoy Oct 1, 2020

hoch Sep 30, 2020

rtoy Oct 1, 2020

hoch Sep 30, 2020

rtoy Oct 1, 2020

hoch Sep 30, 2020

rtoy Oct 1, 2020

hoch Oct 2, 2020

rtoy Oct 2, 2020

hoch Sep 30, 2020

rtoy Oct 1, 2020

hoch Oct 2, 2020

hoch Oct 2, 2020

rtoy Oct 2, 2020

hoch Oct 2, 2020

rtoy Oct 2, 2020

hoch Oct 2, 2020

rtoy Oct 2, 2020

hoch Oct 2, 2020

rtoy commented Nov 3, 2020

karlt commented Nov 4, 2020

rtoy commented Nov 6, 2020

rtoy commented Nov 6, 2020

hoch left a comment


		For each array, the array contains the [=computedValue=] of the parameter for all frames in the [=render quantum=]. However, if no automation is scheduled during this render quantum, the array MAY have length 1 with the array element being the constant value of the {{AudioParam}} for the [=render quantum=].

		Insert text here about how these aren't freshly allocated each time, unless the object is transferred.

Fix #1933: Use FrozenArray for AudioWorkletProcessor process() #2250

Fix #1933: Use FrozenArray for AudioWorkletProcessor process() #2250

Conversation

rtoy commented Sep 22, 2020 • edited by pr-preview bot

rtoy commented Sep 24, 2020

rtoy commented Sep 25, 2020

rtoy commented Sep 25, 2020

rtoy commented Sep 25, 2020

hoch left a 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

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

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

rtoy commented Nov 3, 2020

karlt commented Nov 4, 2020

rtoy commented Nov 6, 2020

rtoy commented Nov 6, 2020

hoch left a comment

Choose a reason for hiding this comment

rtoy commented Sep 22, 2020 •

edited by pr-preview bot