Rewrite media::Pipeline state transition machinery and simplify shutdown.

media/base/pipeline.cc

Index: media/base/pipeline.cc
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 7f6efded12e69cd8ed38ae816662fc531c0d41a0..f69eba859c274ec31f7a67bf14808dd7b31dca05 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -18,7 +18,6 @@
 #include "base/synchronization/condition_variable.h"
 #include "media/base/audio_decoder.h"
 #include "media/base/audio_renderer.h"
-#include "media/base/buffers.h"
 #include "media/base/clock.h"
 #include "media/base/filter_collection.h"
 #include "media/base/media_log.h"
@@ -63,31 +62,22 @@ media::PipelineStatus PipelineStatusNotification::status() {
   return status_;
 }
 
-struct Pipeline::PipelineInitState {
-  scoped_refptr<AudioDecoder> audio_decoder;
-};
-
 Pipeline::Pipeline(const scoped_refptr<base::MessageLoopProxy>& message_loop,
                    MediaLog* media_log)
     : message_loop_(message_loop),
       media_log_(media_log),
       running_(false),
-      seek_pending_(false),
-      tearing_down_(false),
-      playback_rate_change_pending_(false),
       did_loading_progress_(false),
       total_bytes_(0),
       natural_size_(0, 0),
       volume_(1.0f),
       playback_rate_(0.0f),
-      pending_playback_rate_(0.0f),
       clock_(new Clock(&base::Time::Now)),
       waiting_for_clock_update_(false),
       status_(PIPELINE_OK),
       has_audio_(false),
       has_video_(false),
       state_(kCreated),
-      seek_timestamp_(kNoTimestamp()),
       audio_ended_(false),
       video_ended_(false),
       audio_disabled_(false),
@@ -98,10 +88,9 @@ Pipeline::Pipeline(const scoped_refptr<base::MessageLoopProxy>& message_loop,
 }
 
 Pipeline::~Pipeline() {
-  base::AutoLock auto_lock(lock_);
   DCHECK(!running_) << "Stop() must complete before destroying object";

[Ami GONE FROM CHROMIUM, 2012/09/06 09:11:34]

Should this assert running on a particular thread?

[scherkus (not reviewing), 2012/09/06 15:33:20]

That would be a nice thing! Here's hoping it doesn't spuriously DCHECK (it
shouldn't as we drain the threads in ~WMPI())

   DCHECK(stop_cb_.is_null());
-  DCHECK(!seek_pending_);
+  DCHECK(seek_cb_.is_null());
 
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::PIPELINE_DESTROYED));
@@ -256,75 +245,65 @@ void Pipeline::SetState(State next_state) {
         "Media.TimeToPipelineStarted", base::Time::Now() - creation_time_);
     creation_time_ = base::Time();
   }
+
+  DVLOG(2) << GetStateString(state_) << " -> " << GetStateString(next_state);
+
   state_ = next_state;
   media_log_->AddEvent(media_log_->CreatePipelineStateChangedEvent(next_state));
 }
 
-bool Pipeline::IsPipelineOk() {
-  base::AutoLock auto_lock(lock_);
-  return status_ == PIPELINE_OK;
-}
+#define RETURN_STRING(state) case state: return #state;
 
-bool Pipeline::IsPipelineSeeking() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  if (!seek_pending_)
-    return false;
-  DCHECK(kSeeking == state_ || kPausing == state_ ||
-         kFlushing == state_ || kStarting == state_)
-      << "Current state : " << state_;
-  return true;
+const char* Pipeline::GetStateString(State state) {
+  switch (state) {
+    RETURN_STRING(kCreated);
+    RETURN_STRING(kInitDemuxer);
+    RETURN_STRING(kInitAudioDecoder);
+    RETURN_STRING(kInitAudioRenderer);
+    RETURN_STRING(kInitVideoRenderer);
+    RETURN_STRING(kInitPrerolling);
+    RETURN_STRING(kSeeking);
+    RETURN_STRING(kStarting);
+    RETURN_STRING(kStarted);
+    RETURN_STRING(kStopping);
+    RETURN_STRING(kStopped);
+  }
+  NOTREACHED();

[Ami GONE FROM CHROMIUM, 2012/09/06 09:11:34]

Is it not possible to omit this and the next lines?

[scherkus (not reviewing), 2012/09/06 15:33:20]

gcc complains about reaching end of non-void function (see my revert your
suggestions to the end of GetNextState() in PS#11)

+  return "INVALID";
 }
 
-void Pipeline::ReportStatus(const PipelineStatusCB& cb, PipelineStatus status) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  if (cb.is_null())
-    return;
-  cb.Run(status);
-  // Prevent double-reporting of errors to clients.
-  if (status != PIPELINE_OK)
-    error_cb_.Reset();
-}
+#undef RETURN_STRING
 
