Integrate HTMLMediaElement with Web Audio API's MediaElementAudioSourceNode

Integrate HTMLMediaElement with Web Audio API's MediaElementAudioSourceNode
RenderAudioSourceProvider implements WebKit::WebAudioSourceProvider using AudioRendererImpl

BUG=none
TEST=none
(tested locally)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116643

[Chris Rogers, 2011-12-17 00:16:15]

acolwell and/or vrk: overall review and especially details in:
RenderViewImpl, WebMediaPlayerImpl

enal: any comments related to AudioRendererSink, AudioDevice, AudioRendererImpl
henrika and/or xians: sanity check for AudioRendererSink, AudioDevice

Notes:

* I have a JavaScript test with a WebGL visualizer which works well with this
code - code snippet:

    var audioElement = document.getElementById("myAudio");
    var mediaSourceNode = context.createMediaElementSource(audioElement);
    mediaSourceNode.connect(analyser);
    analyser.connect(context.destination);

* The abstraction of AudioRendererSink also moves us one additional step closer
to renderer-side mixing for AudioRendererImpl.  I'm happy to discuss my ideas
here in a separate thread.

* Before anything is landed here, we need one small but important fixup in
WebMediaPlayerClientImpl which relates to a call to setClient()

* We still need one more patch in WebKit to handle resampling and channel
handling related to the setFormat() call.  I have some working code for this,
but it still needs to be tidied up for formal WebKit review

Thanks for your help in review

[henrika (OOO until Aug 14), 2011-12-19 14:26:52]

Checked AudioRenderSink and AudioDevice. The new
RenderCallback::SetAudioRendererSink() method with a default implementation
feels a bit odd. I don't really understand the design pattern here. It feels a
bit like a hack in its current shape. Can you elaborate some more around it.
Thanks.

[henrika (OOO until Aug 14), 2011-12-19 14:27:04]

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.h:79: : public media::AudioRendererSink,
May I ask how we define Source and Sink in Chrome? What would a similar
interface/filter for capture be called? Is sink is used to denote "receiver/sink
of callbacks"?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.h:125: void GetVolume(double* volume);
Any plan to add this to the interface as well?

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:18: // periodically call Render() on a
callback object.  The normal implementation
And a non-normal implementation would do what ;-)

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:31: virtual void
SetAudioRendererSink(media::AudioRendererSink* audio_sink) { }
What about adding a comment on this method?

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:39: // Initialize() sets important
information about the audio stream format.
nit, don't think we have to list the function name in the comment as well.

[tommi (sloooow) - chröme, 2011-12-19 15:26:24]

drive-by nits and curiosity

http://codereview.chromium.org/8980008/diff/5003/chrome/renderer/prerender/pr...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8980008/diff/5003/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_webmediaplayer.cc:29: NULL, // audio
callback
nit: two spaces before //

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.cc:64: return;
maybe log a warning/error?

