Bug 208179

Created attachment 391637 [details] Patch

<rdar://problem/59758451>

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?

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

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

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

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

Created attachment 391716 [details] Windows build fix attempt 1

Created attachment 391721 [details] Windows build fix attempt 2

Created attachment 391728 [details] Windows build fix attempt 3

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

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.

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

Created attachment 391826 [details] Patch for landing

Comment on attachment 391826 [details] Patch for landing Wait for EWS...

Created attachment 391829 [details] Patch for landing

Ugh... unified builds strikes again :(

Created attachment 391832 [details] Patch for landing

Comment on attachment 391832 [details] Patch for landing Clearing flags on attachment: 391832 Committed r257551: <https://trac.webkit.org/changeset/257551>

All reviewed patches have been landed. Closing bug.