Bug 217531

Created attachment 410983 [details]
Patch

Created attachment 410984 [details]
Patch

Created attachment 410985 [details]
Patch

Created attachment 410986 [details]
Patch

Created attachment 410987 [details]
Patch

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.

(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).

Created attachment 411118 [details]
Patch

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?

(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.

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

Created attachment 411129 [details]
Patch

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

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

<rdar://problem/70216380>

(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.

(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.