If ever we call Initialize more than once, we could in theory be passing in
different arguments the second time and then assume that the AudioDevice is
initialized with those parameters and not the ones that were actually used for
the successful initialization.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:272: // it to make callbacks to us
any more.
nit: anymore

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:292: // !!!!! should only call
Play() if we're in the playing state.
will you fix this or is it a todo?
do we need to check for is_initialized_ here as well?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.h:84: // Passing in NULL means to use
the default sink which is the AudioDevice.
should this documentation perhaps rather be in the interface?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:19: client_(0) {
use NULL for initializing pointers.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:23: if (!is_running_) {
nit: just set is_running_ to true and skip the if()

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:29: if (is_running_) {
ditto

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:35: is_running_ = true;
what's the difference between Start() and Play()?  If Start() must be called
before Play() can be called, then should is_running_ perhaps already be true?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:39: is_running_ = false;
same here with Stop vs Pause... could you call Pause() and then Stop()?

Actually... should Play() call Start() and Pause() Stop()?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:71:
renderer_->SetAudioRendererSink(0);
NULL

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:83: for (size_t i = 0; i <
audio_data.size(); ++i)
memcpy?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:91: for (size_t i = 0; i <
audio_data.size(); ++i) {
nit: no {} like above or add {} there

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.h:32: virtual
~RenderAudioSourceProvider() { }
{}

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
content/renderer/render_view_impl.cc:1946: // collection->AddAudioRenderer(new
AudioRendererImpl());
remove this?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
content/renderer/render_view_impl.cc:1947: audio_renderer = new
AudioRendererImpl( /*MessageLoop::current()*/ );
remove /**/ bit?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
content/renderer/render_view_impl.cc:1970: //
scoped_ptr<webkit_glue::WebMediaPlayerImpl> result(
remove?

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:34: virtual ~RenderCallback() { }
{}

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:37: virtual ~AudioRendererSink() { }
{}

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:112: : audio_source_provider_(0),
nit: this isn't needed since audio_source_provider_ is a scoped_refptr.  Also,
NULL should be used instead of 0.

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:136: audio_sink_render_callback_ =
render_callback;
initialize in initializer list

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:138: new
RenderAudioSourceProvider(audio_sink_render_callback_);
initialize this in the initializer list and provide render_callback as the
argument to RenderAudioSourceProvider()'s constructor.
Out of curiosity, why do we need to hang on to audio_sink_render_callback_?
Can we access it via audio_source_provider_?

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:932: if (!audio_sink_render_callback_)
maybe a comment on why we don't return the audio source provider if the callback
isn't set.

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.h:182: scoped_refptr<RenderAudioSourceProvider>
audio_source_provider_;
make member variables private.

[Chris Rogers, 2011-12-19 21:40:34]

Thanks for the reviews so far!

Henrik, I agree that SetAudioRendererSink() feels a bit odd.  Please see
comments below about it.

I believe I've addressed the other comments, or added questions if the issue
hasn't yet been resolved.

http://codereview.chromium.org/8980008/diff/5003/chrome/renderer/prerender/pr...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8980008/diff/5003/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_webmediaplayer.cc:29: NULL, // audio
callback
On 2011/12/19 15:26:24, tommi wrote:
> nit: two spaces before //

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.cc:64: return;
Agreed, I've added a DCHECK

On 2011/12/19 15:26:24, tommi wrote:
> maybe log a warning/error?
> 
> If ever we call Initialize more than once, we could in theory be passing in
> different arguments the second time and then assume that the AudioDevice is
> initialized with those parameters and not the ones that were actually used for
> the successful initialization.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.h:79: : public media::AudioRendererSink,
On 2011/12/19 14:27:05, henrika wrote:
> May I ask how we define Source and Sink in Chrome? What would a similar
> interface/filter for capture be called? Is sink is used to denote
"receiver/sink
> of callbacks"?

"Sink" is just one possible name for the interface.  Some other suggestions are
AudioDestination,
AudioConsumer, AudioEndpoint, AudioReceiver.  Maybe you have some ideas on what
a good name would be?

The idea for the interface is an entity which receives audio by periodically
calling a callback to get
the next N sample-frames of audio.  The Start()/Stop() method will start/stop
the callbacks from happening.
The interface for the callback is called "RenderCallback" and is defined in the
scope of AudioRendererSink (which is the client of that interface).

I would have to study AudioInputDevice in more detail to fully answer your
question about a corresponding interface for capture.  But I think that the most
important feature of the abstraction of an interface would be to implement:
Start();
Stop();
and also to define the CaptureCallback interface for clients of it.

I'm not sure if we will ever need to define this abstraction for
AudioInputDevice, but I think it could be done in a manner consistent with
AudioRendererSink.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.h:125: void GetVolume(double* volume);
We easily can.  It might be more symmetric that way.  What do you think?

On 2011/12/19 14:27:05, henrika wrote:
> Any plan to add this to the interface as well?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:272: // it to make callbacks to us
any more.
On 2011/12/19 15:26:24, tommi wrote:
> nit: anymore

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:292: // !!!!! should only call
Play() if we're in the playing state.
Actually, for now I think it's better to do nothing special here.  I'll address
this by *not* calling Play(), and adding a TODO that we should call Play() under
the right circumstances.  The case where this comes up is very much an edge
case, when somebody has detached an <audio> tag from normal playback (using the
Web Audio API), then put it back in the *normal* state.  It's not going to hit
anybody who's currently using <audio>, and even with the Web Audio API
integration, this case is unlikely to be important.  Will add TODO

On 2011/12/19 15:26:24, tommi wrote:
> will you fix this or is it a todo?
> do we need to check for is_initialized_ here as well?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.h:84: // Passing in NULL means to use
the default sink which is the AudioDevice.
I think you're right given that it's part of the interface.  But, looking closer
I think it would make a lot more sense to just simplify the AudioRendererSink::
RenderCallback interface by removing the method from there, and instead just
make this be a method on AudioRendererImpl.  This method really isn't something
I meant to generalize in the abstract interface.  What do you think?

On 2011/12/19 15:26:24, tommi wrote:
> should this documentation perhaps rather be in the interface?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:19: client_(0) {
Righto! Too much time spent in WebKit :)

On 2011/12/19 15:26:24, tommi wrote:
> use NULL for initializing pointers.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:23: if (!is_running_) {
On 2011/12/19 15:26:24, tommi wrote:
> nit: just set is_running_ to true and skip the if()

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:29: if (is_running_) {
On 2011/12/19 15:26:24, tommi wrote:
> ditto

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:35: is_running_ = true;
The Start()/Play()/Pause()/Stop() state is described in the AudioDevice
interface - now moved to AudioRendererSink with this patch

There *is* the possibility that Pause() could have been called at some point
after Start() was called, so we still need to set is_running_ to true here.

On 2011/12/19 15:26:24, tommi wrote:
> what's the difference between Start() and Play()?  If Start() must be called
> before Play() can be called, then should is_running_ perhaps already be true?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:39: is_running_ = false;
I realize that it's unfortunate to have four calls (Start/Play/Pause/Stop)
instead of just two (Start/Stop).  The reason we have it is because of the
recent simplification of AudioRendererImpl.  For more details, you can look at
the review comments for that CL:
http://codereview.chromium.org/8477037/

We added a TODO in audio_device.h:
  // TODO(crogers): it should be possible to remove the extra complexity
  // of Play() and Pause() with additional re-factoring work in
  // AudioRendererImpl.

On 2011/12/19 15:26:24, tommi wrote:
> same here with Stop vs Pause... could you call Pause() and then Stop()?
> 
> Actually... should Play() call Start() and Pause() Stop()?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:71:
renderer_->SetAudioRendererSink(0);
On 2011/12/19 15:26:24, tommi wrote:
> NULL

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:83: for (size_t i = 0; i <
audio_data.size(); ++i)
This is copying pointer values (one per audio channel - 2 in the stereo case),
not copying blocks of data.

On 2011/12/19 15:26:24, tommi wrote:
> memcpy?

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:91: for (size_t i = 0; i <
audio_data.size(); ++i) {
On 2011/12/19 15:26:24, tommi wrote:
> nit: no {} like above or add {} there

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.h:32: virtual
~RenderAudioSourceProvider() { }
On 2011/12/19 15:26:24, tommi wrote:
> {}

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
content/renderer/render_view_impl.cc:1946: // collection->AddAudioRenderer(new
AudioRendererImpl());
On 2011/12/19 15:26:24, tommi wrote:
> remove this?

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
content/renderer/render_view_impl.cc:1947: audio_renderer = new
AudioRendererImpl( /*MessageLoop::current()*/ );
On 2011/12/19 15:26:24, tommi wrote:
> remove /**/ bit?

Done.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_view...
content/renderer/render_view_impl.cc:1970: //
scoped_ptr<webkit_glue::WebMediaPlayerImpl> result(
On 2011/12/19 15:26:24, tommi wrote:
> remove?

Done.

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:18: // periodically call Render() on a
callback object.  The normal implementation
RenderAudioSourceProvider is an example in this CL of a "non-normal"
implementation.  So the abstraction we're making here isn't just an intellectual
exercise.  But, your point is well taken that the language "normal
implementation" is a bit awkward.  Any suggestions for how I could reword this
comment?

On 2011/12/19 14:27:05, henrika wrote:
> And a non-normal implementation would do what ;-)

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:31: virtual void
SetAudioRendererSink(media::AudioRendererSink* audio_sink) { }
Tommi had a similar comment about this method in audio_renderer_impl.h

There I responded that maybe it's best to simplify this and remove this method
from the abstract interface here, since it's not that generally useful.  And
instead just have it be a method specific to AudioRendererImpl.

What do you think?
On 2011/12/19 14:27:05, henrika wrote:
> What about adding a comment on this method?

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:34: virtual ~RenderCallback() { }
On 2011/12/19 15:26:24, tommi wrote:
> {}

Done.

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:37: virtual ~AudioRendererSink() { }
On 2011/12/19 15:26:24, tommi wrote:
> {}

Done.

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:39: // Initialize() sets important
information about the audio stream format.
On 2011/12/19 14:27:05, henrika wrote:
> nit, don't think we have to list the function name in the comment as well.

Done.

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:112: : audio_source_provider_(0),
On 2011/12/19 15:26:24, tommi wrote:
> nit: this isn't needed since audio_source_provider_ is a scoped_refptr.  Also,
> NULL should be used instead of 0.

Done.

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:138: new
RenderAudioSourceProvider(audio_sink_render_callback_);
Fixed to initialize in initializer list.  Also, I've simplified to not keep
track via audio_source_provider_ as you suggest.  We may be able to clean this
up further by creating the AudioRendererImpl directly here (not passing into
constructor) instead of in RenderViewImpl.

Aaron can help us there...


On 2011/12/19 15:26:24, tommi wrote:
> initialize this in the initializer list and provide render_callback as the
> argument to RenderAudioSourceProvider()'s constructor.
> Out of curiosity, why do we need to hang on to audio_sink_render_callback_?
> Can we access it via audio_source_provider_?

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:932: if (!audio_sink_render_callback_)
I've taken this out, since it's no longer relevant.  It was part of an older bit
of code where it was required.

On 2011/12/19 15:26:24, tommi wrote:
> maybe a comment on why we don't return the audio source provider if the
callback
> isn't set.

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/8980008/diff/5003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.h:182: scoped_refptr<RenderAudioSourceProvider>
audio_source_provider_;
On 2011/12/19 15:26:24, tommi wrote:
> make member variables private.

Done.

[acolwell GONE FROM CHROMIUM, 2011-12-19 23:01:49]

I have a few suggestions I wanted to throw out here before doing a more detailed
review.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:51: sink_ = audio_device_.get();
How about passing the sink in via the constructor? Then the renderer doesn't
have to be aware of sink switching at all.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:15:
media::AudioRendererSink::RenderCallback* renderer)
It feels to me like this should be an AudioRendererSink and all the switching
logic should be in here. 

This object could be passed to the AudioRendererImpl and the AudioDevice could
be the AudioRendererSink passed to this constructor. Calls could be forwarded to
the sink(AudioDevice) until setClient() is called.

http://codereview.chromium.org/8980008/diff/8002/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/8002/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:31: virtual void
SetAudioRendererSink(media::AudioRendererSink* audio_sink) {}
I too believe this should be removed. Conceptually it seems strange to me to
pass an object to an AudioRendererSink with a method that is supposed to change
it's implementation. I can't quite put my finger on what to call it, but it
seems like breaking encapsulation.

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:108:
media::AudioRendererSink::RenderCallback* render_callback,
nit: Consider changing this to WebAudioSourceProvider* so we don't have to leak
unnecessary implementation details into this class.

[Chris Rogers, 2011-12-19 23:28:14]

Aaron, thanks for the initial comments:

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:51: sink_ = audio_device_.get();
We can't pass it in via the constructor because the sink can/will change during
the lifetime of the AudioRendererImpl.

For example, if we just create an ordinary <audio> element, then the constructor
has already been called.  At a later time, we add some code like this:

    var audioElement = document.getElementById("myAudio");
    var mediaSourceNode = context.createMediaElementSource(audioElement);
    mediaSourceNode.connect(analyser);
    analyser.connect(context.destination);

Conversely, it will be possible to disconnect and delete the mediaSourceNode, in
which case the sink will change back to output directly to the audio hardware.



On 2011/12/19 23:01:49, acolwell wrote:
> How about passing the sink in via the constructor? Then the renderer doesn't
> have to be aware of sink switching at all.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:15:
media::AudioRendererSink::RenderCallback* renderer)
On 2011/12/19 23:01:49, acolwell wrote:
> It feels to me like this should be an AudioRendererSink and all the switching
> logic should be in here. 
> 
> This object could be passed to the AudioRendererImpl and the AudioDevice could
> be the AudioRendererSink passed to this constructor. Calls could be forwarded
to
> the sink(AudioDevice) until setClient() is called.

I agree that I should switch this to take an AudioRendererImpl as the argument,
but I think the logic still needs to remain in AudioRendererImpl (see my comment
there about dynamic changes being made to sink during its lifetime).  Also,
there's thread-safety/locking which needs to be in-place directly in 
AudioRendererImpl, so I don't believe this logic can be moved into this class.

http://codereview.chromium.org/8980008/diff/8002/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/8002/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:31: virtual void
SetAudioRendererSink(media::AudioRendererSink* audio_sink) {}
That settles it!  I'm moving this directly into AudioRendererImpl (when we've
finished discussing your other comments)

On 2011/12/19 23:01:49, acolwell wrote:
> I too believe this should be removed. Conceptually it seems strange to me to
> pass an object to an AudioRendererSink with a method that is supposed to
change
> it's implementation. I can't quite put my finger on what to call it, but it
> seems like breaking encapsulation.

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:108:
media::AudioRendererSink::RenderCallback* render_callback,
Actually, I was hoping to remove this constructor argument and create the
AudioRendererImpl directly in WebMediaPlayerImpl::WebMediaPlayerImpl() instead
of in RenderViewImpl.

This goes hand-in-hand with your other comment about changing
RenderAudioSourceProvider::RenderAudioSourceProvider() to directly take an
AudioRendererImpl.  And also is necessary so that we can get move
SetAudioRendererSink() out of the abstract interface and make it a simple method
on AudioRendererImpl.

On 2011/12/19 23:01:49, acolwell wrote:
> nit: Consider changing this to WebAudioSourceProvider* so we don't have to
leak
> unnecessary implementation details into this class.

[acolwell GONE FROM CHROMIUM, 2011-12-20 01:04:06]

A little more clarification about my suggestions. If there is still disagreement
or you'd like more clarification, just IM me tomorrow and we can hash it out.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:51: sink_ = audio_device_.get();
On 2011/12/19 23:28:14, Chris Rogers wrote:
> We can't pass it in via the constructor because the sink can/will change
during
> the lifetime of the AudioRendererImpl.

Why can't we hide this fact from this code though? It seems like
RenderAudioSourceProvider is in a better position to detect this and handle the
switching.

Also since RenderAudioSourceProvider and AudioDevice are both AudioRendererSinks
now, it would be nice to remove specific knowledge of AudioDevice. If we do that
then most of AudioRendererImpl code could be folded into AudioRendererBase in a
follow-on CL.

> 
> For example, if we just create an ordinary <audio> element, then the
constructor
> has already been called.  At a later time, we add some code like this:
> 
>     var audioElement = document.getElementById("myAudio");
>     var mediaSourceNode = context.createMediaElementSource(audioElement);
>     mediaSourceNode.connect(analyser);
>     analyser.connect(context.destination);
> 
> Conversely, it will be possible to disconnect and delete the mediaSourceNode,
in
> which case the sink will change back to output directly to the audio hardware.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:15:
media::AudioRendererSink::RenderCallback* renderer)
On 2011/12/19 23:28:14, Chris Rogers wrote:
> On 2011/12/19 23:01:49, acolwell wrote:
> > It feels to me like this should be an AudioRendererSink and all the
switching
> > logic should be in here. 
> > 
> > This object could be passed to the AudioRendererImpl and the AudioDevice
could
> > be the AudioRendererSink passed to this constructor. Calls could be
forwarded
> to
> > the sink(AudioDevice) until setClient() is called.
> 
> I agree that I should switch this to take an AudioRendererImpl as the
argument,
> but I think the logic still needs to remain in AudioRendererImpl (see my
comment
> there about dynamic changes being made to sink during its lifetime).  

I'm suggesting AudioRendererSink not AudioRendererImpl. Why not handle switching
here since this appears to be the code that seems to detect/control it?

> Also,
> there's thread-safety/locking which needs to be in-place directly in 
> AudioRendererImpl, so I don't believe this logic can be moved into this class.

Isn't the thread-safety issues only because sink_ can change? I'm saying move
that logic in here and don't let sink_ change in AudioRendererImpl.

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:108:
media::AudioRendererSink::RenderCallback* render_callback,
On 2011/12/19 23:28:14, Chris Rogers wrote:
> Actually, I was hoping to remove this constructor argument and create the
> AudioRendererImpl directly in WebMediaPlayerImpl::WebMediaPlayerImpl() instead
> of in RenderViewImpl.

I am definitely in favor of doing that in this CL.

> 
> This goes hand-in-hand with your other comment about changing
> RenderAudioSourceProvider::RenderAudioSourceProvider() to directly take an
> AudioRendererImpl.  And also is necessary so that we can get move
> SetAudioRendererSink() out of the abstract interface and make it a simple
method
> on AudioRendererImpl.

I'm not sure I'd go that far. I just think it means that
RenderAudioSourceProvider and AudioRendererImpl should be constructed and hooked
up in the same source file so we don't burden other code with their coupling.

[Chris Rogers, 2011-12-20 01:09:56]

Hi Aaron, thanks again for looking at this.

I think it's best if we chat tomorrow, since it's easy to misunderstand each
other with this stuff.  Is there a good time for me to catch you?

[tommi (sloooow) - chröme, 2011-12-20 15:05:46]

Thanks for the update Chris.  Some replies below

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:292: // !!!!! should only call
Play() if we're in the playing state.
On 2011/12/19 21:40:35, Chris Rogers wrote:
> Actually, for now I think it's better to do nothing special here.  I'll
address
> this by *not* calling Play(), and adding a TODO that we should call Play()
under
> the right circumstances.  The case where this comes up is very much an edge
> case, when somebody has detached an <audio> tag from normal playback (using
the
> Web Audio API), then put it back in the *normal* state.  It's not going to hit
> anybody who's currently using <audio>, and even with the Web Audio API
> integration, this case is unlikely to be important.  Will add TODO
> 
> On 2011/12/19 15:26:24, tommi wrote:
> > will you fix this or is it a todo?
> > do we need to check for is_initialized_ here as well?
> 

sounds good.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.h:84: // Passing in NULL means to use
the default sink which is the AudioDevice.
On 2011/12/19 21:40:35, Chris Rogers wrote:
> I think you're right given that it's part of the interface.  But, looking
closer
> I think it would make a lot more sense to just simplify the
AudioRendererSink::
> RenderCallback interface by removing the method from there, and instead just
> make this be a method on AudioRendererImpl.  This method really isn't
something
> I meant to generalize in the abstract interface.  What do you think?
> 
> On 2011/12/19 15:26:24, tommi wrote:
> > should this documentation perhaps rather be in the interface?
> 

That sounds reasonable to me.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:39: is_running_ = false;
On 2011/12/19 21:40:35, Chris Rogers wrote:
> I realize that it's unfortunate to have four calls (Start/Play/Pause/Stop)
> instead of just two (Start/Stop).  The reason we have it is because of the
> recent simplification of AudioRendererImpl.  For more details, you can look at
> the review comments for that CL:
> http://codereview.chromium.org/8477037/
> 
> We added a TODO in audio_device.h:
>   // TODO(crogers): it should be possible to remove the extra complexity
>   // of Play() and Pause() with additional re-factoring work in
>   // AudioRendererImpl.
> 
> On 2011/12/19 15:26:24, tommi wrote:
> > same here with Stop vs Pause... could you call Pause() and then Stop()?
> > 
> > Actually... should Play() call Start() and Pause() Stop()?
> 

ah, I see. Thanks for the info.  Two methods would be nice moving forward :)

http://codereview.chromium.org/8980008/diff/5003/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:83: for (size_t i = 0; i <
audio_data.size(); ++i)
On 2011/12/19 21:40:35, Chris Rogers wrote:
> This is copying pointer values (one per audio channel - 2 in the stereo case),
> not copying blocks of data.
> 
> On 2011/12/19 15:26:24, tommi wrote:
> > memcpy?
> 

