Bug 219964

Created attachment 416370 [details]
WIP Patch

Created attachment 416371 [details]
WIP Patch

Created attachment 416373 [details]
WIP Patch

Created attachment 416421 [details]
Patch

Created attachment 416423 [details]
Patch

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.

Comment on attachment 416423 [details]
Patch

Anyway, patch looks good to me.

r=me

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

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)

Created attachment 416431 [details]
Patch

Created attachment 416433 [details]
Patch

Committed r270938: <https://trac.webkit.org/changeset/270938>

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

<rdar://problem/72433468>

(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