Pass the elapsed time from VoE to WebRtcAudioRenderer as the current time for the audio/video eleme…

Pass the elapsed time from VoE to WebRtcAudioRenderer as the current time for the audio/video element.

BUG=367161
and webrtc 3111

R=xians@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276673

[no longer working on chromium, 2014-06-10 16:21:39]

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:183:
renderer_->SetCurrentRenderTime(
this is on audio render thread.

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_renderer.cc:422:
DCHECK(thread_checker_.CalledOnValidThread());
it will hit the dcheck here.

[Ronghua Wu (Left Chromium), 2014-06-10 17:01:07]

PTAL

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:183:
renderer_->SetCurrentRenderTime(
On 2014/06/10 16:21:38, xians1 wrote:
> this is on audio render thread.

Done.

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/328493003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_renderer.cc:422:
DCHECK(thread_checker_.CalledOnValidThread());
On 2014/06/10 16:21:39, xians1 wrote:
> it will hit the dcheck here.

Done.

[Ronghua Wu (Left Chromium), 2014-06-10 21:48:22]

One more change

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer.cc:422: lock_.AssertAcquired();
this is called by
WebRtcAudioRenderer::Render ->
WebRtcAudioRenderer::SourceCallback ->
WebRtcAudioDeviceImpl::RenderData ->
WebRtcAudioRenderer::SetCurrentRenderTime

The lock should have already been acquired at this point. We can't acquire it
again.

[Ronghua Wu (Left Chromium), 2014-06-11 16:35:34]

ping for another look

[no longer working on chromium, 2014-06-11 16:55:01]

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:183:
renderer_->SetCurrentRenderTime(
Thanks for the explain. After giving it a second though, this might not the
right way to do. The first problem is that it depends on a circular relationship
between the renderer and the WebRtcAudioDeviceImpl, the second problem is what
you described about the locking.

I would suggest the following alternative
Change the RenderData() interface to pass a pointer of base::TimeDelta here, and
assign value to the variable.

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer.cc:422: lock_.AssertAcquired();
On 2014/06/10 21:48:22, Ronghua Wu wrote:
> this is called by
> WebRtcAudioRenderer::Render ->
> WebRtcAudioRenderer::SourceCallback ->
> WebRtcAudioDeviceImpl::RenderData ->
> WebRtcAudioRenderer::SetCurrentRenderTime
> 
> The lock should have already been acquired at this point. We can't acquire it
> again.

Please use the interface and avoid this setter instead.

[Ronghua Wu (Left Chromium), 2014-06-11 17:39:15]

PTAL

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:183:
renderer_->SetCurrentRenderTime(
On 2014/06/11 16:55:01, xians1 wrote:
> Thanks for the explain. After giving it a second though, this might not the
> right way to do. The first problem is that it depends on a circular
relationship
> between the renderer and the WebRtcAudioDeviceImpl, the second problem is what
> you described about the locking.
> 
> I would suggest the following alternative
> Change the RenderData() interface to pass a pointer of base::TimeDelta here,
and
> assign value to the variable.
> 

Great suggestion. Done.

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/328493003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer.cc:422: lock_.AssertAcquired();
On 2014/06/11 16:55:01, xians1 wrote:
> On 2014/06/10 21:48:22, Ronghua Wu wrote:
> > this is called by
> > WebRtcAudioRenderer::Render ->
> > WebRtcAudioRenderer::SourceCallback ->
> > WebRtcAudioDeviceImpl::RenderData ->
> > WebRtcAudioRenderer::SetCurrentRenderTime
> > 
> > The lock should have already been acquired at this point. We can't acquire
it
> > again.
> 
> Please use the interface and avoid this setter instead.

Done.

[no longer working on chromium, 2014-06-11 18:37:50]

lgtm, thanks.

[Ronghua Wu (Left Chromium), 2014-06-12 14:29:59]

Committed patchset #4 manually as r276673 (presubmit successful).