Yes, I know, but since the pointers are stored in a contiguous vector memcpy
should still work.  However, it's not going make a dramatic difference, so this
is fine.

I wonder though if we could get rid of some of these alloc+copy operations
purely for the sake of being able to call a single method.  Could Render() be
changed to accept a const float** with a size or is the additional vector<>
adding anything of value?

If so, then we could call Render() directly with the webkit vector:

renderer_->Render(&audio_data[0], audio_data.size(), number_of_frames, 0);

[no longer working on chromium, 2011-12-20 15:47:19]

some nits.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:111: }
shall we returns false when it has been initialized?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:273: if (sink_ ==
audio_device_.get())
if sink_!=audio_device_.get(), do we also need to pause the sink_ before
changing to the new sink?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:296: }
Just want to make sure what happens if sink==NULL and
sink_==audio_device_.get()?
Shall we need to handle this case?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:58:
WebKit::WebAudioSourceProviderClient* client) {
Do we allow this client to be NULL?
If it is a valid user case, why we have DCHECK(client_) in provideInput()?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:84: renderer_->Render(v,
number_of_frames, 0);
Please correct me if I am wrong here.
Is there a casting here? If there is, what about using
reinterpret_cast<int16*>(v)

[henrika (OOO until Aug 14), 2011-12-20 21:01:00]

Additional clarifications and replies to questions/comments. Thanks for removing
the "controversial" method in your interface.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.h:79: : public media::AudioRendererSink,
The reason I asked is because I sometimes gets confused when I see how sink and
source are used in Chrome. You could perhaps also argue that this interface "is
used as an" audio-feeder to the audio layer, i.e., functions as a source and not
sink.  But I do see your point here. The interface as such does work as a sink
to the actual caller. With a similar line of reasoning for capture, the
interface would define/initiate a data flow from the interface, hence it would
function as a source. Feels OK.

http://codereview.chromium.org/8980008/diff/5003/content/renderer/media/audio...
content/renderer/media/audio_device.h:125: void GetVolume(double* volume);
I see now reason why we should make an exception for this method only.

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:18: // periodically call Render() on a
callback object.  The normal implementation
typical instead of normal?

http://codereview.chromium.org/8980008/diff/5003/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:31: virtual void
SetAudioRendererSink(media::AudioRendererSink* audio_sink) { }
Seems like you did changes here already. No further comments here.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_device.h:125: void GetVolume(double* volume);
My proposal was to make this one virtual as well.

[Chris Rogers, 2011-12-21 01:38:16]

I think I've addressed all of the comments.  The most interesting change from
the previous patch-set is to move to the model that Aaron proposes where the
AudioRendererImpl doesn't need to worry about the details of changing sinks. 
That logic has now been moved into RenderAudioSourceProvider.  Also,
RenderViewImpl no longer creates the AudioRendererImpl.  Instead, it's now
created in WebMediaPlayerImpl.

Shijing, some of your comments were in areas which were moved to
RenderAudioSourceProvider, and so the code has changed a little there.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_device.h:125: void GetVolume(double* volume);
On 2011/12/20 21:01:01, henrika wrote:
> My proposal was to make this one virtual as well.

Done.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:51: sink_ = audio_device_.get();
On 2011/12/19 23:01:49, acolwell wrote:
> How about passing the sink in via the constructor? Then the renderer doesn't
> have to be aware of sink switching at all.

Done.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:111: }
On 2011/12/20 15:47:19, xians1 wrote:
> shall we returns false when it has been initialized?

