Rewrite media::Pipeline state transition machinery.

Rewrite media::Pipeline state transition machinery.

Previously we had three separate state machine handlers: OnFilterInitialize(), OnFilterStateTransition(), and OnTeardownStateTransition(). This change combines them into a single state machine handler OnStateTransition().

As part of the cleanup redundant states kEnded and kError were eliminated.

BUG=138583
TEST=everything

[scherkus (not reviewing), 2012-07-28 02:26:07]

*boom*

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (left):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#oldcode107
media/base/pipeline.cc:107: CHECK(running_) << "Media pipeline isn't running";
it's now always valid to call Stop() -- this avoids clients having to track
whether it's OK to stop or not (ie WMPI::started_ variable)

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode389
media/base/pipeline.cc:389: PlaybackRateChangedTask(GetPlaybackRate());
here's the new method for handling rate/volume changes during non-playback
states

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode408
media/base/pipeline.cc:408: video_decoder_->PrepareForShutdownHack();
this might actually go away now that we don't Pause+Flush before Stop

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode524
media/base/pipeline.cc:524: base::Closure done_cb_no_status = base::Bind(
now that there's only one entry point for state transitions (OST()) we can
technically drop this bind-ing business + cb args for DoPause/Flush/Seek/etc...

in my previous CL I liked having that CB passed in but now I don't care as much

WDYT?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode537
media/base/pipeline.cc:537: NOTREACHED() << "kCreated";
kCreated, kStarted, kStopped are non transitioning states, hence we should never
be in this method during those states

right now they have NOTREACHED() today but I can replace them with a DCHECK at
the top of this method for IsTransitioning() or something like that

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode655
media/base/pipeline.cc:655: if (!start_cb_.is_null()) {
put your thinking caps on for this one + look at my Stop_During* / Error_During*
tests!!!

if I call Stop() while Start()'ing -- which callbacks should be executed?! Right
now I execute all pending ones and if there are no pending ones, error_cb

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode697
media/base/pipeline.cc:697: DoStateTransition(PIPELINE_OK);
technically I think I can SetState(kStopping) here instead of having that bit of
code inside of DST() that inspects for stop_cb_

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h
File media/base/pipeline.h (left):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#oldcode248
media/base/pipeline.h:248: kEnded,
kEnded wasn't actually used so it's been folded into kStarted

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#oldcode251
media/base/pipeline.h:251: kError,
kError has been replaced by kStopped && status_ != PIPELINE_OK

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#oldcode256
media/base/pipeline.h:256: void ResetState();
This was only called via Stop() path but not SetError() path -- madness!

We now set stuff in ctor and be done with it -- no more resetting of variables

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#oldcode465
media/base/pipeline.h:465: bool playback_rate_change_pending_;
I replaced this + set volume pending-ness with a simpler policy: before we begin
playback always set the rate + volume to the current value

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (left):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:103: EXPECT_CALL(*mocks_->audio_renderer(),
Pause(_))
Note lack of Pause/Flush prior to Stop!

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:341: TEST_F(PipelineTest, NeverInitializes) {
this test hangs with the new pipeline because inside StopTask() we see that
IsTransitioning() is true, so we don't forcibly try to tear things down

I'm fine with that because I don't like living in a world where we have to deal
with cb's that might never execute or even worse execute after we've stopped +
shut everything down

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:718: TEST_F(PipelineTest, ErrorDuringSeek) {
test moved up above

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:259: EXPECT_CALL(*mocks_->audio_renderer(),
SetVolume(_));
note new calls to SPR/SV()

[scherkus (not reviewing), 2012-07-28 02:31:47]

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode570
media/base/pipeline.cc:570: has_audio_ = !!audio_renderer_ && !audio_disabled_;
hmm... this will actually cause us to not render the volume button for busted
sound cards...

[Ami GONE FROM CHROMIUM, 2012-07-30 04:06:21]

I love this CL, but I have two largish comments on it.  Fundamentally, I'm
afraid of a CL this big that doesn't also address some of the other weaknesses
of Pipeline.

Most importantly, I'd like to see the reliance on locks and (overly-flexible)
trampolining removed.  I believe the actual ways Pipeline is called are much
simpler than the code allows for now, and I think that by locking them down
(with thread assertions instead of AutoLock's) we'll gain a lot of confidence in
this change (and in Pipeline!).  

Secondarily, I think the state machine can be simplified significantly, by using
RunIn{Series,Parallel} more.

For more on both of these concerns, see specific comments below.

(I only skimmed the test change, but it looks fine to me).

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode73
media/base/pipeline.cc:73: running_(false),
I believe it is the case that:
running_ == state_ > kCreated && state_ < kStopped
If that's the case can you drop running_?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode93
media/base/pipeline.cc:93: base::AutoLock auto_lock(lock_);
If you think you need a lock in a dtor, you're gonna have a bad time.

Replace w/ assertion about thread of deletion?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode107
media/base/pipeline.cc:107: base::AutoLock auto_lock(lock_);
replace w/ assertion about running on render thread?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode117
media/base/pipeline.cc:117: base::AutoLock auto_lock(lock_);
ditto to asserting on render thread (lock is not otherwise needed here).

Actually instead of commenting in each method, lemme say: I think all the public
methods of this class are called on the render thread.  Let the ctor capture
that (lifecycle_proxy_ = MessageLoopProxy::current()) and then the public
methods can assert being run on that thread.

Hopefully all the locks can be dropped and replaced with annotations on the
members of the class about which members can be accessed on which thread.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode157
media/base/pipeline.cc:157: if (running_) {
couldn't this be an early-return at the top of the function?  
Ditto for other methods that inspect running_.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode248
media/base/pipeline.cc:248: state_ = next_state;
can you assert a correct thread or lock is held?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode366
media/base/pipeline.cc:366: void Pipeline::DoFlush(const base::Closure& done_cb)
{
Is it crazy to still have this around?
Is it not already possible to collapse the pause/flush/seek/play dance into a
single pile of RunInSeries / RunInParallel?  That should hopefully allow you to
drop a further number of states!

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode408
media/base/pipeline.cc:408: video_decoder_->PrepareForShutdownHack();
On 2012/07/28 02:26:07, scherkus wrote:
> this might actually go away now that we don't Pause+Flush before Stop

Yes, it can!

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode510
media/base/pipeline.cc:510: // Force-post a task on state transitions since to
avoid reentrancy between
s/since/

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode523
media/base/pipeline.cc:523: PipelineStatusCB done_cb =
base::Bind(&Pipeline::OnStateTransition, this);
Can move south of the error-handling case.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode524
media/base/pipeline.cc:524: base::Closure done_cb_no_status = base::Bind(
On 2012/07/28 02:26:07, scherkus wrote:
> now that there's only one entry point for state transitions (OST()) we can
> technically drop this bind-ing business + cb args for
DoPause/Flush/Seek/etc...
> 
> in my previous CL I liked having that CB passed in but now I don't care as
much
> 
> WDYT?

See above; I like the CBing, but I think you can simplify the pause/flush/seek
story a bunch.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode524
media/base/pipeline.cc:524: base::Closure done_cb_no_status = base::Bind(
d_c_no_status is a funny name for a thing that has a status_ in it...

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode527
media/base/pipeline.cc:527: // Check if we need to stop due to an error or due
to |stop_cb_| being set.
I'm pretty sure you dropped the VideoDecoder fallback code (where if the HW
decoder fails to initialize b/c of unsupported profile or resource exhaustion,
Pipeline will fall back to SW decoder).  See l.619 in the "before" of this CL.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode537
media/base/pipeline.cc:537: NOTREACHED() << "kCreated";
On 2012/07/28 02:26:07, scherkus wrote:
> kCreated, kStarted, kStopped are non transitioning states, hence we should
never
> be in this method during those states
> 
> right now they have NOTREACHED() today but I can replace them with a DCHECK at
> the top of this method for IsTransitioning() or something like that

IMO best to keep as-is, and drop the default: case below, to catch all states.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode548
media/base/pipeline.cc:548: DoInitAudioDecoder(done_cb);
I'm wondering whether the initialization dance could also be done as a
RunInSeries.

Rationale being that the part of a state machine that is a chain (instead of a
graph) is far simpler to grok and ought not complicate the description of the
state machine.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode571
media/base/pipeline.cc:571: has_video_ = !!video_renderer_;
Is it really worthwhile maintaining members for these?
(has_{audio,video}_)
Why can't they be computed on demand?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode655
media/base/pipeline.cc:655: if (!start_cb_.is_null()) {
On 2012/07/28 02:26:07, scherkus wrote:
> put your thinking caps on for this one + look at my Stop_During* /
Error_During*
> tests!!!
> 
> if I call Stop() while Start()'ing -- which callbacks should be executed?!
Right
> now I execute all pending ones and if there are no pending ones, error_cb

Can you be more assertive about the possibility of more than one of
{start,seek,stop}_cb_ being non-is_null() at once?
In fact, can those all be a single member whose meaning varies depending on
state_ and whose setting always checks that it is_null() beforehand?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode677
media/base/pipeline.cc:677: default:
drop

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode704
media/base/pipeline.cc:704: // Preserve the error code.
s/the/an existing/

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode719
media/base/pipeline.cc:719: // Suppress rate change until we're playing.
Is it right to drop this?  Or does WMPI already cover for us?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode739
media/base/pipeline.cc:739: // Suppress volume change until we're playing.
ditto

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode836
media/base/pipeline.cc:836: if (!demuxer_) {
is this something other than programming error?  Can we promote this to a
DCHECK?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode844
media/base/pipeline.cc:844: void Pipeline::DoInitAudioDecoder(const
PipelineStatusCB& done_cb) {
Is it right that Pipeline still initializes the decoders, even though they are
owned by the renderers now?  Why not have AudioRenderer init AudioDecoder, and
ditto for Video{Renderer,Decoder}?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode856
media/base/pipeline.cc:856:
filter_collection_->SelectAudioDecoder(&pipeline_init_state_->audio_decoder);
This SelectFoo() api is funny in that it doesn't just return its selected
pointer...

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode857
media/base/pipeline.cc:857: if (!pipeline_init_state_->audio_decoder) {
ditto (here and below).  Promote to DCHECK?

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode930
media/base/pipeline.cc:930: video_decoder_ =
pipeline_init_state_->video_decoder;
Drop this member.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode958
media/base/pipeline.cc:958: if (!skip_demuxer_seek)
Is this really important?  What happens if we don't skip the demuxer seek during
the preroll during init?

(put another way, can we make that demuxer seek free and avoid this complexity?)

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h
File media/base/pipeline.h (left):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#oldcode160
media/base/pipeline.h:160: // Returns true if the pipeline has been started and
fully initialized to a
hawt!

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#newcode63
media/base/pipeline.h:63: // Pipeline runs the media pipeline.  Filters are
created and called on the
The sentence starting with "Filters" is a lie.
IWBN to have a description of the thread-story of Pipeline.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#newcode79
media/base/pipeline.h:79: //  kStarted ---------> kPausing
The fact that most states have a single ingress edge means the names are
interesting.  For example, kStarting implies "pre-rolled/seeked", kFlushing &
kSeeking imply "Paused", etc.

All this is to say I think s/kStarted/kPlaying/ is an improvement to
consistency.  (either verbify everything or adjective everything; don't mix)

If you agree w/ this, then maybe also s/kCreated/kInitPipeline/

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#newcode89
media/base/pipeline.h:89: // pipeline will transition to a stopped state.
s/stopped/(terminal) stopped/

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.h#newcode245
media/base/pipeline.h:245: void DoStateTransition(PipelineStatus status);
These names are not sufficiently self-explanatory that they don't need
descriptions.

http://codereview.chromium.org/10828045/diff/7/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10828045/diff/7/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:118:
message_loop_factory_(message_loop_factory),
kill started_ in the .h (and the TODO above it)

[acolwell GONE FROM CHROMIUM, 2012-07-30 18:33:10]

In general I agree with most of Ami's comments. I would like to see this split
up into a set of smaller CL's though. I think the CLs could fall into 5 general
categories.

1. Lock asserts, removals, and MessageLoop assertions to clarify/narrow who can
call what & when.
2. Converting the initialization states into one state and a callback sequence.
3. Converting the pause/flush/seek states into one state and a callback
sequence.
4. Allowing Stop() to be called at any time.
5. General cleanup of member variables that are redundant or no longer needed.

I think having smaller more focused CLs will make it easier to keep track
everything that is changing. It will also make me less worried about bugs
getting introduced especially in the non-happy path.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode32
media/base/pipeline.cc:32:
PipelineStatusNotification::PipelineStatusNotification()
Shouldn't this be in its own file? It isn't even used by this class. It appears
to only be used by player_wtl & player_x11.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode117
media/base/pipeline.cc:117: base::AutoLock auto_lock(lock_);
On 2012/07/30 04:06:21, Ami Fischman wrote:
> ditto to asserting on render thread (lock is not otherwise needed here).
> 
> Actually instead of commenting in each method, lemme say: I think all the
public
> methods of this class are called on the render thread.  Let the ctor capture
> that (lifecycle_proxy_ = MessageLoopProxy::current()) and then the public
> methods can assert being run on that thread.
> 
> Hopefully all the locks can be dropped and replaced with annotations on the
> members of the class about which members can be accessed on which thread.
Most of the GetXXX methods need locks because they are updated on the Pipeline
thread and read from the render thread. Unless we change how these are accessed,
I don't see that going away.

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode157
media/base/pipeline.cc:157: if (running_) {
On 2012/07/30 04:06:21, Ami Fischman wrote:
> couldn't this be an early-return at the top of the function?  
> Ditto for other methods that inspect running_.
The running_ check is here because it needs to be protected by the lock because
it is updated by 2 threads. It's set to true on render thread in Start() and set
to false on Pipeline thread in Pipeline::DoStateTransition().

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (left):

http://codereview.chromium.org/10828045/diff/7/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:341: TEST_F(PipelineTest, NeverInitializes) {
On 2012/07/28 02:26:07, scherkus wrote:
> this test hangs with the new pipeline because inside StopTask() we see that
> IsTransitioning() is true, so we don't forcibly try to tear things down
> 
> I'm fine with that because I don't like living in a world where we have to
deal
> with cb's that might never execute or even worse execute after we've stopped +
> shut everything down
Doesn't this essentially test the case where the pipeline gets shutdown before
the Demuxer was able to successfully connect to a server or the connection is
REALLY slow? If so don't we want to make sure that we can shutdown properly?

[scherkus (not reviewing), 2012-08-07 03:00:43]

As discussed last week I'm going to close this issue and break it into a few
CLs:
  - Fix flaky DCHECKs (done, see http://codereview.chromium.org/10830146/)
  - Merge states (next up!)
  - Move FFmpegDemuxer off of PipelineThread
  - Always execute Stop() for teardown
  - Misc cleanup (remove useless methods, add thread annotations, etc...)