Bug 217704 – Add support for GPUProcess WebAudio media element providers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Add support for GPUProcess WebAudio media element providers

[youenn fablet]

Created attachment 411313 [details]
Patch

[youenn fablet]

Created attachment 411422 [details]
Patch

[youenn fablet]

Created attachment 411428 [details]
Patch

[youenn fablet]

Created attachment 411430 [details]
Patch

[youenn fablet]

Created attachment 411431 [details]
Patch

[youenn fablet]

Created attachment 411432 [details]
Patch

[Eric Carlson]

Comment on attachment 411432 [details]
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?

[youenn fablet]

(In reply to Eric Carlson from comment #7)
> Comment on attachment 411432 [details]
> 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<...>
>     ...
> }

I did that initially but thought it was as good this way.
I'll change the ring buffer callback to pass a UniqueRef.

> > 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)`

CMake build is defining these macros in Source/cmake/OptionsXXX.cmake so it might be safer to keep these.

> > Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.cpp:29
> > +#include "Logging.h"
> 
> Is this needed?

Unified build issue.

[youenn fablet]

Created attachment 411547 [details]
Patch for landing

[EWS]

Committed r268577: <https://trac.webkit.org/changeset/268577>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411547 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70371753>