[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