Bug 217153 - Add basic infrastructure for AudioWorklet
Summary: Add basic infrastructure for AudioWorklet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 217058 217194
  Show dependency treegraph
 
Reported: 2020-09-30 16:16 PDT by Chris Dumez
Modified: 2020-10-01 16:22 PDT (History)
26 users (show)

See Also:


Attachments
Patch (150.26 KB, patch)
2020-09-30 17:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (150.32 KB, patch)
2020-09-30 17:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (158.59 KB, patch)
2020-09-30 17:41 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (159.68 KB, patch)
2020-09-30 17:49 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (159.87 KB, patch)
2020-09-30 18:09 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (160.92 KB, patch)
2020-09-30 19:00 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (160.74 KB, patch)
2020-09-30 19:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (160.73 KB, patch)
2020-09-30 19:49 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (162.11 KB, patch)
2020-09-30 20:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (162.65 KB, patch)
2020-10-01 08:24 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (164.01 KB, patch)
2020-10-01 09:07 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (165.93 KB, patch)
2020-10-01 09:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (165.95 KB, patch)
2020-10-01 09:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (159.62 KB, patch)
2020-10-01 12:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (163.84 KB, patch)
2020-10-01 14:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-09-30 16:16:34 PDT
Add basic infrastructure for AudioWorklet:
- https://www.w3.org/TR/webaudio/#audioworklet
- https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
Comment 1 Chris Dumez 2020-09-30 17:20:24 PDT
Created attachment 410177 [details]
Patch
Comment 2 Chris Dumez 2020-09-30 17:24:38 PDT
Created attachment 410179 [details]
Patch
Comment 3 Chris Dumez 2020-09-30 17:41:33 PDT
Created attachment 410182 [details]
Patch
Comment 4 Chris Dumez 2020-09-30 17:49:00 PDT
Created attachment 410183 [details]
Patch
Comment 5 Chris Dumez 2020-09-30 18:09:58 PDT
Created attachment 410186 [details]
Patch
Comment 6 Chris Dumez 2020-09-30 19:00:57 PDT
Created attachment 410189 [details]
Patch
Comment 7 Chris Dumez 2020-09-30 19:46:21 PDT
Created attachment 410194 [details]
Patch
Comment 8 Chris Dumez 2020-09-30 19:49:10 PDT
Created attachment 410195 [details]
Patch
Comment 9 Chris Dumez 2020-09-30 20:42:36 PDT
Created attachment 410198 [details]
Patch
Comment 10 Chris Dumez 2020-10-01 08:24:18 PDT
Created attachment 410234 [details]
Patch
Comment 11 Chris Dumez 2020-10-01 09:07:38 PDT
Created attachment 410236 [details]
Patch
Comment 12 Chris Dumez 2020-10-01 09:45:43 PDT
Created attachment 410238 [details]
Patch
Comment 13 Chris Dumez 2020-10-01 09:52:18 PDT
Created attachment 410240 [details]
Patch
Comment 14 Sam Weinig 2020-10-01 12:12:07 PDT
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.
Comment 15 Chris Dumez 2020-10-01 12:17:52 PDT
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.
Comment 16 Chris Dumez 2020-10-01 12:30:32 PDT
Created attachment 410258 [details]
Patch
Comment 17 Sam Weinig 2020-10-01 14:19:28 PDT
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.
Comment 18 Chris Dumez 2020-10-01 14:27:48 PDT
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 :)
Comment 19 Chris Dumez 2020-10-01 14:43:30 PDT
Created attachment 410270 [details]
Patch
Comment 20 Sam Weinig 2020-10-01 14:47:48 PDT
(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.
Comment 21 Chris Dumez 2020-10-01 14:49:55 PDT
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 :)
Comment 22 Sam Weinig 2020-10-01 14:51:30 PDT
(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.
Comment 23 Chris Dumez 2020-10-01 16:21:15 PDT
Comment on attachment 410270 [details]
Patch

Clearing flags on attachment: 410270

Committed r267859: <https://trac.webkit.org/changeset/267859>
Comment 24 Chris Dumez 2020-10-01 16:21:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2020-10-01 16:22:23 PDT
<rdar://problem/69861084>