Done.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:273: if (sink_ ==
audio_device_.get())
Yes, I've generalized this code.

On 2011/12/20 15:47:19, xians1 wrote:
> if sink_!=audio_device_.get(), do we also need to pause the sink_ before
> changing to the new sink?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:296: }
If sink==NULL and sink_==audio_device_.get() then we don't need to do anything
because we're already using the "default" sink which is audio_device_

On 2011/12/20 15:47:19, xians1 wrote:
> Just want to make sure what happens if sink==NULL and
> sink_==audio_device_.get()?
> Shall we need to handle this case?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:15:
media::AudioRendererSink::RenderCallback* renderer)
On 2011/12/20 01:04:06, acolwell wrote:
> On 2011/12/19 23:28:14, Chris Rogers wrote:
> > On 2011/12/19 23:01:49, acolwell wrote:
> > > It feels to me like this should be an AudioRendererSink and all the
> switching
> > > logic should be in here. 
> > > 
> > > This object could be passed to the AudioRendererImpl and the AudioDevice
> could
> > > be the AudioRendererSink passed to this constructor. Calls could be
> forwarded
> > to
> > > the sink(AudioDevice) until setClient() is called.
> > 
> > I agree that I should switch this to take an AudioRendererImpl as the
> argument,
> > but I think the logic still needs to remain in AudioRendererImpl (see my
> comment
> > there about dynamic changes being made to sink during its lifetime).  
> 
> I'm suggesting AudioRendererSink not AudioRendererImpl. Why not handle
switching
> here since this appears to be the code that seems to detect/control it?
> 
> > Also,
> > there's thread-safety/locking which needs to be in-place directly in 
> > AudioRendererImpl, so I don't believe this logic can be moved into this
class.
> 
> Isn't the thread-safety issues only because sink_ can change? I'm saying move
> that logic in here and don't let sink_ change in AudioRendererImpl. 
> 