-void Pipeline::FinishSeek() {
+Pipeline::State Pipeline::GetNextState() const {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  seek_timestamp_ = kNoTimestamp();
-  seek_pending_ = false;
-
-  // Execute the seek callback, if present.  Note that this might be the
-  // initial callback passed into Start().
-  ReportStatus(seek_cb_, status_);
-  seek_cb_.Reset();
-}
-
-// static
-bool Pipeline::TransientState(State state) {
-  return state == kPausing ||
-         state == kFlushing ||
-         state == kSeeking ||
-         state == kStarting ||
-         state == kStopping;
-}
-
-// static
-Pipeline::State Pipeline::FindNextState(State current) {
-  // TODO(scherkus): refactor InitializeTask() to make use of this function.
-  if (current == kPausing) {
-    return kFlushing;
-  } else if (current == kFlushing) {
-    // We will always honor Seek() before Stop(). This is based on the
-    // assumption that we never accept Seek() after Stop().
-    DCHECK(IsPipelineSeeking() ||
-           !stop_cb_.is_null() ||
-           tearing_down_);
-    return IsPipelineSeeking() ? kSeeking : kStopping;
-  } else if (current == kSeeking) {
-    return kStarting;
-  } else if (current == kStarting) {
-    return kStarted;
-  } else if (current == kStopping) {
-    return kStopped;
-  } else {
-    return current;
+  DCHECK(stop_cb_.is_null())
+      << "State transitions don't happen when stopping";
+  DCHECK_EQ(status_, PIPELINE_OK)
+      << "State transitions don't happen when there's an error: " << status_;
+
+  switch (state_) {
+    case kCreated:
+      return kInitDemuxer;
+    case kInitDemuxer:
+      return kInitAudioDecoder;
+    case kInitAudioDecoder:
+      return kInitAudioRenderer;
+    case kInitAudioRenderer:
+      return kInitVideoRenderer;
+    case kInitVideoRenderer:
+      return kInitPrerolling;
+    case kInitPrerolling:
+      return kStarting;
+    case kSeeking:
+      return kStarting;
+    case kStarting:
+      return kStarted;
+
+    case kStarted:
+    case kStopping:
+    case kStopped:
+      NOTREACHED() << "State has no transition: " << state_;
+      return state_;
   }
 }
 
@@ -422,31 +401,181 @@ TimeDelta Pipeline::TimeForByteOffset_Locked(int64 byte_offset) const {
   return time_offset;
 }
 
-void Pipeline::DoPause(const PipelineStatusCB& done_cb) {
+void Pipeline::OnStateTransition(PipelineStatus status) {
+  // Force post to process state transitions after current execution frame.
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &Pipeline::StateTransitionTask, this, status));
+}
+
+void Pipeline::StateTransitionTask(PipelineStatus status) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  // Preserve existing abnormal status, otherwise update based on the result of
+  // the previous operation.
+  status_ = (status_ != PIPELINE_OK ? status_ : status);
+
+  // No-op any state transitions if we're stopping.
+  if (state_ == kStopping || state_ == kStopped)
+    return;
+
+  if (status_ != PIPELINE_OK) {
+    ErrorChangedTask(status_);
+    return;
+  }
+
+  // TODO(scherkus): not every state transition uses |pending_callbacks_| today.

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

nit: I think this comment needs to be changed into an action so it is clear what
there is "to do". Perhaps "Make every state transition use
|pending_callbacks_|"?

[scherkus (not reviewing), 2012/09/06 15:33:20]

Done.

+  if (state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) {

[Ami GONE FROM CHROMIUM, 2012/09/06 09:11:34]

I think what you really want is the single-statement DCHECK with a comment
elucidating the (mis-coding) dangers you're guarding against.

[scherkus (not reviewing), 2012/09/06 15:33:20]

The comment I can agree with but the resulting DCHECK won't be fun to debug if
it fires. Coded it up.

+    DCHECK(pending_callbacks_.get())
+        << "pending_callbacks_ should exist when in state " << state_;
+  } else {
+    DCHECK(!pending_callbacks_.get())
+        << "pending_callbacks_ should not be used when in state " << state_;
+  }
+  pending_callbacks_.reset();
+
+  PipelineStatusCB done_cb = base::Bind(&Pipeline::OnStateTransition, this);
+
+  SetState(GetNextState());
+  switch (state_) {
+    case kInitDemuxer:
+      return InitializeDemuxer(done_cb);
+
+    case kInitAudioDecoder:
+      {
+        base::AutoLock l(lock_);
+        // We do not want to start the clock running. We only want to set the
+        // base media time so our timestamp calculations will be correct.
+        clock_->SetTime(demuxer_->GetStartTime(), demuxer_->GetStartTime());
+      }
+      return InitializeAudioDecoder(done_cb);
+
+    case kInitAudioRenderer:
+      return InitializeAudioRenderer(done_cb);
+
+    case kInitVideoRenderer:
+      return InitializeVideoRenderer(done_cb);
+
+    case kInitPrerolling:
+      filter_collection_.reset();
+      {
+        base::AutoLock l(lock_);
+        // TODO(scherkus): |has_audio_| should be true no matter what --
+        // otherwise people with muted/disabled sound cards will make our
+        // default controls look as if every video doesn't contain an audio
+        // track.
+        has_audio_ = audio_renderer_ && !audio_disabled_;
+        has_video_ = video_renderer_;

[scherkus (not reviewing), 2012/09/05 15:19:30]

FYI:
E:\b\build\slave\win\build\src\media\base\pipeline.cc(467) : warning C4800:
'media::VideoRenderer *' : forcing value to bool 'true' or 'false' (performance
warning)

!!video_renderer_ then?

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

I'd prefer video_renderer_ != NULL since that matches most of our other code.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Done.

+      }
+      if (!audio_renderer_ && !video_renderer_) {
+        done_cb.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
+        return;
+      }
+
+      buffering_state_cb_.Run(kHaveMetadata);
+
+      return DoInitialPreroll(done_cb);
+
+    case kStarting:
+      return DoPlay(done_cb);
+
+    case kStarted:
+      {
+        base::AutoLock l(lock_);
+        // We use audio stream to update the clock. So if there is such a
+        // stream, we pause the clock until we receive a valid timestamp.
+        waiting_for_clock_update_ = true;
+        if (!has_audio_) {
+          clock_->SetMaxTime(clock_->Duration());
+          StartClockIfWaitingForTimeUpdate_Locked();
+        }
+      }
+
+      // Fire canplaythrough immediately after playback begins because of
+      // crbug.com/106480.
+      // TODO(vrk): set ready state to HaveFutureData when bug above is fixed.
+      if (status_ == PIPELINE_OK)

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

Based on the DCHECK on line 500, I believe this conditional can be dropped and
the line below moved after the DCHECKS.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Done.

+        buffering_state_cb_.Run(kPrerollCompleted);
+
+      DCHECK(!seek_cb_.is_null());
+      DCHECK_EQ(status_, PIPELINE_OK);
+      return base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
+
+    case kStopping:
+    case kStopped:
+    case kCreated:
+    case kSeeking:
+      NOTREACHED() << "State has no transition: " << state_;
+      return;
+  }
+}
+
+static void SetPlaybackRateForPreroll(
+    const scoped_refptr<AudioRenderer>& audio_renderer,
+    float playback_rate,
+    const base::Closure& done_cb) {
+  audio_renderer->SetPlaybackRate(playback_rate);

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

This doesn't seem right. Won't this cause the audio renderer to get out of sync
with the video renderer and clock.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Removed entirely -- see response in pipeline_unittest.cc

+  done_cb.Run();
+}
+
+void Pipeline::DoInitialPreroll(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(!pending_callbacks_.get());
   SerialRunner::Queue bound_fns;
 
-  if (audio_renderer_)
-    bound_fns.Push(base::Bind(&AudioRenderer::Pause, audio_renderer_));
+  base::TimeDelta seek_timestamp = demuxer_->GetStartTime();
 
-  if (video_renderer_)
-    bound_fns.Push(base::Bind(&VideoRenderer::Pause, video_renderer_));
+  // Preroll renderers.
+  if (audio_renderer_) {
+    bound_fns.Push(base::Bind(
+        &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate()));

[Ami GONE FROM CHROMIUM, 2012/09/06 09:11:34]

Is there some reason you can't set the playbackrate in-line here and avoid the
extra indirection?

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

I don't think this is right. I think you should set the rate on both renderers
like the original code does.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Removed entirely -- see response in pipeline_unittest.cc

+    bound_fns.Push(base::Bind(
+        &AudioRenderer::Preroll, audio_renderer_, seek_timestamp));
+  }
+
+  if (video_renderer_) {
+    bound_fns.Push(base::Bind(
+        &VideoRenderer::Preroll, video_renderer_, seek_timestamp));
+  }
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
 
-void Pipeline::DoFlush(const PipelineStatusCB& done_cb) {
+void Pipeline::DoSeek(
+    base::TimeDelta seek_timestamp,
+    const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(!pending_callbacks_.get());
   SerialRunner::Queue bound_fns;
 
+  // Pause.
   if (audio_renderer_)
-    bound_fns.Push(base::Bind(&AudioRenderer::Flush, audio_renderer_));
+    bound_fns.Push(base::Bind(&AudioRenderer::Pause, audio_renderer_));
+  if (video_renderer_)
+    bound_fns.Push(base::Bind(&VideoRenderer::Pause, video_renderer_));
 
+  // Flush.
+  if (audio_renderer_)
+    bound_fns.Push(base::Bind(&AudioRenderer::Flush, audio_renderer_));
   if (video_renderer_)
     bound_fns.Push(base::Bind(&VideoRenderer::Flush, video_renderer_));
 
+  // Seek demuxer.
+  bound_fns.Push(base::Bind(
+      &Demuxer::Seek, demuxer_, seek_timestamp));
+
+  // Preroll renderers.
+  if (audio_renderer_) {

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

How about creating a helper function for DoSeek() & DoInitialPreroll() to remove
duplicate code. Something like

AddPrerollFunctions(seek_timestamp, &bound_fns);

[scherkus (not reviewing), 2012/09/06 15:33:20]

I think the tiny bit of duplication here is OK -- keeps things looking same-y

+    bound_fns.Push(base::Bind(
+        &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate()));

[Ami GONE FROM CHROMIUM, 2012/09/06 09:11:34]

ditto

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

I don't think you want to bind to a static function here. I think you should
bind to a member function that sets the playback rate and volume on both
renderers. Don't bind the rate because it may change during the pause,flush, and
demuxer seek.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Removed entirely -- see response in pipeline_unittest.cc

+    bound_fns.Push(base::Bind(
+        &AudioRenderer::Preroll, audio_renderer_, seek_timestamp));
+  }
+
+  if (video_renderer_) {
+    bound_fns.Push(base::Bind(
+        &VideoRenderer::Preroll, video_renderer_, seek_timestamp));
+  }
+
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
 
@@ -455,6 +584,9 @@ void Pipeline::DoPlay(const PipelineStatusCB& done_cb) {
   DCHECK(!pending_callbacks_.get());
   SerialRunner::Queue bound_fns;
 
+  PlaybackRateChangedTask(GetPlaybackRate());
+  VolumeChangedTask(GetVolume());
+
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Play, audio_renderer_));
 
@@ -481,6 +613,38 @@ void Pipeline::DoStop(const PipelineStatusCB& done_cb) {
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
 
+void Pipeline::OnStopCompleted(PipelineStatus status) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kStopping);
+  {
+    base::AutoLock l(lock_);
+    running_ = false;
+  }
+
+  SetState(kStopped);
+  pending_callbacks_.reset();
+  filter_collection_.reset();
+  audio_decoder_ = NULL;
+  audio_renderer_ = NULL;
+  video_renderer_ = NULL;
+  demuxer_ = NULL;
+
+  // If we stop during initialization/seeking we want to run |seek_cb_|
+  // followed by |stop_cb_| so we don't leave outstanding callbacks around.
+  if (!seek_cb_.is_null()) {
+    base::ResetAndReturn(&seek_cb_).Run(status_);
+    error_cb_.Reset();
+  }
+  if (!stop_cb_.is_null()) {
+    base::ResetAndReturn(&stop_cb_).Run();
+    error_cb_.Reset();
+  }
+  if (!error_cb_.is_null()) {
+    DCHECK_NE(status_, PIPELINE_OK);
+    base::ResetAndReturn(&error_cb_).Run(status_);
+  }
+}
+
 void Pipeline::AddBufferedByteRange(int64 start, int64 end) {
   DCHECK(IsRunning());
   base::AutoLock auto_lock(lock_);
@@ -520,33 +684,6 @@ void Pipeline::OnVideoRendererEnded() {
 }
 
 // Called from any thread.
-void Pipeline::OnFilterInitialize(PipelineStatus status) {
-  // Continue the initialize task by proceeding to the next stage.
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::InitializeTask, this, status));
-}
-
-// Called from any thread.
-// This method makes the PipelineStatusCB behave like a Closure. It
-// makes it look like a host()->SetError() call followed by a call to
-// OnFilterStateTransition() when errors occur.
-//
-// TODO(scherkus): Revisit this code when SetError() is removed from FilterHost
-// and all the Closures are converted to PipelineStatusCB.
-void Pipeline::OnFilterStateTransition(PipelineStatus status) {
-  if (status != PIPELINE_OK)
-    SetError(status);
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::FilterStateTransitionTask, this));
-}
-
-void Pipeline::OnTeardownStateTransition(PipelineStatus status) {
-  // Ignore any errors during teardown.
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::TeardownStateTransitionTask, this));
-}
-
-// Called from any thread.
 void Pipeline::OnUpdateStatistics(const PipelineStatistics& stats) {
   base::AutoLock auto_lock(lock_);
   statistics_.audio_bytes_decoded += stats.audio_bytes_decoded;
@@ -570,117 +707,9 @@ void Pipeline::StartTask(scoped_ptr<FilterCollection> filter_collection,
   seek_cb_ = seek_cb;
   buffering_state_cb_ = buffering_state_cb;
 
-  // Kick off initialization.
-  pipeline_init_state_.reset(new PipelineInitState());
-
-  SetState(kInitDemuxer);
-  InitializeDemuxer();
-}
-
-// Main initialization method called on the pipeline thread.  This code attempts
-// to use the specified filter factory to build a pipeline.
-// Initialization step performed in this method depends on current state of this
-// object, indicated by |state_|.  After each step of initialization, this
-// object transits to the next stage.  It starts by creating a Demuxer, and then
-// connects the Demuxer's audio stream to an AudioDecoder which is then
-// connected to an AudioRenderer.  If the media has video, then it connects a
-// VideoDecoder to the Demuxer's video stream, and then connects the
-// VideoDecoder to a VideoRenderer.
-//
-// When all required filters have been created and have called their
-// FilterHost's InitializationComplete() method, the pipeline will update its
-// state to kStarted and |init_cb_|, will be executed.
-//
-// TODO(hclam): InitializeTask() is now starting the pipeline asynchronously. It
-// works like a big state change table. If we no longer need to start filters
-// in order, we need to get rid of all the state change.
-void Pipeline::InitializeTask(PipelineStatus last_stage_status) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-
-  if (last_stage_status != PIPELINE_OK) {
-    SetError(last_stage_status);
-    return;
-  }
-
-  // If we have received the stop or error signal, return immediately.
-  if (!stop_cb_.is_null() || state_ == kStopped || !IsPipelineOk())
-    return;
-
-  DCHECK(state_ == kInitDemuxer ||
-         state_ == kInitAudioDecoder ||
-         state_ == kInitAudioRenderer ||
-         state_ == kInitVideoRenderer);
-
-  // Demuxer created, create audio decoder.
-  if (state_ == kInitDemuxer) {
-    SetState(kInitAudioDecoder);
-    // If this method returns false, then there's no audio stream.
-    if (InitializeAudioDecoder(demuxer_))
-      return;
-  }
-
-  // Assuming audio decoder was created, create audio renderer.
-  if (state_ == kInitAudioDecoder) {
-    SetState(kInitAudioRenderer);
-
-    // Returns false if there's no audio stream.
-    if (InitializeAudioRenderer(pipeline_init_state_->audio_decoder)) {
-      base::AutoLock auto_lock(lock_);
-      has_audio_ = true;
-      return;
-    }
-  }
-
-  // Assuming audio renderer was created, create video renderer.
-  if (state_ == kInitAudioRenderer) {
-    SetState(kInitVideoRenderer);
-    scoped_refptr<DemuxerStream> video_stream =
-        demuxer_->GetStream(DemuxerStream::VIDEO);
-    if (InitializeVideoRenderer(video_stream)) {
-      base::AutoLock auto_lock(lock_);
-      has_video_ = true;
-
-      // Get an initial natural size so we have something when we signal
-      // the kHaveMetadata buffering state.
-      natural_size_ = video_stream->video_decoder_config().natural_size();
-      return;
-    }
-  }
-
-  if (state_ == kInitVideoRenderer) {
-    if (!IsPipelineOk() || !(HasAudio() || HasVideo())) {
-      SetError(PIPELINE_ERROR_COULD_NOT_RENDER);
-      return;
-    }
-
-    // Clear initialization state now that we're done.
-    filter_collection_.reset();
-    pipeline_init_state_.reset();
-
-    // Initialization was successful, we are now considered paused, so it's safe
-    // to set the initial playback rate and volume.
-    PlaybackRateChangedTask(GetPlaybackRate());
-    VolumeChangedTask(GetVolume());
-
-    buffering_state_cb_.Run(kHaveMetadata);
-
-    // Fire a seek request to get the renderers to preroll. We can skip a seek
-    // here as the demuxer should be at the start of the stream.
-    seek_pending_ = true;
-    SetState(kSeeking);
-    seek_timestamp_ = demuxer_->GetStartTime();
-    DoSeek(seek_timestamp_, true,
-           base::Bind(&Pipeline::OnFilterStateTransition, this));
-  }
+  StateTransitionTask(PIPELINE_OK);
 }
 
