Very early implementation for HTMLMediaElement / Web Audio API integration

Very early implementation for HTMLMediaElement / Web Audio API integration

This is for early high-level comments and later will be split up into separate, more modular patches.

[Chris Rogers, 2011-08-17 22:59:02]

Early working prototype code.  Looking for early review to iron out the rough
spots.

I'll email out the WebKit-side patch separately.

[scherkus (not reviewing), 2011-08-23 15:16:00]

didn't take a deep look but spotted a few higher-level things

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:61: class AudioSink : public
base::RefCountedThreadSafe<AudioSink> {
what's the reasoning behind using an AudioSink as opposed to passing an
AudioDevice directly?

should also get moved to a separate .h file w/ docs

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:98: bool Start();
Start/Stop/SetVolume() should become virtual and OVERRIDE

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:74: void
AudioRendererImpl::Render(const std::vector<float*>& audio_data,
I haven't taken an indepth look at this class but we'll likely have to do some
manual testing to make sure it works

http://codereview.chromium.org/7631033/diff/1/content/renderer/renderer_webau...
File content/renderer/renderer_webaudiosourceprovider_impl.h (right):

http://codereview.chromium.org/7631033/diff/1/content/renderer/renderer_webau...
content/renderer/renderer_webaudiosourceprovider_impl.h:15: class
RendererWebAudioSourceProviderImpl : public WebKit::WebAudioSourceProvider,
naming nit... similar to how WebViewClient -> RenderView, what about cutting
down on the Webs and Impls and going with RenderAudioSourceProvider?

http://codereview.chromium.org/7631033/diff/1/content/renderer/renderer_webau...
content/renderer/renderer_webaudiosourceprovider_impl.h:15: class
RendererWebAudioSourceProviderImpl : public WebKit::WebAudioSourceProvider,
docs for this class

http://codereview.chromium.org/7631033/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7631033/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:12: #include
"content/renderer/media/audio_renderer_impl.h"
src/webkit cannot depend on src/content code

what we'll likely have to do is declare an intermediate type/interface that
allows us to create the AudioRendererImpl inside of RenderView but still let us
set the audio provider

[Chris Rogers, 2011-08-24 00:41:51]

Hi Andrew, I've responded to your initial comments and have uploaded a new patch
trying to get us a little closer :)

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:61: class AudioSink : public
base::RefCountedThreadSafe<AudioSink> {
The idea is that there can be more than one kind of AudioSink:
1) AudioDevice which talks to the browser process
2) RenderAudioSourceProvider which connects into MediaElementAudioSourceNode
inside WebKit
3) ... maybe some other kind of destination for rendered audio stream in the
renderer ...

In this latest patch, I've renamed AudioSink to AudioRendererSink

On 2011/08/23 15:16:01, scherkus wrote:
> what's the reasoning behind using an AudioSink as opposed to passing an
> AudioDevice directly?
> 
> should also get moved to a separate .h file w/ docs

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:98: bool Start();
On 2011/08/23 15:16:01, scherkus wrote:
> Start/Stop/SetVolume() should become virtual and OVERRIDE

Done.

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7631033/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:74: void
AudioRendererImpl::Render(const std::vector<float*>& audio_data,
Agreed.  Also, we should loop Aaron into the conversation since he's the one who
provided the majority of the code for this file.

On 2011/08/23 15:16:01, scherkus wrote:
> I haven't taken an indepth look at this class but we'll likely have to do some
> manual testing to make sure it works

http://codereview.chromium.org/7631033/diff/1/content/renderer/renderer_webau...
File content/renderer/renderer_webaudiosourceprovider_impl.h (right):

http://codereview.chromium.org/7631033/diff/1/content/renderer/renderer_webau...
content/renderer/renderer_webaudiosourceprovider_impl.h:15: class
RendererWebAudioSourceProviderImpl : public WebKit::WebAudioSourceProvider,
Ok, sounds good.  I've renamed this class to RenderAudioSourceProvider as you
suggest...

On 2011/08/23 15:16:01, scherkus wrote:
> naming nit... similar to how WebViewClient -> RenderView, what about cutting
> down on the Webs and Impls and going with RenderAudioSourceProvider?

http://codereview.chromium.org/7631033/diff/1/content/renderer/renderer_webau...
content/renderer/renderer_webaudiosourceprovider_impl.h:15: class
RendererWebAudioSourceProviderImpl : public WebKit::WebAudioSourceProvider,
I've added some comments here.  I can add more detail as necessary...

On 2011/08/23 15:16:01, scherkus wrote:
> docs for this class

http://codereview.chromium.org/7631033/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7631033/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:12: #include
"content/renderer/media/audio_renderer_impl.h"
I've abstracted this a bit more and now am dealing with:
media::AudioRendererSink::RenderCallback

On 2011/08/23 15:16:01, scherkus wrote:
> src/webkit cannot depend on src/content code
> 
> what we'll likely have to do is declare an intermediate type/interface that
> allows us to create the AudioRendererImpl inside of RenderView but still let
us
> set the audio provider