Done.

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:58:
WebKit::WebAudioSourceProviderClient* client) {
passing in NULL for client effectively means that there's no client, so we
expect that there will be no more calls to provideInput().

In other words, it would be a bug if provideInput() were called after
setClient(NULL)

On 2011/12/20 15:47:19, xians1 wrote:
> Do we allow this client to be NULL?
> If it is a valid user case, why we have DCHECK(client_) in provideInput()?

http://codereview.chromium.org/8980008/diff/8002/content/renderer/render_audi...
content/renderer/render_audiosourceprovider.cc:84: renderer_->Render(v,
number_of_frames, 0);
No, there's no cast here.  The type is std::vector<float*>

On 2011/12/20 15:47:19, xians1 wrote:
> Please correct me if I am wrong here.
> Is there a casting here? If there is, what about using
> reinterpret_cast<int16*>(v)

http://codereview.chromium.org/8980008/diff/8002/media/filters/audio_renderer...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/8002/media/filters/audio_renderer...
media/filters/audio_renderer_sink.h:31: virtual void
SetAudioRendererSink(media::AudioRendererSink* audio_sink) {}
On 2011/12/19 23:28:14, Chris Rogers wrote:
> That settles it!  I'm moving this directly into AudioRendererImpl (when we've
> finished discussing your other comments)
> 
> On 2011/12/19 23:01:49, acolwell wrote:
> > I too believe this should be removed. Conceptually it seems strange to me to
> > pass an object to an AudioRendererSink with a method that is supposed to
> change
> > it's implementation. I can't quite put my finger on what to call it, but it
> > seems like breaking encapsulation.
> 