-// This method is called as a result of the client calling Pipeline::Stop() or
-// as the result of an error condition.
-// We stop the filters in the reverse order.
-//
-// TODO(scherkus): beware!  this can get posted multiple times since we post
-// Stop() tasks even if we've already stopped.  Perhaps this should no-op for
-// additional calls, however most of this logic will be changing.
 void Pipeline::StopTask(const base::Closure& stop_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(stop_cb_.is_null());
@@ -690,71 +719,44 @@ void Pipeline::StopTask(const base::Closure& stop_cb) {
     return;
   }
 
+  // TODO(scherkus): Remove after pipeline state machine refactoring has some
+  // time to bake http://crbug.com/110228
   if (video_renderer_)
     video_renderer_->PrepareForShutdownHack();
 
-  if (tearing_down_ && status_ != PIPELINE_OK) {
-    // If we are stopping due to SetError(), stop normally instead of
-    // going to error state and calling |error_cb_|. This converts
-    // the teardown in progress from an error teardown into one that acts
-    // like the error never occurred.
-    base::AutoLock auto_lock(lock_);
-    status_ = PIPELINE_OK;
-  }
-
+  SetState(kStopping);
+  pending_callbacks_.reset();
   stop_cb_ = stop_cb;
 
-  if (!IsPipelineSeeking() && !tearing_down_) {
-    // We will tear down pipeline immediately when there is no seek operation
-    // pending and no teardown in progress. This should include the case where
-    // we are partially initialized.
-    TearDownPipeline();
-  }
+  DoStop(base::Bind(&Pipeline::OnStopCompleted, this));

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

Move the bind into DoStop() since all call sites do tthe exact same bind.

[scherkus (not reviewing), 2012/09/06 15:33:20]

I prefer to have the resulting callback function documented where we initiate
the stop as opposed to buried within the function. Sames goes for all the
DoXXX() functions.

 }
 
 void Pipeline::ErrorChangedTask(PipelineStatus error) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_NE(PIPELINE_OK, error) << "PIPELINE_OK isn't an error!";
 
