Bug 66441 – Implement WebMediaPlayerClientImpl::audioSourceProvider() and interface into chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Implement WebMediaPlayerClientImpl::audioSourceProvider() and interface into chromium

[Chris Rogers]

Created attachment 104287 [details]
Patch

[Chris Rogers]

This is one patch out of several to implement MediaElementAudioSourceNode (see approach #2):
https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioElementIntegration-section

It's a simple chromium implementation for WebMediaPlayerClientImpl::audioSourceProvider().

which is meant to override the default empty implementation in MediaPlayerPrivateInterface::audioSourceProvider()
Please see this patch for more details of how this grows out of WebCore:
https://bugs.webkit.org/show_bug.cgi?id=66398

It reaches out into chromium code via the WebAudioSourceProvider abstract interface

For the chromium-side, here's a patch for a very early prototype implementation (incomplete and rough edges):
http://codereview.chromium.org/7631033/

[Darin Fisher (:fishd, Google)]

Comment on attachment 104287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104287&action=review

> Source/WebKit/chromium/public/WebMediaPlayer.h:158
> +    virtual WebAudioSourceProvider* webAudioSourceProvider() { return 0; }

nit: we drop the "web" prefix on methods like this.  see for example WebFrame::dataSource().  that returns a WebDataSource.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:626
> +void WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput(WebCore::AudioBus* bus, size_t framesToProcess)

it seems like this function should be listed after we are done defining
all of the WebMediaPlayerClientImpl functions, and methods in the .cpp
file should appear in the same order as they are declared in the .h file.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:173
> +        void set(WebAudioSourceProvider* provider) { m_webAudioSourceProvider = provider; }

nit: set -> initialize?  (it seems like "set" is a bit too vague, and setProvider would be funny too since your implementing a provider.  initialize seems to capture what you are doing.)

[Chris Rogers]

Created attachment 104382 [details]
Patch

[Chris Rogers]

Comment on attachment 104287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104287&action=review

>> Source/WebKit/chromium/public/WebMediaPlayer.h:158
>> +    virtual WebAudioSourceProvider* webAudioSourceProvider() { return 0; }
> 
> nit: we drop the "web" prefix on methods like this.  see for example WebFrame::dataSource().  that returns a WebDataSource.

FIXED

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:626
>> +void WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput(WebCore::AudioBus* bus, size_t framesToProcess)
> 
> it seems like this function should be listed after we are done defining
> all of the WebMediaPlayerClientImpl functions, and methods in the .cpp
> file should appear in the same order as they are declared in the .h file.

I've moved WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput() to the end of the .cpp (after all the WebMediaPlayerClientImpl methods).

Also, I've put the audioSourceProvider() immediately following videoDecodedByteCount() following the order from MediaPlayerPrivateInterface

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:173
>> +        void set(WebAudioSourceProvider* provider) { m_webAudioSourceProvider = provider; }
> 
> nit: set -> initialize?  (it seems like "set" is a bit too vague, and setProvider would be funny too since your implementing a provider.  initialize seems to capture what you are doing.)

Seems reasonable - FIXED

[WebKit Review Bot]

Comment on attachment 104382 [details]
Patch

Clearing flags on attachment: 104382

Committed r93367: <http://trac.webkit.org/changeset/93367>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.