Adding audio support to the new webmediaplyer

This allow connection remote stream to audio tag, and enable users to control the audio via audio tag controller.

Since WebRtcAudioDeviceImpl is not a thread safe class, it can only take commands from VoE, so the output code (including AudioOutputDevice) is broken down into a WebRtcAudioRenderer. This WebRtcAudioRenderer gets calls from 3 threads:
WebRtcAudioDeviceImpl thread for commands: Play(), Pause()
WebMediaPlayer thread for commands: Play(), Pause(), Stop(), SetVolume()
Audio thread: Render() callback.


BUG=142988
TEST=https://webrtc-demos.appspot.com/html/pc1.html, make a call, right click to show the controller, you should be able to control the audio in the second tag.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164814

[no longer working on chromium, 2012-10-24 14:38:31]

Wjia, please take a look on the overall design, and let me know if you have any
opinion on the code.

BR,
SX

[wjia(left Chromium), 2012-10-24 22:05:15]

Thanks for adding audio support!

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:281:
scoped_refptr<content::WebRtcAudioRenderer> renderer =
nit: no need content::

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:507:
scoped_refptr<content::WebRtcAudioRenderer>
nit: no need content::

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:516: return new
content::WebRtcAudioRenderer();
ditto.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:155:
scoped_refptr<content::WebRtcAudioRenderer> CreateRemoteAudioRenderer(
nit: no need content::

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:211: // reference to the
capturer object.
repeating comments?

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:215: // Callback to get the
rendered interleaved data.
indent.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:222: virtual void
SetRenderFormat(media::AudioParameters params) = 0;
(const media::AudioParameters& params) ?

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:251: virtual void
SetRenderFormat(media::AudioParameters params) OVERRIDE;
ditto.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:70: }
move above stuff into anonymous namespace.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:223: int
audio_delay_milliseconds) {
indent.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.h (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:29: // Methods called on pipeline
thread ----------------------------------------
RtcAudioRenderer methods will be called on main thread.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:45: // Flag to keep track the
state set by the pipeline.
Is this comment valid?

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:53: private:
redundant "private".

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.cc (right):

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.cc:4: 
Is this a newly added file, or a renamed version of AudioRenderer? Somehow the
history of this file is different from its header file.

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.h (right):

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.h:16: 
nit: remove empty line.

http://codereview.chromium.org/11270012/diff/2001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_ms.cc (right):

http://codereview.chromium.org/11270012/diff/2001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_ms.cc:203: // TODO(wjia): add audio support. See
crbug.com/142988.
nit: remove this TODO.

http://codereview.chromium.org/11270012/diff/2001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_ms.cc:204: return (audio_renderer_);
nit: no need to have parentheses.

[no longer working on chromium, 2012-10-25 10:19:41]

Hi Wei, I think I have already addressed all your comments, please take a second
look.

Andrew, do you have time to help reviewing the media part of this CL?

Thanks,
BR,
SX

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:281:
scoped_refptr<content::WebRtcAudioRenderer> renderer =
On 2012/10/24 22:05:15, wjia wrote:
> nit: no need content::

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:507:
scoped_refptr<content::WebRtcAudioRenderer>
On 2012/10/24 22:05:15, wjia wrote:
> nit: no need content::

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:516: return new
content::WebRtcAudioRenderer();
On 2012/10/24 22:05:15, wjia wrote:
> ditto.

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:155:
scoped_refptr<content::WebRtcAudioRenderer> CreateRemoteAudioRenderer(
On 2012/10/24 22:05:15, wjia wrote:
> nit: no need content::

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:211: // reference to the
capturer object.
On 2012/10/24 22:05:15, wjia wrote:
> repeating comments?

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:215: // Callback to get the
rendered interleaved data.
On 2012/10/24 22:05:15, wjia wrote:
> indent. 

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:222: virtual void
SetRenderFormat(media::AudioParameters params) = 0;
On 2012/10/24 22:05:15, wjia wrote:
> (const media::AudioParameters& params) ?

thanks, done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:251: virtual void
SetRenderFormat(media::AudioParameters params) OVERRIDE;
On 2012/10/24 22:05:15, wjia wrote:
> ditto.

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:70: }
On 2012/10/24 22:05:15, wjia wrote:
> move above stuff into anonymous namespace.

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:223: int
audio_delay_milliseconds) {
On 2012/10/24 22:05:15, wjia wrote:
> indent.

Done.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.h (right):

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:29: // Methods called on pipeline
thread ----------------------------------------
On 2012/10/24 22:05:15, wjia wrote:
> RtcAudioRenderer methods will be called on main thread.

Good to know, thanks. I just realized that those methods can also be called by
WebRtcAudioDeviceImpl, so I changed the comment.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:45: // Flag to keep track the
state set by the pipeline.
On 2012/10/24 22:05:15, wjia wrote:
> Is this comment valid?

No. It is changed.

http://codereview.chromium.org/11270012/diff/2001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:53: private:
On 2012/10/24 22:05:15, wjia wrote:
> redundant "private".

Done.

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.cc (right):

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.cc:4: 
On 2012/10/24 22:05:15, wjia wrote:
> Is this a newly added file, or a renamed version of AudioRenderer? Somehow the
> history of this file is different from its header file.

Yes, it is weird. It is a new file. I copied the audio_renderer.h/cc and
modified them to rtc_audio_renderer.h/cc, for some reason git cl keeps the
history of audio_renderer.cc. I am not sure how to remove the wrong history.

Another thing here is the naming, it is unsure if rtc_audio_renderer is a good
name, since I use webrtc_audio_renderer for its implementation in
content/renderer/media. How do you think if I change the interface file to
webrtc_audio_renderer, and use webrtc_audio_renderer_impl as the implementation
file name?
Or any other suggestion?

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.h (right):

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.h:16: 
On 2012/10/24 22:05:15, wjia wrote:
> nit: remove empty line.

Done.

http://codereview.chromium.org/11270012/diff/2001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_ms.cc (right):

http://codereview.chromium.org/11270012/diff/2001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_ms.cc:203: // TODO(wjia): add audio support. See
crbug.com/142988.
On 2012/10/24 22:05:15, wjia wrote:
> nit: remove this TODO.

Done.

http://codereview.chromium.org/11270012/diff/2001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_ms.cc:204: return (audio_renderer_);
On 2012/10/24 22:05:15, wjia wrote:
> nit: no need to have parentheses. 

I changed it to be the same style as in hasVideo(), hope it is fine.

[no longer working on chromium, 2012-10-25 10:21:26]

http://codereview.chromium.org/11270012/diff/10004/content/renderer/media/web...
File content/renderer/media/webrtc_audio_renderer.cc (right):

http://codereview.chromium.org/11270012/diff/10004/content/renderer/media/web...
content/renderer/media/webrtc_audio_renderer.cc:235: int
audio_delay_milliseconds) {
the review tool seems to show the wrong indentation here, but if opening the
diff file from "Riew", you can see the indentation is correct.

[wjia(left Chromium), 2012-10-25 14:15:04]

Nice work! LGTM.

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.cc (right):

http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.cc:4: 
On 2012/10/25 10:19:41, xians1 wrote:
> On 2012/10/24 22:05:15, wjia wrote:
> > Is this a newly added file, or a renamed version of AudioRenderer? Somehow
the
> > history of this file is different from its header file.
> 
> Yes, it is weird. It is a new file. I copied the audio_renderer.h/cc and
> modified them to rtc_audio_renderer.h/cc, for some reason git cl keeps the
> history of audio_renderer.cc. I am not sure how to remove the wrong history.
> 
> Another thing here is the naming, it is unsure if rtc_audio_renderer is a good
> name, since I use webrtc_audio_renderer for its implementation in
> content/renderer/media. How do you think if I change the interface file to
> webrtc_audio_renderer, and use webrtc_audio_renderer_impl as the
implementation
> file name?
> Or any other suggestion?

From WebMediaPlayer point of view, it's audio_player, not just audio_renderer.
Some people think "pause" will pause the source of media element as well. I am
trying to sort out what's really desired. 

For now, it's ok to keep current name. We can refactor later.

[no longer working on chromium, 2012-10-25 15:49:04]

On 2012/10/25 14:15:04, wjia wrote:
> Nice work! LGTM.
> 
>
http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
> File media/base/rtc_audio_renderer.cc (right):
> 
>
http://codereview.chromium.org/11270012/diff/2001/media/base/rtc_audio_render...
> media/base/rtc_audio_renderer.cc:4: 
> On 2012/10/25 10:19:41, xians1 wrote:
> > On 2012/10/24 22:05:15, wjia wrote:
> > > Is this a newly added file, or a renamed version of AudioRenderer? Somehow
> the
> > > history of this file is different from its header file.
> > 
> > Yes, it is weird. It is a new file. I copied the audio_renderer.h/cc and
> > modified them to rtc_audio_renderer.h/cc, for some reason git cl keeps the
> > history of audio_renderer.cc. I am not sure how to remove the wrong history.
> > 
> > Another thing here is the naming, it is unsure if rtc_audio_renderer is a
good
> > name, since I use webrtc_audio_renderer for its implementation in
> > content/renderer/media. How do you think if I change the interface file to
> > webrtc_audio_renderer, and use webrtc_audio_renderer_impl as the
> implementation
> > file name?
> > Or any other suggestion?
> 
> From WebMediaPlayer point of view, it's audio_player, not just audio_renderer.
> Some people think "pause" will pause the source of media element as well. I am
> trying to sort out what's really desired. 
> 
> For now, it's ok to keep current name. We can refactor later.

SGTM on the future refactoring.
Regarding the "pause", the current approach used by this CL is to continue to
consume the data in WebRtc but not playout the sound, this is to avoid the
buffering inside WebRtc and the delay when the users resume the playing. This
will not be right approach if we should pause the source of media element.

[scherkus (not reviewing), 2012-10-25 17:47:38]

few high level comments + nits

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:219: virtual void
SetRenderFormat(const media::AudioParameters& params) = 0;
fix indent

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:268:
scoped_refptr<WebRtcAudioRenderer> renderer =  new WebRtcAudioRenderer();
remove extra space after =

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:343: 
remove extra blank line

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:454: 
remove extra blank line

http://codereview.chromium.org/11270012/diff/4019/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.h (right):

http://codereview.chromium.org/11270012/diff/4019/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.h:13: class MEDIA_EXPORT RtcAudioRenderer
I don't see any reason why this has to be in ./media (there's no code inside of
./media that makes use of this interface). Can you move this beside
VideoFrameProvider under ./webkit/media?

Also the name isn't very descriptive. It's returned by MediaStreamClient and is
used by the MediaStream version of WebMediaPlayer -- perhaps have the word
"Stream" in there as opposed to "RTC"? (media streams don't necessarily go over
RTC...)

http://codereview.chromium.org/11270012/diff/4019/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.h:14: : public
base::RefCountedThreadSafe<RtcAudioRenderer> {
what necessitates this being refcounted?

http://codereview.chromium.org/11270012/diff/4019/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_ms.cc (right):

http://codereview.chromium.org/11270012/diff/4019/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_ms.cc:139: audio_renderer_->Pause();
nit: add blank line after this (consistent w/ play() code)

[no longer working on chromium, 2012-10-26 09:35:29]

Thanks Andrew for the comments.
I think I have already addressed all of them, please take a second look.

Brett, can I have your owner stamp for 
content/content_renderer.gypi
webkit/support/test_media_stream_client.h/.cc

Thanks,

BR,
SX

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:219: virtual void
SetRenderFormat(const media::AudioParameters& params) = 0;
On 2012/10/25 17:47:39, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:268:
scoped_refptr<WebRtcAudioRenderer> renderer =  new WebRtcAudioRenderer();
On 2012/10/25 17:47:39, scherkus wrote:
> remove extra space after =

Done.

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:343: 
On 2012/10/25 17:47:39, scherkus wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/11270012/diff/4019/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:454: 
On 2012/10/25 17:47:39, scherkus wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/11270012/diff/4019/media/base/rtc_audio_render...
File media/base/rtc_audio_renderer.h (right):

http://codereview.chromium.org/11270012/diff/4019/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.h:13: class MEDIA_EXPORT RtcAudioRenderer
On 2012/10/25 17:47:39, scherkus wrote:
> I don't see any reason why this has to be in ./media (there's no code inside
of
> ./media that makes use of this interface). Can you move this beside
> VideoFrameProvider under ./webkit/media?
> 
> Also the name isn't very descriptive. It's returned by MediaStreamClient and
is
> used by the MediaStream version of WebMediaPlayer -- perhaps have the word
> "Stream" in there as opposed to "RTC"? (media streams don't necessarily go
over
> RTC...)

Good idea, I was also not sure if the location and name of this interface are
correct. I will move it to webkit/media, and change the name to
media_stream_audio_renderer.h

http://codereview.chromium.org/11270012/diff/4019/media/base/rtc_audio_render...
media/base/rtc_audio_renderer.h:14: : public
base::RefCountedThreadSafe<RtcAudioRenderer> {
On 2012/10/25 17:47:39, scherkus wrote:
> what necessitates this being refcounted?

This needs to be ref-counted because it is used by both WebAudioDeviceImpl and
WebMediaPlayerMS, and it is unsure who will go away first.
And in the future, we also need to keep a ref of the object in the AudioTrack,
for features like connecting one remote AudioTrack's rendered data to a local
AudioTrack's capture source, and send it out via peerconnection.

http://codereview.chromium.org/11270012/diff/4019/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_ms.cc (right):

http://codereview.chromium.org/11270012/diff/4019/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_ms.cc:139: audio_renderer_->Pause();
On 2012/10/25 17:47:39, scherkus wrote:
> nit: add blank line after this (consistent w/ play() code)

Done.

[scherkus (not reviewing), 2012-10-26 17:28:22]

lgtm

lgtm w/ nit and q

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:279: } else {
nit: remove else

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:693: renderer_->Pause();
which thread is this being called on?

is seems a bit odd to call the same methods that WMPMS calls and potentially on
a different thread

does Start/StopPlayout() really mean the same thing as WMPMS::play/pause()?

[no longer working on chromium, 2012-10-29 15:01:35]

Thanks Andrew, see comments inline.

Brett might be busy, Avi, can I have your owner stamp for
content/content_renderer.gypi?
tkent, owner stamp for webkit/support?

Thanks,

SX

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:279: } else {
On 2012/10/26 17:28:22, scherkus wrote:
> nit: remove else

Done.

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/11270012/diff/26006/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:693: renderer_->Pause();
On 2012/10/26 17:28:22, scherkus wrote:
> which thread is this being called on?
> 
> is seems a bit odd to call the same methods that WMPMS calls and potentially
on
> a different thread
> 

It is called on libjingle thread.

> does Start/StopPlayout() really mean the same thing as WMPMS::play/pause()?

Yes, they are. WMPMS gets the command from the UI, and Start/StopPlayout gets
commands from libjingle which also controls the webrtc.
In the future Start/StopPlayout should not trigger Play()/Pause() after we sort
out the correct behavior on libjingle, but it is the case for now.

[Avi (use Gerrit), 2012-10-29 15:18:46]

gypi stamp lgtm

[jamesr, 2012-10-29 19:19:39]

webkit lgtm

[commit-bot: I haz the power, 2012-10-29 19:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/11270012/34001

[commit-bot: I haz the power, 2012-10-29 20:15:57]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-10-29 22:16:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/11270012/34001

[commit-bot: I haz the power, 2012-10-30 01:37:38]

Change committed as 164814

[tommi (sloooow) - chröme, 2012-11-13 11:08:30]

late drive-by.  Since this is refactoring the WebRTC audio code, it would have
been good to at least cc Henrik.

https://chromiumcodereview.appspot.com/11270012/diff/34001/content/renderer/m...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://chromiumcodereview.appspot.com/11270012/diff/34001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.h:128:
WebRtcAudioDeviceImpl* GetWebRtcAudioDevice();
I don't think it's a good idea to hand out raw pointers to a ref counted object.
 Instead return scoped_reptr.

https://chromiumcodereview.appspot.com/11270012/diff/34001/content/renderer/m...
File content/renderer/media/webrtc_audio_renderer.h (right):

https://chromiumcodereview.appspot.com/11270012/diff/34001/content/renderer/m...
content/renderer/media/webrtc_audio_renderer.h:60: WebRtcAudioRendererSource*
source_;
where does the ownership lie?