Done.

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/8002/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:108:
media::AudioRendererSink::RenderCallback* render_callback,
On 2011/12/20 01:04:06, acolwell wrote:
> On 2011/12/19 23:28:14, Chris Rogers wrote:
> > Actually, I was hoping to remove this constructor argument and create the
> > AudioRendererImpl directly in WebMediaPlayerImpl::WebMediaPlayerImpl()
instead
> > of in RenderViewImpl.
> 
> I am definitely in favor of doing that in this CL.
> 
> > 
> > This goes hand-in-hand with your other comment about changing
> > RenderAudioSourceProvider::RenderAudioSourceProvider() to directly take an
> > AudioRendererImpl.  And also is necessary so that we can get move
> > SetAudioRendererSink() out of the abstract interface and make it a simple
> method
> > on AudioRendererImpl.
> 
> I'm not sure I'd go that far. I just think it means that
> RenderAudioSourceProvider and AudioRendererImpl should be constructed and
hooked
> up in the same source file so we don't burden other code with their coupling.

Done.

[acolwell GONE FROM CHROMIUM, 2011-12-21 22:53:55]

looking good.

http://codereview.chromium.org/8980008/diff/20001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8980008/diff/20001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:29: NULL,  // audio
callback
I think you may need to look at the caller and see if it provides access to
cmd-line options so you can properly set the disable_audio bool.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:63: DCHECK(!is_initialized_);
Should this be a CHECK() ? Do we really want to try to proceed  in release mode
if this gets called twice?

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:64: if (is_initialized_)
I don't think this should be here. We should either support multiple
Initialize() calls or not. Returning early here seem like it could lead to
strange behavior especially since the caller has know way of knowing that all of
the parameters passed in are being completely ignored.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.h (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.h:108: // sink_ is the sink
(destination) for rendered audio.
s/sink_ is the/The/

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.h:109: media::AudioRendererSink*
sink_;
shouldn't this be a scoped_refptr<AudioRendererSink>?

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
content/renderer/render_audiosourceprovider.cc:35: renderer_ = renderer;
Why is this method needed? Can you use what is passed into Initialize() instead?

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
content/renderer/render_audiosourceprovider.cc:89: if (!is_initialized_) {
nit: reverse condition & return early so you don't have to indent everything.
Perhaps just replace w/ CHECK(!is_initialized_) since it doesn't really look
like we support multiple Initialize() calls.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
File content/renderer/render_audiosourceprovider.h (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
content/renderer/render_audiosourceprovider.h:86: scoped_refptr<AudioDevice>
audio_device_;
Do we need to know this is an AudioDevice? Could this be
scoped_refptr<media::AudioRendererSink> default_sink_?

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
I think this should go in media/base so it is in the same directory that the
AudioRenderer interface is declared.

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:17: // rendered audio in the renderer
process.  An implementation is expected to
Remove "in the renderer process". media is techincally independent of the Chrome
process model. The last sentence should also be removed since this code can
operate independent of the Chrome process model.

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:25: class RenderCallback {
Please convert this to 
typedef base::Callback<void(const std::vector<float*>&, size_t, size_t)>
RenderCallback; 

so it matches the callback pattern used in the rest of the media code.

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:43: RenderCallback* callback) = 0;
Change to const RenderCallback&.

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:30: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebAudioSourceProviderClient.h"
Are these third_party includes needed?
content/renderer/render_audiosourceprovider.h I think should be sufficient.

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.h:67: class WebAudioSourceProviderClient;
Is this still needed?

[Chris Rogers, 2011-12-22 00:54:32]

Thanks Aaron, comments addressed except for the Callback one:

http://codereview.chromium.org/8980008/diff/20001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8980008/diff/20001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:29: NULL,  // audio
callback
On 2011/12/21 22:53:56, acolwell wrote:
> I think you may need to look at the caller and see if it provides access to
> cmd-line options so you can properly set the disable_audio bool.

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:63: DCHECK(!is_initialized_);
On 2011/12/21 22:53:56, acolwell wrote:
> Should this be a CHECK() ? Do we really want to try to proceed  in release
mode
> if this gets called twice? 

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:64: if (is_initialized_)
On 2011/12/21 22:53:56, acolwell wrote:
> I don't think this should be here. We should either support multiple
> Initialize() calls or not. Returning early here seem like it could lead to
> strange behavior especially since the caller has know way of knowing that all
of
> the parameters passed in are being completely ignored.

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.h (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.h:108: // sink_ is the sink
(destination) for rendered audio.
On 2011/12/21 22:53:56, acolwell wrote:
> s/sink_ is the/The/

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.h:109: media::AudioRendererSink*
sink_;
On 2011/12/21 22:53:56, acolwell wrote:
> shouldn't this be a scoped_refptr<AudioRendererSink>?

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
File content/renderer/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
content/renderer/render_audiosourceprovider.cc:35: renderer_ = renderer;
On 2011/12/21 22:53:56, acolwell wrote:
> Why is this method needed? Can you use what is passed into Initialize()
instead?

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
content/renderer/render_audiosourceprovider.cc:89: if (!is_initialized_) {
On 2011/12/21 22:53:56, acolwell wrote:
> nit: reverse condition & return early so you don't have to indent everything.
> Perhaps just replace w/ CHECK(!is_initialized_) since it doesn't really look
> like we support multiple Initialize() calls.

Done.

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
File content/renderer/render_audiosourceprovider.h (right):

http://codereview.chromium.org/8980008/diff/20001/content/renderer/render_aud...
content/renderer/render_audiosourceprovider.h:86: scoped_refptr<AudioDevice>
audio_device_;
On 2011/12/21 22:53:56, acolwell wrote:
> Do we need to know this is an AudioDevice? Could this be
> scoped_refptr<media::AudioRendererSink> default_sink_?

Done.

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
File media/filters/audio_renderer_sink.h (right):

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/12/21 22:53:56, acolwell wrote:
> I think this should go in media/base so it is in the same directory that the
> AudioRenderer interface is declared.

Done.

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:17: // rendered audio in the renderer
process.  An implementation is expected to
On 2011/12/21 22:53:56, acolwell wrote:
> Remove "in the renderer process". media is techincally independent of the
Chrome
> process model. The last sentence should also be removed since this code can
> operate independent of the Chrome process model.

Done.

http://codereview.chromium.org/8980008/diff/20001/media/filters/audio_rendere...
media/filters/audio_renderer_sink.h:25: class RenderCallback {
I think it would be better to defer this change to a followup CL because it
brings several more files and changes into this CL (the current clients of
AudioDevice) with associated risk.  The current pattern is effectively identical
to what has been in AudioDevice for quite some time.  So the proposed change
seems orthogonal to the rest of the changes in this patch.  I don't disagree
with your proposal, just the staging of the change.  How about a TODO here? 

On 2011/12/21 22:53:56, acolwell wrote:
> Please convert this to 
> typedef base::Callback<void(const std::vector<float*>&, size_t, size_t)>
> RenderCallback; 
> 
> so it matches the callback pattern used in the rest of the media code.

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:30: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebAudioSourceProviderClient.h"
On 2011/12/21 22:53:56, acolwell wrote:
> Are these third_party includes needed?
> content/renderer/render_audiosourceprovider.h I think should be sufficient.

Done.

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/8980008/diff/20001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.h:67: class WebAudioSourceProviderClient;
On 2011/12/21 22:53:56, acolwell wrote:
> Is this still needed?

Done.

[Chris Rogers, 2011-12-22 00:59:19]

Last patch-set also merges in Eugene's recent commit:
http://src.chromium.org/viewvc/chrome?view=rev&revision=115253

[henrika (OOO until Aug 14), 2011-12-22 08:45:58]

Thanks Chris.

media/audio
media/base

LGTM.

[no longer working on chromium, 2011-12-22 10:04:04]

lgtm too

[enal, 2011-12-22 15:43:25]

On 2011/12/22 10:04:04, xians1 wrote:

lgtm.

I don't like absence of tests, though...

[acolwell GONE FROM CHROMIUM, 2011-12-22 17:43:27]

LGTM

I agree w/ enal though we should get tests for this in soon.

http://codereview.chromium.org/8980008/diff/20005/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8980008/diff/20005/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:307: bool disable_audio =
cmd->HasSwitch(switches::kDisableAudio);
Hmm.. I was thinking that PrerenderWebMediaPlayer's caller would already have
CommandLine handling code. 

If you go up one more level you'll be in RenderViewImpl, but it looks like that
will trigger modifications to several other unrelated files. :(

How about we just put this back down in PrerenderWebMediaPlayer since there is
no real advantage to putting this code here. Sorry to go back and forth. I
should have done a little more research before the original suggestion.

[Chris Rogers, 2011-12-22 20:57:32]

Fixed compile error in webkit/support/webkit_support.cc
I'll throw this at the bots one more time

http://codereview.chromium.org/8980008/diff/20005/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8980008/diff/20005/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:307: bool disable_audio =
cmd->HasSwitch(switches::kDisableAudio);
Hi Aaron, sorry I'm not quite following you here.  Before in the previous
patch-set I wasn't even handling the command-line state and instead was passing
NULL (should have been false).  Can you be more specific about the exact code
change?  Thanks!

On 2011/12/22 17:43:27, acolwell wrote:
> Hmm.. I was thinking that PrerenderWebMediaPlayer's caller would already have
> CommandLine handling code. 
> 
> If you go up one more level you'll be in RenderViewImpl, but it looks like
that
> will trigger modifications to several other unrelated files. :(
> 
> How about we just put this back down in PrerenderWebMediaPlayer since there is
> no real advantage to putting this code here. Sorry to go back and forth. I
> should have done a little more research before the original suggestion.

[acolwell GONE FROM CHROMIUM, 2011-12-22 21:48:49]

Sorry for the confusion. Also please fix the Lint errors that the code review
tool is reporting. I missed those things in my review.

http://codereview.chromium.org/8980008/diff/20005/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8980008/diff/20005/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:307: bool disable_audio =
cmd->HasSwitch(switches::kDisableAudio);
Sorry. Nevermind. This code is fine as is. I think I just confused myself when I
was swapping back and forth between source files during the review.

[Ami GONE FROM CHROMIUM, 2012-01-13 18:02:14]

[post-landing drive-by]

http://codereview.chromium.org/8980008/diff/43006/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/8980008/diff/43006/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.h:106: // This object takes ownership of the
|audio_source_provider|.
This comment is a lie in patchset 11 (and what ended up landing), because
audio_source_provider_ is a raw pointer.
I *think* the truth is that AudioRendererImpl (which receives a copy of
audio_source_provider in RenderViewImpl::createMediaPlayer) is responsible for
deleting the audio_source_provider by holding a scoped_refptr<> to it.

If that's true, please remove the comment here.
If I'm misreading the other code can you clarify?

Thanks!