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-
WIP Patch (32.62 KB, patch)
2020-12-16 16:56 PST, Chris Dumez
ews-feeder: commit-queue-
WIP Patch (32.64 KB, patch)
2020-12-16 17:02 PST, Chris Dumez
no flags
Patch (38.47 KB, patch)
2020-12-17 08:57 PST, Chris Dumez
ews-feeder: commit-queue-
Patch (38.50 KB, patch)
2020-12-17 09:00 PST, Chris Dumez
no flags
Patch (38.46 KB, patch)
2020-12-17 09:44 PST, Chris Dumez
ews-feeder: commit-queue-
Patch (38.49 KB, patch)
2020-12-17 09:51 PST, Chris Dumez
no flags
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
Chris Dumez
Comment 5 2020-12-17 09:00:10 PST
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
Chris Dumez
Comment 11 2020-12-17 09:51:38 PST
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
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.