-  // Suppress executing additional error logic. Note that if we are currently
-  // performing a normal stop, then we return immediately and continue the
-  // normal stop.
-  if (state_ == kStopped || tearing_down_) {
+  if (state_ == kStopping || state_ == kStopped)
     return;
-  }
 
-  base::AutoLock auto_lock(lock_);
+  SetState(kStopping);
+  pending_callbacks_.reset();
   status_ = error;
 
-  // Posting TearDownPipeline() to message loop so that we can make sure
-  // it runs after any pending callbacks that are already queued.
-  // |tearing_down_| is set early here to make sure that pending callbacks
-  // don't modify the state before TearDownPipeline() can run.
-  tearing_down_ = true;
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::TearDownPipeline, this));
+  DoStop(base::Bind(&Pipeline::OnStopCompleted, this));
 }
 
 void Pipeline::PlaybackRateChangedTask(float playback_rate) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  if (state_ == kStopped || tearing_down_)
+  // Playback rate changes are only carried out while playing.
+  if (state_ != kStarting && state_ != kStarted)
     return;
 
-  // Suppress rate change until after seeking.
-  if (IsPipelineSeeking()) {
-    pending_playback_rate_ = playback_rate;
-    playback_rate_change_pending_ = true;
-    return;
-  }
-
   {
     base::AutoLock auto_lock(lock_);
     clock_->SetPlaybackRate(playback_rate);
   }
 
