WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
219964
[GPUProcess] Replace WebAudio rendering timer with a cross-process semaphore
https://bugs.webkit.org/show_bug.cgi?id=219964
Summary
[GPUProcess] Replace WebAudio rendering timer with a cross-process semaphore
Chris Dumez
Reported
2020-12-16 15:44:58 PST
Replace WebAudio rendering timer (that was added in
Bug 219818
) with a cross-process semaphore.
Attachments
WIP Patch
(30.18 KB, patch)
2020-12-16 16:41 PST
,
Chris Dumez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
WIP Patch
(32.62 KB, patch)
2020-12-16 16:56 PST
,
Chris Dumez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
WIP Patch
(32.64 KB, patch)
2020-12-16 17:02 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(38.47 KB, patch)
2020-12-17 08:57 PST
,
Chris Dumez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(38.50 KB, patch)
2020-12-17 09:00 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(38.46 KB, patch)
2020-12-17 09:44 PST
,
Chris Dumez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(38.49 KB, patch)
2020-12-17 09:51 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2020-12-16 16:41:42 PST
Created
attachment 416370
[details]
WIP Patch
Chris Dumez
Comment 2
2020-12-16 16:56:46 PST
Created
attachment 416371
[details]
WIP Patch
Chris Dumez
Comment 3
2020-12-16 17:02:44 PST
Created
attachment 416373
[details]
WIP Patch
Chris Dumez
Comment 4
2020-12-17 08:57:11 PST
Created
attachment 416421
[details]
Patch
Chris Dumez
Comment 5
2020-12-17 09:00:10 PST
Created
attachment 416423
[details]
Patch
Geoffrey Garen
Comment 6
2020-12-17 09:17:25 PST
Comment on
attachment 416423
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=416423&action=review
> Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:88 > + if (m_dispatchToRenderThread) { > + // If there is an AudioWorklet active, we need to render the quantum on the AudioWorkletThread. > + m_dispatchToRenderThread([this, protectedThis = makeRef(*this)] {
I wonder if we should always initialize m_dispatchToRenderThread, and always call it -- but in the no-worklet case we initialize it to a trivial function that just invokes the passed-in lambda synchronously. That would simplify the logic here.
> Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:186 > + auto dispatchToRenderThread = std::exchange(m_dispatchToRenderThread, nullptr); > + if (dispatchToRenderThread) { > + // Do a round-trip to the worklet thread to make sure we call the completion handler after > + // the last rendering quantum has been processed by the worklet thread. > + dispatchToRenderThread([callCompletionHandler = WTFMove(callCompletionHandler)]() mutable { > + callOnMainThread(WTFMove(callCompletionHandler)); > + }); > + } else > + callCompletionHandler(); > });
Here too I think it would be a little simpler (and a little more consistent) if we had a single flow that always called dispatchToRenderThread and always used callOnMainThread for the callback.
Geoffrey Garen
Comment 7
2020-12-17 09:18:25 PST
Comment on
attachment 416423
[details]
Patch Anyway, patch looks good to me. r=me
Chris Dumez
Comment 8
2020-12-17 09:22:49 PST
(In reply to Geoffrey Garen from
comment #6
)
> Comment on
attachment 416423
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=416423&action=review
> > > Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:88 > > + if (m_dispatchToRenderThread) { > > + // If there is an AudioWorklet active, we need to render the quantum on the AudioWorkletThread. > > + m_dispatchToRenderThread([this, protectedThis = makeRef(*this)] { > > I wonder if we should always initialize m_dispatchToRenderThread, and always > call it -- but in the no-worklet case we initialize it to a trivial function > that just invokes the passed-in lambda synchronously. That would simplify > the logic here. > > > Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:186 > > + auto dispatchToRenderThread = std::exchange(m_dispatchToRenderThread, nullptr); > > + if (dispatchToRenderThread) { > > + // Do a round-trip to the worklet thread to make sure we call the completion handler after > > + // the last rendering quantum has been processed by the worklet thread. > > + dispatchToRenderThread([callCompletionHandler = WTFMove(callCompletionHandler)]() mutable { > > + callOnMainThread(WTFMove(callCompletionHandler)); > > + }); > > + } else > > + callCompletionHandler(); > > }); > > Here too I think it would be a little simpler (and a little more consistent) > if we had a single flow that always called dispatchToRenderThread and always > used callOnMainThread for the callback.
Ok. I will look into doing this in a follow-up since I should probably update the non-GPU process code path accordingly too.
Wenson Hsieh
Comment 9
2020-12-17 09:31:13 PST
Comment on
attachment 416423
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=416423&action=review
> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:151 > + for (unsigned i = 0; i < numberOfFrames; i+= WebCore::AudioUtilities::renderQuantumSize) {
(Minor nit - space after the i)
Chris Dumez
Comment 10
2020-12-17 09:44:35 PST
Created
attachment 416431
[details]
Patch
Chris Dumez
Comment 11
2020-12-17 09:51:38 PST
Created
attachment 416433
[details]
Patch
EWS
Comment 12
2020-12-17 11:11:19 PST
Committed
r270938
: <
https://trac.webkit.org/changeset/270938
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 416433
[details]
.
Radar WebKit Bug Importer
Comment 13
2020-12-17 11:12:20 PST
<
rdar://problem/72433468
>
Chris Dumez
Comment 14
2020-12-17 12:43:04 PST
(In reply to Chris Dumez from
comment #8
)
> (In reply to Geoffrey Garen from
comment #6
) > > Comment on
attachment 416423
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=416423&action=review
> > > > > Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:88 > > > + if (m_dispatchToRenderThread) { > > > + // If there is an AudioWorklet active, we need to render the quantum on the AudioWorkletThread. > > > + m_dispatchToRenderThread([this, protectedThis = makeRef(*this)] { > > > > I wonder if we should always initialize m_dispatchToRenderThread, and always > > call it -- but in the no-worklet case we initialize it to a trivial function > > that just invokes the passed-in lambda synchronously. That would simplify > > the logic here. > > > > > Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:186 > > > + auto dispatchToRenderThread = std::exchange(m_dispatchToRenderThread, nullptr); > > > + if (dispatchToRenderThread) { > > > + // Do a round-trip to the worklet thread to make sure we call the completion handler after > > > + // the last rendering quantum has been processed by the worklet thread. > > > + dispatchToRenderThread([callCompletionHandler = WTFMove(callCompletionHandler)]() mutable { > > > + callOnMainThread(WTFMove(callCompletionHandler)); > > > + }); > > > + } else > > > + callCompletionHandler(); > > > }); > > > > Here too I think it would be a little simpler (and a little more consistent) > > if we had a single flow that always called dispatchToRenderThread and always > > used callOnMainThread for the callback. > > Ok. I will look into doing this in a follow-up since I should probably > update the non-GPU process code path accordingly too.
Follow-up:
https://bugs.webkit.org/show_bug.cgi?id=219990
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug