Bug 208179 - Create AudioDestination in the GPU process
Summary: Create AudioDestination in the GPU process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 208044
  Show dependency treegraph
 
Reported: 2020-02-24 22:54 PST by Ryosuke Niwa
Modified: 2020-02-26 19:51 PST (History)
17 users (show)

See Also:


Attachments
Patch (95.91 KB, patch)
2020-02-25 01:33 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
GTK+ build fix attempt 1 (96.22 KB, patch)
2020-02-25 18:15 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
GTK+ build fix attempt 2 (96.26 KB, patch)
2020-02-25 18:37 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
GTK+ build fix attempt 3 (96.44 KB, patch)
2020-02-25 18:45 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Windows build fix attempt 1 (96.56 KB, patch)
2020-02-25 19:01 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Windows build fix attempt 2 (96.56 KB, patch)
2020-02-25 20:56 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Windows build fix attempt 3 (96.75 KB, patch)
2020-02-26 00:59 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (93.33 KB, patch)
2020-02-26 18:22 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (93.33 KB, patch)
2020-02-26 18:37 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (97.28 KB, patch)
2020-02-26 19:05 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-02-24 22:54:57 PST
This bug adds the support for having AudioDestination in GPU process instead of WebContent process when enabled.
Comment 1 Ryosuke Niwa 2020-02-25 01:33:24 PST
Created attachment 391637 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-02-25 01:34:30 PST
<rdar://problem/59758451>
Comment 3 Peng Liu 2020-02-25 10:06:28 PST
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?
Comment 4 Ryosuke Niwa 2020-02-25 18:11:45 PST
(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.
Comment 5 Ryosuke Niwa 2020-02-25 18:15:17 PST
Created attachment 391710 [details]
GTK+ build fix attempt 1
Comment 6 Ryosuke Niwa 2020-02-25 18:37:32 PST
Created attachment 391714 [details]
GTK+ build fix attempt 2
Comment 7 Ryosuke Niwa 2020-02-25 18:45:44 PST
Created attachment 391715 [details]
GTK+ build fix attempt 3
Comment 8 Ryosuke Niwa 2020-02-25 19:01:31 PST
Created attachment 391716 [details]
Windows build fix attempt 1
Comment 9 Ryosuke Niwa 2020-02-25 20:56:51 PST
Created attachment 391721 [details]
Windows build fix attempt 2
Comment 10 Ryosuke Niwa 2020-02-26 00:59:44 PST
Created attachment 391728 [details]
Windows build fix attempt 3
Comment 11 Ryosuke Niwa 2020-02-26 12:28:48 PST
Ping reviewers. All EWS are green (I don’t think GTK+ API test failure is related to this patch).
Comment 12 Jer Noble 2020-02-26 17:25:09 PST
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.
Comment 13 Ryosuke Niwa 2020-02-26 18:06:57 PST
(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.
Comment 14 Ryosuke Niwa 2020-02-26 18:22:10 PST
Created attachment 391826 [details]
Patch for landing
Comment 15 Ryosuke Niwa 2020-02-26 18:23:55 PST
Comment on attachment 391826 [details]
Patch for landing

Wait for EWS...
Comment 16 Ryosuke Niwa 2020-02-26 18:37:36 PST
Created attachment 391829 [details]
Patch for landing
Comment 17 Ryosuke Niwa 2020-02-26 18:57:46 PST
Ugh... unified builds strikes again :(
Comment 18 Ryosuke Niwa 2020-02-26 19:05:24 PST
Created attachment 391832 [details]
Patch for landing
Comment 19 Ryosuke Niwa 2020-02-26 19:51:19 PST
Comment on attachment 391832 [details]
Patch for landing

Clearing flags on attachment: 391832

Committed r257551: <https://trac.webkit.org/changeset/257551>
Comment 20 Ryosuke Niwa 2020-02-26 19:51:21 PST
All reviewed patches have been landed.  Closing bug.