-  // These will get set after initialization completes in case playback rate is
-  // set prior to initialization.
   if (demuxer_)
     demuxer_->SetPlaybackRate(playback_rate);
   if (audio_renderer_)
@@ -766,7 +768,8 @@ void Pipeline::PlaybackRateChangedTask(float playback_rate) {
 void Pipeline::VolumeChangedTask(float volume) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  if (state_ == kStopped || tearing_down_)
+  // Volume changes are only carried out while playing.
+  if (state_ != kStarting && state_ != kStarted)
     return;
 
   if (audio_renderer_)
@@ -779,6 +782,9 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
 
   // Suppress seeking if we're not fully started.
   if (state_ != kStarted) {
+    DCHECK(state_ == kStopping || state_ == kStopped)
+        << "Receive extra seek in unexpected state: " << state_;
+
     // TODO(scherkus): should we run the callback?  I'm tempted to say the API
     // will only execute the first Seek() request.
     DVLOG(1) << "Media pipeline has not started, ignoring seek to "
@@ -786,29 +792,22 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
     return;
   }
 
-  DCHECK(!seek_pending_);
-  seek_pending_ = true;
-
-  // We'll need to pause every filter before seeking.  The state transition
-  // is as follows:
-  //   kStarted
-  //   kPausing (for each filter)
-  //   kSeeking (for each filter)
-  //   kStarting (for each filter)
-  //   kStarted
-  SetState(kPausing);
+  DCHECK(seek_cb_.is_null());
+
+  SetState(kSeeking);
+  base::TimeDelta seek_timestamp = std::max(time, demuxer_->GetStartTime());
+  seek_cb_ = seek_cb;
   audio_ended_ = false;
   video_ended_ = false;
-  seek_timestamp_ = std::max(time, demuxer_->GetStartTime());
-  seek_cb_ = seek_cb;
 
   // Kick off seeking!
   {
     base::AutoLock auto_lock(lock_);
     if (clock_->IsPlaying())
       clock_->Pause();
+    clock_->SetTime(seek_timestamp, seek_timestamp);
   }
-  DoPause(base::Bind(&Pipeline::OnFilterStateTransition, this));
+  DoSeek(seek_timestamp, base::Bind(&Pipeline::OnStateTransition, this));
 }
 
 void Pipeline::DoAudioRendererEnded() {
@@ -856,7 +855,9 @@ void Pipeline::RunEndedCallbackIfNeeded() {
     clock_->EndOfStream();
   }
 
-  ReportStatus(ended_cb_, status_);
+  // TODO(scherkus): Change |ended_cb_| into a Closure.
+  DCHECK_EQ(status_, PIPELINE_OK);
+  ended_cb_.Run(status_);
 }
 
 void Pipeline::AudioDisabledTask() {
@@ -869,252 +870,97 @@ void Pipeline::AudioDisabledTask() {
   // Notify our demuxer that we're no longer rendering audio.
   demuxer_->OnAudioRendererDisabled();
 
-  // Start clock since there is no more audio to
-  // trigger clock updates.
+  // Start clock since there is no more audio to trigger clock updates.
   clock_->SetMaxTime(clock_->Duration());
   StartClockIfWaitingForTimeUpdate_Locked();
 }
 
-void Pipeline::FilterStateTransitionTask() {
+void Pipeline::InitializeDemuxer(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(pending_callbacks_.get())
-      << "Filter state transitions must be completed via pending_callbacks_";
-  pending_callbacks_.reset();
 
-  // State transitions while tearing down are handled via
-  // TeardownStateTransitionTask().
-  //
-  // TODO(scherkus): Merge all state machinery!
-  if (state_ == kStopped || tearing_down_) {
-    return;
-  }
-
-  if (!TransientState(state_)) {
-    NOTREACHED() << "Invalid current state: " << state_;
-    SetError(PIPELINE_ERROR_ABORT);
+  demuxer_ = filter_collection_->GetDemuxer();
+  if (!demuxer_) {

[Ami GONE FROM CHROMIUM, 2012/09/06 09:11:34]

You said "done" to acolwell's comment about turning this into a DCHECK, but
didn't.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Remove this stuff so we crash when faced with missing filter.

+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return;
   }
 
-  // Decrement the number of remaining transitions, making sure to transition
-  // to the next state if needed.
-  SetState(FindNextState(state_));
-  if (state_ == kSeeking) {
-    base::AutoLock auto_lock(lock_);
-    DCHECK(seek_timestamp_ != kNoTimestamp());
-    clock_->SetTime(seek_timestamp_, seek_timestamp_);
-  }
-
-  // Carry out the action for the current state.
-  if (TransientState(state_)) {
-    if (state_ == kPausing) {
-      DoPause(base::Bind(&Pipeline::OnFilterStateTransition, this));
-    } else if (state_ == kFlushing) {
-      DoFlush(base::Bind(&Pipeline::OnFilterStateTransition, this));
-    } else if (state_ == kSeeking) {
-      DoSeek(seek_timestamp_, false,
-             base::Bind(&Pipeline::OnFilterStateTransition, this));
-    } else if (state_ == kStarting) {
-      DoPlay(base::Bind(&Pipeline::OnFilterStateTransition, this));
-    } else if (state_ == kStopping) {
-      DoStop(base::Bind(&Pipeline::OnFilterStateTransition, this));
-    } else {
-      NOTREACHED() << "Unexpected state: " << state_;
-    }
-  } else if (state_ == kStarted) {
-
-    // Fire canplaythrough immediately after playback begins because of
-    // crbug.com/106480.
-    // TODO(vrk): set ready state to HaveFutureData when bug above is fixed.
-    if (status_ == PIPELINE_OK)
-      buffering_state_cb_.Run(kPrerollCompleted);
-
-    FinishSeek();
-
-    // If a playback rate change was requested during a seek, do it now that
-    // the seek has compelted.
-    if (playback_rate_change_pending_) {
-      playback_rate_change_pending_ = false;
-      PlaybackRateChangedTask(pending_playback_rate_);
-    }
-
-    base::AutoLock auto_lock(lock_);
-    // We use audio stream to update the clock. So if there is such a stream,
-    // we pause the clock until we receive a valid timestamp.
-    waiting_for_clock_update_ = true;
-    if (!has_audio_) {
-      clock_->SetMaxTime(clock_->Duration());
-      StartClockIfWaitingForTimeUpdate_Locked();
-    }
-
-    // Check if we have a pending stop request that needs to be honored.
-    if (!stop_cb_.is_null()) {
-      TearDownPipeline();
-    }
-  } else {
-    NOTREACHED() << "Unexpected state: " << state_;
-  }
+  demuxer_->Initialize(this, done_cb);
 }
 
-void Pipeline::TeardownStateTransitionTask() {
-  DCHECK(tearing_down_);
-  DCHECK(pending_callbacks_.get())
-      << "Teardown state transitions must be completed via pending_callbacks_";
-  pending_callbacks_.reset();
-
-  switch (state_) {
-    case kStopping:
-      SetState(kStopped);
-      FinishDestroyingFiltersTask();
-      break;
-    case kPausing:
-      SetState(kFlushing);
-      DoFlush(base::Bind(&Pipeline::OnTeardownStateTransition, this));
-      break;
-    case kFlushing:
-      SetState(kStopping);
-      DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
-      break;
-
-    case kCreated:
-    case kInitDemuxer:
-    case kInitAudioDecoder:
-    case kInitAudioRenderer:
-    case kInitVideoRenderer:
-    case kSeeking:
-    case kStarting:
-    case kStopped:
-    case kStarted:
-      NOTREACHED() << "Unexpected state for teardown: " << state_;
-      break;
-    // default: intentionally left out to force new states to cause compiler
-    // errors.
-  };
-}
-
-void Pipeline::FinishDestroyingFiltersTask() {
+void Pipeline::InitializeAudioDecoder(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, kStopped);
-
-  audio_renderer_ = NULL;
-  video_renderer_ = NULL;
-  demuxer_ = NULL;
-  tearing_down_ = false;
-  {
-    base::AutoLock l(lock_);
-    running_ = false;
-  }
 
-  if (!IsPipelineOk() && !error_cb_.is_null())
-    error_cb_.Run(status_);
-
-  if (!stop_cb_.is_null())
-    base::ResetAndReturn(&stop_cb_).Run();
-}
-
-void Pipeline::InitializeDemuxer() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
+  scoped_refptr<DemuxerStream> stream =
+      demuxer_->GetStream(DemuxerStream::AUDIO);
 
-  demuxer_ = filter_collection_->GetDemuxer();
-  if (!demuxer_) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+  if (!stream) {
+    done_cb.Run(PIPELINE_OK);
     return;
   }
 
-  demuxer_->Initialize(this, base::Bind(&Pipeline::OnDemuxerInitialized, this));
-}
+  filter_collection_->SelectAudioDecoder(&audio_decoder_);
 
-void Pipeline::OnDemuxerInitialized(PipelineStatus status) {
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &Pipeline::OnDemuxerInitialized, this, status));
+  if (!audio_decoder_) {
+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return;
   }
 
-  if (status != PIPELINE_OK) {
-    SetError(status);
-    return;
-  }
-
-  {
-    base::AutoLock auto_lock(lock_);
-    // We do not want to start the clock running. We only want to set the base
-    // media time so our timestamp calculations will be correct.
-    clock_->SetTime(demuxer_->GetStartTime(), demuxer_->GetStartTime());
-  }
-
-  OnFilterInitialize(PIPELINE_OK);
+  audio_decoder_->Initialize(
+      stream, done_cb, base::Bind(&Pipeline::OnUpdateStatistics, this));
 }
 
