Extract media::Clock::IsPlaying() into media::Pipeline::ClockState enum.

Extract media::Clock::IsPlaying() into media::Pipeline::ClockState enum.

Instead of using both Clock::IsPlaying() and waiting_for_clock_update_,
use a three-state representation so it's easier to determine the actual
state of both audio rendering and the clock when Pipeline is in its
playing state. Introduce some helpers to deal with transitioning between states.

This is needed for the upcoming BufferingState changes as Pipeline now
handles underflow/preroll.

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

[acolwell GONE FROM CHROMIUM, 2014-05-20 22:30:35]

lgtm

[scherkus (not reviewing), 2014-05-21 00:08:52]

can you take another look?

there were some corner-case DCHECKs that popped up on try bots that I fixed by
adding a PauseClockAndStopRendering_Locked() helper to make code consistent in
how we pause/stop everything

also added Clock::IsPlaying() back in because android MediaSourcePlayer uses it
and I don't feel like mucking about in that code just yet

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:1155: INSTANTIATE_TEARDOWN_TEST(Stop, Pausing);
these tests were removed because they don't make a lot of sense now that
StopRendering() is a synchronous call (they were hitting reentrancy issues since
MockAudioRenderer has expectations that call public Pipeline APIs ... which
isn't really the API contract Pipeline has)

[acolwell GONE FROM CHROMIUM, 2014-05-22 01:28:21]

lgtm

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

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:965: break;
nit: return instead?

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:1155: INSTANTIATE_TEARDOWN_TEST(Stop, Pausing);
On 2014/05/21 00:08:52, scherkus wrote:
> these tests were removed because they don't make a lot of sense now that
> StopRendering() is a synchronous call (they were hitting reentrancy issues
since
> MockAudioRenderer has expectations that call public Pipeline APIs ... which
> isn't really the API contract Pipeline has)

Could this be causing other issues? I wonder if we should have versions of
RunClosure<> and RunCallback<> that actutally PostTask() so the pipeline runs in
the context it is expected to.

[scherkus (not reviewing), 2014-05-22 17:23:40]

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

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:965: break;
On 2014/05/22 01:28:21, acolwell wrote:
> nit: return instead?

Done.

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/294133003/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:1155: INSTANTIATE_TEARDOWN_TEST(Stop, Pausing);
On 2014/05/22 01:28:21, acolwell wrote:
> On 2014/05/21 00:08:52, scherkus wrote:
> > these tests were removed because they don't make a lot of sense now that
> > StopRendering() is a synchronous call (they were hitting reentrancy issues
> since
> > MockAudioRenderer has expectations that call public Pipeline APIs ... which
> > isn't really the API contract Pipeline has)
> 
> Could this be causing other issues? I wonder if we should have versions of
> RunClosure<> and RunCallback<> that actutally PostTask() so the pipeline runs
in
> the context it is expected to.

It'd be a nice to have, but it's not really causing any issues w/ the existing
tests today as calling Pipeline::Stop/SetError() force-post tasks anyway.

The one little snag with these tests was that Stop() has DCHECK(IsRunning())
inside the method, where the implementation of IsRunning() tries to grab a lock
to read a boolean :\

(I had a CL somewhere that got rid of IsRunning() since it's kind of useless of
a check anyway...)

[scherkus (not reviewing), 2014-05-22 17:24:57]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:28:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/294133003/40001

[commit-bot: I haz the power, 2014-05-22 20:24:38]

Change committed as 272301

[RyanS, 2014-05-23 00:41:29]

A revert of this CL has been created in
https://codereview.chromium.org/300433002/ by rschoen@chromium.org.

The reason for reverting is: Speculative revert for failing media_perftests on
desktop (and maybe Android, just hasn't cycled yet)..