Fold AudioRenderer::Stop() into the dtor.

Fold AudioRenderer::Stop() into the dtor.

BUG=349211
TEST=Existing tests pass.

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

[xhwang, 2014-07-18 17:09:40]

All unit tests pass. I need to test more on real videos. PTAL

[scherkus (not reviewing), 2014-07-18 17:23:31]

https://codereview.chromium.org/407583002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/407583002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:524: audio_renderer_.reset();
is doing it here not enough?

perhaps this should be a DCHECK instead

https://codereview.chromium.org/407583002/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/407583002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.cc:77: base::AutoLock auto_lock(lock_);
this is a bit of a code smell where we should hopefully not have to worry about
synchronization issues during shutdown ... but with the current code *I think*
it's possible for Render() to be running on the audio thread while we enter the
dtor

in this case I wonder whether we're better off with either:
  a) calling sink_->Stop() first, or...
  b) requiring pipeline to call StopRendering() prior to shutdown (we can DCHECK
here)

it'd be good to audit whether sink_->Stop() will block and guarantee Render()
won't be called

https://codereview.chromium.org/407583002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.cc:79: time_cb_.Reset();
I know you copied this code over, but there really isn't a need to reset these
callbacks

[xhwang, 2014-07-21 23:31:33]

comments addressed

[xhwang, 2014-07-21 23:32:40]

Comments addressed.

dalecurtis: I added you to the reviewer list to help double check whether my
understanding of the sink_->Stop() code is correct.

https://codereview.chromium.org/407583002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/407583002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:524: audio_renderer_.reset();
On 2014/07/18 17:23:30, scherkus wrote:
> is doing it here not enough?
> 
> perhaps this should be a DCHECK instead

Done.

https://codereview.chromium.org/407583002/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/407583002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.cc:77: base::AutoLock auto_lock(lock_);
On 2014/07/18 17:23:31, scherkus wrote:
> this is a bit of a code smell where we should hopefully not have to worry
about
> synchronization issues during shutdown ... but with the current code *I think*
> it's possible for Render() to be running on the audio thread while we enter
the
> dtor
> 
> in this case I wonder whether we're better off with either:
>   a) calling sink_->Stop() first, or...
>   b) requiring pipeline to call StopRendering() prior to shutdown (we can
DCHECK
> here)

I chose (a) because it's simpler. 

StopRendering() is also a sync call, it acquires a lock first, then unlock it
before calling sink_->Pause(). I don't see much difference between calling
StopRendering() and calling sink_->Stop() directly for our purpose.

> it'd be good to audit whether sink_->Stop() will block and guarantee Render()
> won't be called

I checked WebAudioSourceProviderImpl() and sink_->Stop() does wait for Render()
to complete. Also checked with dalecurtis@ that this should be generally true
for all AudioRendererSinks.

https://codereview.chromium.org/407583002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.cc:79: time_cb_.Reset();
On 2014/07/18 17:23:31, scherkus wrote:
> I know you copied this code over, but there really isn't a need to reset these
> callbacks

Done.

[scherkus (not reviewing), 2014-07-21 23:38:20]

lgtm w/ nit

will defer to dalecurtis@ if he's got comments

https://codereview.chromium.org/407583002/diff/20001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/407583002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:74: // If Render() is in process, this call
will wait for Render() to finish.
s/process/progress/?

https://codereview.chromium.org/407583002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:77: sink_ = NULL;
is this required to force the ref-count to zero? if not I'd just remove the call
and reformat the code

also sink_ should never be NULL (it's set via ctor and never touched again)

// If Render() is in ...
sink_->Stop();

[xhwang, 2014-07-21 23:40:33]

rebase only

[xhwang, 2014-07-21 23:53:57]

comments addressed

[xhwang, 2014-07-21 23:56:33]

https://codereview.chromium.org/407583002/diff/20001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/407583002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:74: // If Render() is in process, this call
will wait for Render() to finish.
On 2014/07/21 23:38:20, scherkus wrote:
> s/process/progress/?

Done.

https://codereview.chromium.org/407583002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:77: sink_ = NULL;
On 2014/07/21 23:38:20, scherkus wrote:
> is this required to force the ref-count to zero? 

Probably not needed. The ref-count will be dropped when sink_ is dtor'ed
anyways.

> if not I'd just remove the call
> and reformat the code
> 
> also sink_ should never be NULL (it's set via ctor and never touched again)
> 
> // If Render() is in ...
> sink_->Stop();

Good point. Done.

[DaleCurtis, 2014-07-22 00:15:48]

lgtm

https://codereview.chromium.org/407583002/diff/50008/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/407583002/diff/50008/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:75: sink_->Stop();
Correct, once this returns no further calls to the render callback should
happen.

[xhwang, 2014-07-22 00:34:25]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-22 00:40:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/407583002/50008

[commit-bot: I haz the power, 2014-07-22 03:39:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-22 03:41:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 03:41:06]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[xhwang, 2014-07-22 04:45:45]

rebase only

[xhwang, 2014-07-22 04:47:12]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-22 04:48:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/407583002/70001

[xhwang, 2014-07-22 06:08:48]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-22 06:09:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/407583002/90001

[commit-bot: I haz the power, 2014-07-22 09:58:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-22 11:02:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 11:02:46]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[xhwang, 2014-07-22 16:29:38]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-22 16:30:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/407583002/90001

[commit-bot: I haz the power, 2014-07-22 18:27:14]

Change committed as 284734