-bool Pipeline::InitializeAudioDecoder(
-    const scoped_refptr<Demuxer>& demuxer) {
+void Pipeline::InitializeAudioRenderer(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
-  DCHECK(demuxer);
-
-  scoped_refptr<DemuxerStream> stream =
-      demuxer->GetStream(DemuxerStream::AUDIO);
 
-  if (!stream)
-    return false;
-
-  filter_collection_->SelectAudioDecoder(&pipeline_init_state_->audio_decoder);
-
-  if (!pipeline_init_state_->audio_decoder) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
-    return false;
+  if (!audio_decoder_) {
+    done_cb.Run(PIPELINE_OK);
+    return;
   }
 
-  pipeline_init_state_->audio_decoder->Initialize(
-      stream,
-      base::Bind(&Pipeline::OnFilterInitialize, this),
-      base::Bind(&Pipeline::OnUpdateStatistics, this));
-  return true;
-}
-
-bool Pipeline::InitializeAudioRenderer(
-    const scoped_refptr<AudioDecoder>& decoder) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
-
-  if (!decoder)
-    return false;
-
   filter_collection_->SelectAudioRenderer(&audio_renderer_);
   if (!audio_renderer_) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
-    return false;
+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+    return;
   }
 
   audio_renderer_->Initialize(
-      decoder,
-      base::Bind(&Pipeline::OnFilterInitialize, this),
+      audio_decoder_,
+      done_cb,
       base::Bind(&Pipeline::OnAudioUnderflow, this),
       base::Bind(&Pipeline::OnAudioTimeUpdate, this),
       base::Bind(&Pipeline::OnAudioRendererEnded, this),
       base::Bind(&Pipeline::OnAudioDisabled, this),
       base::Bind(&Pipeline::SetError, this));
