[webkit-reviews] review granted: [Bug 217704] Add support for GPUProcess WebAudio media element providers : [Attachment 411432] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 15 20:24:12 PDT 2020
Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 217704: Add support for GPUProcess WebAudio media element providers
https://bugs.webkit.org/show_bug.cgi?id=217704
Attachment 411432: Patch
https://bugs.webkit.org/attachment.cgi?id=411432&action=review
--- Comment #7 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 411432
--> https://bugs.webkit.org/attachment.cgi?id=411432
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=411432&action=review
>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:345
> + m_ringBuffer = m_ringBufferCallback ? m_ringBufferCallback(description,
capacity) : nullptr;
> + if (!m_ringBuffer) {
> + m_ringBuffer = makeUnique<CARingBuffer>();
> + m_ringBuffer->allocate(description, capacity);
> + }
Is it possible for m_ringBufferCallback() to return NULL? If not, I would
change this to something like
if (m_ringBufferCallback)
m_ringBuffer = m_ringBufferCallback(...)
else {
m_ringBuffer = makeUnique<...>
...
}
> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderCocoa.mm:144
> + m_dataSource->pushSamples(MediaTime(m_writeCount,
m_inputDescription->sampleRate()), data, frameCount);
Good catch!
> Source/WebKit/GPUProcess/media/RemoteAudioSourceProviderProxy.cpp:29
> +#if ENABLE(GPU_PROCESS) && ENABLE(WEB_AUDIO) && PLATFORM(COCOA)
`ENABLE_WEB_AUDIO` is defined in PlatformEnableCocoa.h, so I think
`ENABLE(WEB_AUDIO) && PLATFORM(COCOA)` can be just `PLATFORM(COCOA)`
> Source/WebKit/GPUProcess/media/RemoteAudioSourceProviderProxy.h:28
> +#if ENABLE(GPU_PROCESS) && ENABLE(WEB_AUDIO) && PLATFORM(COCOA)
Ditto.
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:889
> +#if ENABLE(WEB_AUDIO) && PLATFORM(COCOA)
Ditto.
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:903
> +#if ENABLE(WEB_AUDIO) && PLATFORM(COCOA)
Ditto.
> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:314
> +#if ENABLE(WEB_AUDIO) && PLATFORM(COCOA)
Ditto.
> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:105
> +#if PLATFORM(COCOA) && ENABLE(WEB_AUDIO)
Ditto.
> Source/WebKit/WebProcess/GPU/GPUProcessConnection.h:70
> +#if PLATFORM(COCOA) && ENABLE(WEB_AUDIO)
Ditto.
> Source/WebKit/WebProcess/GPU/GPUProcessConnection.h:108
> +#if PLATFORM(COCOA) && ENABLE(WEB_AUDIO)
Ditto.
> Source/WebKit/WebProcess/GPU/media/RemoteAudioSourceProvider.cpp:29
> #if ENABLE(GPU_PROCESS) && ENABLE(WEB_AUDIO) && PLATFORM(COCOA)
Ditto.
> Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.cpp:29
> +#include "Logging.h"
Is this needed?
More information about the webkit-reviews
mailing list