Bug 217153 – Add basic infrastructure for AudioWorklet

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

[Chris Dumez]

Add basic infrastructure for AudioWorklet:
- https://www.w3.org/TR/webaudio/#audioworklet
- https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

[Chris Dumez]

Created attachment 410177 [details]
Patch

[Chris Dumez]

Created attachment 410179 [details]
Patch

[Chris Dumez]

Created attachment 410182 [details]
Patch

[Chris Dumez]

Created attachment 410183 [details]
Patch

[Chris Dumez]

Created attachment 410186 [details]
Patch

[Chris Dumez]

Created attachment 410189 [details]
Patch

[Chris Dumez]

Created attachment 410194 [details]
Patch

[Chris Dumez]

Created attachment 410195 [details]
Patch

[Chris Dumez]

Created attachment 410198 [details]
Patch

[Chris Dumez]

Created attachment 410234 [details]
Patch

[Chris Dumez]

Created attachment 410236 [details]
Patch

[Chris Dumez]

Created attachment 410238 [details]
Patch

[Chris Dumez]

Created attachment 410240 [details]
Patch

[Sam Weinig]

Comment on attachment 410240 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.idl:31
> +    EnabledAtRuntime=AudioWorklet,

Given that you can't actually created a AudioWorklet unless this is true, I don't think this is necessary.

> Source/WebCore/Modules/webaudio/AudioWorkletNode.idl:31
>      Conditional=WEB_AUDIO,
> -    EnabledBySetting=AudioWorklet,
> +    EnabledAtRuntime=AudioWorklet,

Why is this needed? Seems like this should only ever called on the main thread.

[Chris Dumez]

Comment on attachment 410240 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.idl:31
>> +    EnabledAtRuntime=AudioWorklet,
> 
> Given that you can't actually created a AudioWorklet unless this is true, I don't think this is necessary.

Oh, I think you are right. If so, I can probably keep using a setting. Let me give it a try.

[Chris Dumez]

Created attachment 410258 [details]
Patch

[Sam Weinig]

Comment on attachment 410258 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.cpp:46
> +    auto* document = worklet.document();
> +    auto* frame = document->frame();
> +    auto jsRuntimeFlags = frame ? frame->settings().javaScriptRuntimeFlags() : JSC::RuntimeFlags();

You can get settings directly from Document, and it is guaranteed to be non-null.

> Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.h:57
> +    WeakPtr<AudioWorklet> m_worklet;

Is this used?

> Source/WebCore/Modules/webaudio/AudioWorkletThread.cpp:96
> +            scriptController->scheduleExecutionTermination();
> +            scriptController->forbidExecution();

What ensures scriptController is non-null here?

> Source/WebCore/Modules/webaudio/AudioWorkletThread.h:48
> +struct WorkletParameters {
> +public:
> +    URL windowURL;
> +    JSC::RuntimeFlags jsRuntimeFlags;
> +
> +    WorkletParameters isolatedCopy() const;
> +};

This should get its own file, also no need for the explicit public:.

> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:129
> +    // FIXME: We should probably limit the number of threads we create for offline audio.
> +    m_renderThread = Thread::create("offline renderer", WTFMove(offThreadRendering), ThreadType::Audio);
>      return { };

Should we be using explicit threads at all here? Seems like a WorkQueue style option might work better (especially if these don't need to be "realish-time".

> Source/WebCore/bindings/scripts/preprocess-idls.pl:101
> +$audioWorkletGlobalScopeConstructorsFile = CygwinPathIfNeeded($audioWorkletGlobalScopeConstructorsFile);

If we are going to keep adding new types of global scopes, we really need to work out a better model than this copy and paste silliness.

[Chris Dumez]

Comment on attachment 410258 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.cpp:46
>> +    auto jsRuntimeFlags = frame ? frame->settings().javaScriptRuntimeFlags() : JSC::RuntimeFlags();
> 
> You can get settings directly from Document, and it is guaranteed to be non-null.

Nice, will fix.

>> Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.h:57
>> +    WeakPtr<AudioWorklet> m_worklet;
> 
> Is this used?

Hmm, maybe not yet. I guess I can omit it until it is actually needed.

>> Source/WebCore/Modules/webaudio/AudioWorkletThread.cpp:96
>> +            scriptController->forbidExecution();
> 
> What ensures scriptController is non-null here?

The AudioWorkletGlobalScope constructs it unconditionally and we literally just constructed the AudioWorkletGlobalScope. The script only gets nulled out when prepareForDestruction() gets called.

>> Source/WebCore/Modules/webaudio/AudioWorkletThread.h:48
>> +};
> 
> This should get its own file, also no need for the explicit public:.

Ok.

>> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:129
>>      return { };
> 
> Should we be using explicit threads at all here? Seems like a WorkQueue style option might work better (especially if these don't need to be "realish-time".

This is pre-existing code so I'd rather not change this at this point. Switching to a WorkQueue would also not be a trivial effort I believe because the WorkQueue's backing thread can change and we have assertions everywhere to make sure we are on the audio rendering thread.

>> Source/WebCore/bindings/scripts/preprocess-idls.pl:101
>> +$audioWorkletGlobalScopeConstructorsFile = CygwinPathIfNeeded($audioWorkletGlobalScopeConstructorsFile);
> 
> If we are going to keep adding new types of global scopes, we really need to work out a better model than this copy and paste silliness.

Agreed. I did not anticipate we would add so many new ones :)

[Chris Dumez]

Created attachment 410270 [details]
Patch

[Sam Weinig]

(In reply to Chris Dumez from comment #18)
> 
> >> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:129
> >>      return { };
> > 
> > Should we be using explicit threads at all here? Seems like a WorkQueue style option might work better (especially if these don't need to be "realish-time".
> 
> This is pre-existing code so I'd rather not change this at this point.
> Switching to a WorkQueue would also not be a trivial effort I believe
> because the WorkQueue's backing thread can change and we have assertions
> everywhere to make sure we are on the audio rendering thread.
> 

My comment was only in relation to the comment you added, not about making a code change now.

[Chris Dumez]

Comment on attachment 410258 [details]
Patch

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

> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:-100
> -    // FIXME: We should probably limit the number of threads we create for offline audio.

I did not add the comment, it was already here. As I said, this is pre-existing code that I moved :)

[Sam Weinig]

(In reply to Chris Dumez from comment #21)
> Comment on attachment 410258 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410258&action=review
> 
> > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:-100
> > -    // FIXME: We should probably limit the number of threads we create for offline audio.
> 
> I did not add the comment, it was already here. As I said, this is
> pre-existing code that I moved :)

Ah, ok.

[Chris Dumez]

Comment on attachment 410270 [details]
Patch

Clearing flags on attachment: 410270

Committed r267859: <https://trac.webkit.org/changeset/267859>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/69861084>