-  return true;
 }
 
-bool Pipeline::InitializeVideoRenderer(
-    const scoped_refptr<DemuxerStream>& stream) {
+void Pipeline::InitializeVideoRenderer(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
 
-  if (!stream)
-    return false;
+  scoped_refptr<DemuxerStream> stream =
+      demuxer_->GetStream(DemuxerStream::VIDEO);
+
+  if (!stream) {
+    done_cb.Run(PIPELINE_OK);
+    return;
+  }
 
   filter_collection_->SelectVideoRenderer(&video_renderer_);
   if (!video_renderer_) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
-    return false;
+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+    return;
+  }
+
+  {
+    // Get an initial natural size so we have something when we signal
+    // the kHaveMetadata buffering state.
+    base::AutoLock l(lock_);
+    natural_size_ = stream->video_decoder_config().natural_size();
   }
 
   video_renderer_->Initialize(
       stream,
       *filter_collection_->GetVideoDecoders(),
-      base::Bind(&Pipeline::OnFilterInitialize, this),
+      done_cb,
       base::Bind(&Pipeline::OnUpdateStatistics, this),
       base::Bind(&Pipeline::OnVideoTimeUpdate, this),
       base::Bind(&Pipeline::OnNaturalVideoSizeChanged, this),
@@ -1123,99 +969,6 @@ bool Pipeline::InitializeVideoRenderer(
       base::Bind(&Pipeline::GetMediaTime, this),
       base::Bind(&Pipeline::GetMediaDuration, this));
   filter_collection_->GetVideoDecoders()->clear();
-  return true;
-}
-
-void Pipeline::TearDownPipeline() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_NE(kStopped, state_);
-
-  // We're either...
-  //   1) ...tearing down due to Stop() (it doesn't set tearing_down_)
-  //   2) ...tearing down due to an error (it does set tearing_down_)
-  //   3) ...tearing down due to an error and Stop() was called during that time
-  DCHECK(!tearing_down_ ||
-         (tearing_down_ && status_ != PIPELINE_OK) ||
-         (tearing_down_ && !stop_cb_.is_null()));
-
-  // Mark that we already start tearing down operation.
-  tearing_down_ = true;
-
-  // Cancel any pending operation so we can proceed with teardown.
-  pending_callbacks_.reset();
-
-  switch (state_) {
-    case kCreated:
-      SetState(kStopped);
-      // Need to put this in the message loop to make sure that it comes
-      // after any pending callback tasks that are already queued.
-      message_loop_->PostTask(FROM_HERE, base::Bind(
-          &Pipeline::FinishDestroyingFiltersTask, this));
-      break;
-
-    case kInitDemuxer:
-    case kInitAudioDecoder:
-    case kInitAudioRenderer:
-    case kInitVideoRenderer:
-      // Make it look like initialization was successful.
-      filter_collection_.reset();
-      pipeline_init_state_.reset();
-
-      SetState(kStopping);
-      DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
-
-      FinishSeek();
-      break;
-
-    case kPausing:
-    case kSeeking:
-    case kFlushing:
-    case kStarting:
-      SetState(kStopping);
-      DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
-
-      if (seek_pending_)
-        FinishSeek();
-
-      break;
-
-    case kStarted:
-      SetState(kPausing);
-      DoPause(base::Bind(&Pipeline::OnTeardownStateTransition, this));
-      break;
-
-    case kStopping:
-    case kStopped:
-      NOTREACHED() << "Unexpected state for teardown: " << state_;
-      break;
-    // default: intentionally left out to force new states to cause compiler
-    // errors.
-  };
-}
-
-void Pipeline::DoSeek(base::TimeDelta seek_timestamp,
-                      bool skip_demuxer_seek,
-                      const PipelineStatusCB& done_cb) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(!pending_callbacks_.get());
-  SerialRunner::Queue bound_fns;
-
-  if (!skip_demuxer_seek) {
-    bound_fns.Push(base::Bind(
-        &Demuxer::Seek, demuxer_, seek_timestamp));
-  }
-
-  if (audio_renderer_) {
-    bound_fns.Push(base::Bind(
-        &AudioRenderer::Preroll, audio_renderer_, seek_timestamp));
-  }
-
-  if (video_renderer_) {
-    bound_fns.Push(base::Bind(
-        &VideoRenderer::Preroll, video_renderer_, seek_timestamp));
-  }
-
-  pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
 
 void Pipeline::OnAudioUnderflow() {