Bug 208179 – Create AudioDestination in the GPU process

• Component: Media
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This bug adds the support for having AudioDestination in GPU process instead of WebContent process when enabled.

[Ryosuke Niwa]

Created attachment 391637 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/59758451>

[Peng Liu]

Comment on attachment 391637 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:28
> +

#if ENABLE(GPU_PROCESS)?

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:84
> +        auto protectedThis = makeRef(*this);

The protectedThis is not used?

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:87
> +        Vector<Ref<SharedMemory>> buffers;

Can we make the "buffers" as an instance variable to avoid creating it in every render()?

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:104
> +        auto audioBus = AudioBus::create(buffers.size(), framesToProcess, false);

Is it possible to move this code section into the block of sendWithAsyncReply()?

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:108
> +

An extra blank line.

> Source/WebKit/WebProcess/GPU/media/RemoteAudioBusData.h:27
> +

#if ENABLE(GPU_PROCESS)?

> Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:35
> +#if ENABLE(GPU_PROCESS)

Removed?

[Ryosuke Niwa]

(In reply to Peng Liu from comment #3)
> Comment on attachment 391637 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391637&action=review
> 
> > Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:28
> > +
> 
> #if ENABLE(GPU_PROCESS)?

Fixed.

> > Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:84
> > +        auto protectedThis = makeRef(*this);
> 
> The protectedThis is not used?

That's how protectedThis works:
https://webkit.org/code-style-guidelines/#names-protectors-this

> > Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:87
> > +        Vector<Ref<SharedMemory>> buffers;
> 
> Can we make the "buffers" as an instance variable to avoid creating it in
> every render()?

We can but we're not gonna do that since we're going to use a ring buffer instead. There is no point in making this patch more complicated.

> > Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:104
> > +        auto audioBus = AudioBus::create(buffers.size(), framesToProcess, false);
> 
> Is it possible to move this code section into the block of
> sendWithAsyncReply()?

Why do we want to do that? I'd rather make it clear that we're coming back to the original audio thread after the semaphore.

> > Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:108
> > +
> 
> An extra blank line.

Removed.

> > Source/WebKit/WebProcess/GPU/media/RemoteAudioBusData.h:27
> > +
> 
> #if ENABLE(GPU_PROCESS)?

Added.

> > Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:35
> > +#if ENABLE(GPU_PROCESS)
> 
> Removed?

Oops, removed.

[Ryosuke Niwa]

Created attachment 391710 [details]
GTK+ build fix attempt 1

[Ryosuke Niwa]

Created attachment 391714 [details]
GTK+ build fix attempt 2

[Ryosuke Niwa]

Created attachment 391715 [details]
GTK+ build fix attempt 3

[Ryosuke Niwa]

Created attachment 391716 [details]
Windows build fix attempt 1

[Ryosuke Niwa]

Created attachment 391721 [details]
Windows build fix attempt 2

[Ryosuke Niwa]

Created attachment 391728 [details]
Windows build fix attempt 3

[Ryosuke Niwa]

Ping reviewers. All EWS are green (I don’t think GTK+ API test failure is related to this patch).

[Jer Noble]

Comment on attachment 391728 [details]
Windows build fix attempt 3

r=me. I'm really happy with the MediaStrategy changes and the addition of a receiverMap to the GPUProcessConnection class. I wonder whether the PlatformStrategies should all have default implementations inside WebCore, so subclasses can just return PlatformStrategy::create...() rather than have WebKit and WebKitLegacy both have to hard code the default behavior, but since that's a decision that would affect all the other strategies and isn't specific to media, we could table that discussion for a future patch.

[Ryosuke Niwa]

(In reply to Jer Noble from comment #12)
> Comment on attachment 391728 [details]
> Windows build fix attempt 3
> 
> r=me. I'm really happy with the MediaStrategy changes and the addition of a
> receiverMap to the GPUProcessConnection class. I wonder whether the
> PlatformStrategies should all have default implementations inside WebCore,
> so subclasses can just return PlatformStrategy::create...() rather than have
> WebKit and WebKitLegacy both have to hard code the default behavior, but
> since that's a decision that would affect all the other strategies and isn't
> specific to media, we could table that discussion for a future patch.

Yeah, I think it's best to stick with the existing convention for now. We can have a wider discussion about having a default implementation separately though.

[Ryosuke Niwa]

Created attachment 391826 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 391826 [details]
Patch for landing

Wait for EWS...

[Ryosuke Niwa]

Created attachment 391829 [details]
Patch for landing

[Ryosuke Niwa]

Ugh... unified builds strikes again :(

[Ryosuke Niwa]

Created attachment 391832 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 391832 [details]
Patch for landing

Clearing flags on attachment: 391832

Committed r257551: <https://trac.webkit.org/changeset/257551>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.