Bug 217531 – Use AudioWorkletProcessor to process audio

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Use AudioWorkletProcessor to process audio:
- https://www.w3.org/TR/webaudio/#dom-audioworkletprocessor-process

[Chris Dumez]

Created attachment 410983 [details]
Patch

[Chris Dumez]

Created attachment 410984 [details]
Patch

[Chris Dumez]

Created attachment 410985 [details]
Patch

[Chris Dumez]

Created attachment 410986 [details]
Patch

[Chris Dumez]

Created attachment 410987 [details]
Patch

[youenn fablet]

Comment on attachment 410987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410987&action=review

> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:120
>      JSC::JSObject* jsConstructor = constructor->callbackData()->callback();

auto*

> Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:278
> +            errorMessage = "An error was thrown from AudioWorkletProcessor::process() method"_s;

Do other browsers provide information on the exception that was thrown in the worklet?

> Source/WebCore/Modules/webaudio/AudioWorkletNode.h:34
> +#include "AudioArray.h"

Forward declare AudioFloatArray?

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:56
> +static JSMap* constructJSMap(VM& vm, JSGlobalObject* globalObject, const HashMap<String, std::unique_ptr<AudioFloatArray>>& paramValuesMap)

Can we pass a JSGlobalObject& in all these methods?

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:86
> +static void copyDataFromJSArrayToBuses(JSGlobalObject* globalObject, JSArray* jsArray, Vector<RefPtr<AudioBus>>& buses)

JSArray&.
Maybe also Vector<Ref<>> since we are assuming buses are not null.

[Chris Dumez]

(In reply to youenn fablet from comment #6)
> Comment on attachment 410987 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410987&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:120
> >      JSC::JSObject* jsConstructor = constructor->callbackData()->callback();
> 
> auto*
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:278
> > +            errorMessage = "An error was thrown from AudioWorkletProcessor::process() method"_s;
> 
> Do other browsers provide information on the exception that was thrown in
> the worklet?

I don't know about Firefox but the behavior in this patch matches Blink's behavior.

> 
> > Source/WebCore/Modules/webaudio/AudioWorkletNode.h:34
> > +#include "AudioArray.h"
> 
> Forward declare AudioFloatArray?

It is a typedef like so:
typedef AudioArray<float> AudioFloatArray;

This is why I did not forward-declared it. I will look into how to forward-declare such time though.

> 
> > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:56
> > +static JSMap* constructJSMap(VM& vm, JSGlobalObject* globalObject, const HashMap<String, std::unique_ptr<AudioFloatArray>>& paramValuesMap)
> 
> Can we pass a JSGlobalObject& in all these methods?

OK.

> 
> > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:86
> > +static void copyDataFromJSArrayToBuses(JSGlobalObject* globalObject, JSArray* jsArray, Vector<RefPtr<AudioBus>>& buses)
> 
> JSArray&.

OK.

> Maybe also Vector<Ref<>> since we are assuming buses are not null.

The buses are passed as Vector<RefPtr<AudioBus>> because the *input* buses can be null. The *output* buses cannot be null and are still passed in with the same type to promote code sharing (For the converting to JSArrays).

[Chris Dumez]

Created attachment 411118 [details]
Patch

[Saam Barati]

Comment on attachment 411118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411118&action=review

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:93
> +        auto* channelsArray = asObject(jsArray.getIndex(&globalObject, i));

nit: jsCast<JSArray*> instead of asObject since we're already assuming the types here?

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:135
> +    // FIXME: We should consider caching the JSArrays & JSMap and only update their items
> +    // every time process() is called, for performance reasons.

Can't a user change these though?

[Chris Dumez]

(In reply to Saam Barati from comment #9)
> Comment on attachment 411118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411118&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:93
> > +        auto* channelsArray = asObject(jsArray.getIndex(&globalObject, i));
> 
> nit: jsCast<JSArray*> instead of asObject since we're already assuming the
> types here?

Will fix, thanks.

> > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:135
> > +    // FIXME: We should consider caching the JSArrays & JSMap and only update their items
> > +    // every time process() is called, for performance reasons.
> 
> Can't a user change these though?

The JSArrays I pass to JS are frozen so those cannot change. The JS *can* change the Float32Arrays inside the JSArrays but I can simply zero-out these before recycling them.

[Saam Barati]

Comment on attachment 411118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411118&action=review

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:74
> +    JSC::objectConstructorFreeze(&globalObject, channelsData);

you might need exception checks after this. Or an assert an exception didn't happen

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:84
> +    JSC::objectConstructorFreeze(&globalObject, array);

you might need exception checks after this. Or an assert an exception didn't happen

[Chris Dumez]

Created attachment 411129 [details]
Patch

[EWS]

Committed r268365: <https://trac.webkit.org/changeset/268365>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411129 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70216380>

[Saam Barati]

(In reply to Chris Dumez from comment #10)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 411118 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411118&action=review
> > 
> > > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:93
> > > +        auto* channelsArray = asObject(jsArray.getIndex(&globalObject, i));
> > 
> > nit: jsCast<JSArray*> instead of asObject since we're already assuming the
> > types here?
> 
> Will fix, thanks.
> 
> > > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:135
> > > +    // FIXME: We should consider caching the JSArrays & JSMap and only update their items
> > > +    // every time process() is called, for performance reasons.
> > 
> > Can't a user change these though?
> 
> The JSArrays I pass to JS are frozen so those cannot change. The JS *can*
> change the Float32Arrays inside the JSArrays but I can simply zero-out these
> before recycling them.

For arrays, this makes sense. For JSMap, being frozen doesn't prevent things from getting added/removed from the map, so it might not really be good for perf if the user does stuff to it. Also, what if the user captures a pointer to the map in their code, and you change it from under them? Does the spec say these need to be new objects each time? If so, then reusing the same object is observable identity wise.

[Chris Dumez]

(In reply to Saam Barati from comment #15)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Saam Barati from comment #9)
> > > Comment on attachment 411118 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=411118&action=review
> > > 
> > > > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:93
> > > > +        auto* channelsArray = asObject(jsArray.getIndex(&globalObject, i));
> > > 
> > > nit: jsCast<JSArray*> instead of asObject since we're already assuming the
> > > types here?
> > 
> > Will fix, thanks.
> > 
> > > > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:135
> > > > +    // FIXME: We should consider caching the JSArrays & JSMap and only update their items
> > > > +    // every time process() is called, for performance reasons.
> > > 
> > > Can't a user change these though?
> > 
> > The JSArrays I pass to JS are frozen so those cannot change. The JS *can*
> > change the Float32Arrays inside the JSArrays but I can simply zero-out these
> > before recycling them.
> 
> For arrays, this makes sense. For JSMap, being frozen doesn't prevent things
> from getting added/removed from the map, so it might not really be good for
> perf if the user does stuff to it. Also, what if the user captures a pointer
> to the map in their code, and you change it from under them? Does the spec
> say these need to be new objects each time? If so, then reusing the same
> object is observable identity wise.

1. I am actually not supposed to pass a JSMap to JS. It is supposed to be a frozen JSObject. I am in the process of fixing this.
2. No, the specification does not specify it should be a new array/object every time.
3. Blink already has this optimization, which is why I added a FIXME comment.