Rewrite media::Pipeline state transition machinery.

media/base/pipeline.cc

Index: media/base/pipeline.cc
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 24868cfac2c7941c714459e51b99360297d1df99..bd8c08e3836967a4a749b6a83a68001c3fbc8f5f 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -17,6 +17,7 @@
 #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/callback_util.h"
 #include "media/base/clock.h"
 #include "media/base/filter_collection.h"
@@ -69,12 +70,21 @@ struct Pipeline::PipelineInitState {
 Pipeline::Pipeline(MessageLoop* message_loop, MediaLog* media_log)
     : message_loop_(message_loop->message_loop_proxy()),
       media_log_(media_log),
+      running_(false),

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

I believe it is the case that:
running_ == state_ > kCreated && state_ < kStopped
If that's the case can you drop running_?

+      did_loading_progress_(false),
+      total_bytes_(0),
+      volume_(1.0f),
+      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_disabled_(false),
       creation_time_(base::Time::Now()) {
   media_log_->AddEvent(media_log_->CreatePipelineStateChangedEvent(kCreated));
-  ResetState();
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::PIPELINE_CREATED));
 }
@@ -82,8 +92,9 @@ Pipeline::Pipeline(MessageLoop* message_loop, MediaLog* media_log)
 Pipeline::~Pipeline() {
   base::AutoLock auto_lock(lock_);

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

If you think you need a lock in a dtor, you're gonna have a bad time.

Replace w/ assertion about thread of deletion?

   DCHECK(!running_) << "Stop() must complete before destroying object";
-  DCHECK(!stop_pending_);
-  DCHECK(!seek_pending_);
+  DCHECK(start_cb_.is_null());
+  DCHECK(seek_cb_.is_null());
+  DCHECK(stop_cb_.is_null());
 
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::PIPELINE_DESTROYED));
@@ -95,8 +106,8 @@ void Pipeline::Start(scoped_ptr<FilterCollection> collection,
                      const PipelineStatusCB& start_cb) {
   base::AutoLock auto_lock(lock_);

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

replace w/ assertion about running on render thread?

   CHECK(!running_) << "Media pipeline is already running";
-
   running_ = true;
+
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::StartTask, this, base::Passed(&collection),
       ended_cb, error_cb, start_cb));
@@ -104,9 +115,7 @@ void Pipeline::Start(scoped_ptr<FilterCollection> collection,
 
 void Pipeline::Stop(const base::Closure& stop_cb) {
   base::AutoLock auto_lock(lock_);

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

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.

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

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.

-  CHECK(running_) << "Media pipeline isn't running";

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

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)

 
-  // Stop the pipeline, which will set |running_| to false on our behalf.
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::StopTask, this, stop_cb));
 }
@@ -124,24 +133,6 @@ bool Pipeline::IsRunning() const {
   return running_;
 }
 
-bool Pipeline::IsInitialized() const {
-  // TODO(scherkus): perhaps replace this with a bool that is set/get under the
-  // lock, because this is breaching the contract that |state_| is only accessed
-  // on |message_loop_|.
-  base::AutoLock auto_lock(lock_);
-  switch (state_) {
-    case kPausing:
-    case kFlushing:
-    case kSeeking:
-    case kStarting:
-    case kStarted:
-    case kEnded:
-      return true;
-    default:
-      return false;
-  }
-}
-
 bool Pipeline::HasAudio() const {
   base::AutoLock auto_lock(lock_);
   return has_audio_;
@@ -163,7 +154,7 @@ void Pipeline::SetPlaybackRate(float playback_rate) {
 
   base::AutoLock auto_lock(lock_);
   playback_rate_ = playback_rate;
-  if (running_ && !tearing_down_) {
+  if (running_) {

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

couldn't this be an early-return at the top of the function?  
Ditto for other methods that inspect running_.

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

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().

     message_loop_->PostTask(FROM_HERE, base::Bind(
         &Pipeline::PlaybackRateChangedTask, this, playback_rate));
   }
@@ -180,7 +171,7 @@ void Pipeline::SetVolume(float volume) {
 
   base::AutoLock auto_lock(lock_);
   volume_ = volume;
-  if (running_ && !tearing_down_) {
+  if (running_) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &Pipeline::VolumeChangedTask, this, volume));
   }
@@ -243,28 +234,8 @@ void Pipeline::SetClockForTesting(Clock* clock) {
   clock_.reset(clock);
 }
 
-void Pipeline::ResetState() {
-  base::AutoLock auto_lock(lock_);
-  const TimeDelta kZero;
-  running_          = false;
-  stop_pending_     = false;
-  seek_pending_     = false;
-  tearing_down_     = false;
-  error_caused_teardown_ = false;
-  playback_rate_change_pending_ = false;
-  buffered_byte_ranges_.clear();
-  did_loading_progress_ = false;
-  total_bytes_      = 0;
-  natural_size_.SetSize(0, 0);
-  volume_           = 1.0f;
-  playback_rate_    = 0.0f;
-  pending_playback_rate_ = 0.0f;
-  status_           = PIPELINE_OK;
-  has_audio_        = false;
-  has_video_        = false;
-  waiting_for_clock_update_ = false;
-  audio_disabled_   = false;
-  clock_->Reset();
+void Pipeline::SetErrorForTesting(PipelineStatus status) {
+  SetError(status);
 }
 
 void Pipeline::SetState(State next_state) {
@@ -283,81 +254,6 @@ bool Pipeline::IsPipelineOk() {
   return status_ == PIPELINE_OK;
 }
 
-bool Pipeline::IsPipelineStopped() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  return state_ == kStopped || state_ == kError;
-}
-
-bool Pipeline::IsPipelineTearingDown() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  return tearing_down_;
-}
-
-bool Pipeline::IsPipelineStopPending() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  return stop_pending_;
-}
-
-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;
-}
-
-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();
-}
-
-void Pipeline::FinishInitialization() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  // 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() ||
-           IsPipelineStopPending() ||
-           IsPipelineTearingDown());
-    return IsPipelineSeeking() ? kSeeking : kStopping;
-  } else if (current == kSeeking) {
-    return kStarting;
-  } else if (current == kStarting) {
-    return kStarted;
-  } else if (current == kStopping) {
-    return error_caused_teardown_ ? kError : kStopped;
-  } else {
-    return current;
-  }
-}
-
 void Pipeline::OnDemuxerError(PipelineStatus error) {
   SetError(error);
 }
@@ -453,6 +349,9 @@ TimeDelta Pipeline::TimeForByteOffset_Locked(int64 byte_offset) const {
 
 void Pipeline::DoPause(const base::Closure& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kPausing);
+  DCHECK(seek_timestamp_ != kNoTimestamp());
+
   scoped_ptr<std::queue<ClosureFunc> > closures(new std::queue<ClosureFunc>);
 
   if (audio_renderer_)
@@ -466,6 +365,9 @@ void Pipeline::DoPause(const base::Closure& done_cb) {
 
 void Pipeline::DoFlush(const base::Closure& done_cb) {

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

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!

   DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kFlushing);
+  DCHECK(seek_timestamp_ != kNoTimestamp());
+
   scoped_ptr<std::queue<ClosureFunc> > closures(new std::queue<ClosureFunc>);
 
   if (audio_renderer_)
@@ -479,6 +381,14 @@ void Pipeline::DoFlush(const base::Closure& done_cb) {
 
 void Pipeline::DoPlay(const base::Closure& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kStarting);
+  DCHECK(seek_timestamp_ == kNoTimestamp());
+
+  // Update playback rate and volume in case it changed before we resume
+  // playback.
+  PlaybackRateChangedTask(GetPlaybackRate());

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

here's the new method for handling rate/volume changes during non-playback
states

+  VolumeChangedTask(GetVolume());
+
   scoped_ptr<std::queue<ClosureFunc> > closures(new std::queue<ClosureFunc>);
 
   if (audio_renderer_)
@@ -492,6 +402,13 @@ void Pipeline::DoPlay(const base::Closure& done_cb) {
 
 void Pipeline::DoStop(const base::Closure& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kStopping);
+
+  if (video_decoder_) {
+    video_decoder_->PrepareForShutdownHack();

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

this might actually go away now that we don't Pause+Flush before Stop

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

Yes, it can!

+    video_decoder_ = NULL;
+  }
+
   scoped_ptr<std::queue<ClosureFunc> > closures(new std::queue<ClosureFunc>);
 
   if (demuxer_)
@@ -538,37 +455,6 @@ void Pipeline::OnRendererEnded() {
 }
 
 // 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.
-void Pipeline::OnFilterStateTransition() {
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::FilterStateTransitionTask, this));
-}
-
-// 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: Revisit this code when SetError() is removed from FilterHost and
-//       all the Closures are converted to PipelineStatusCB.
-void Pipeline::OnFilterStateTransitionWithStatus(PipelineStatus status) {
-  if (status != PIPELINE_OK)
-    SetError(status);
-  OnFilterStateTransition();
-}
-
-void Pipeline::OnTeardownStateTransition() {
-  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;
@@ -586,201 +472,259 @@ void Pipeline::StartTask(scoped_ptr<FilterCollection> filter_collection,
   filter_collection_ = filter_collection.Pass();
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
-  seek_cb_ = start_cb;
+  start_cb_ = start_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) {
+  DoInitDemuxer(base::Bind(&Pipeline::DoStateTransition, this));
+}
+
+bool Pipeline::IsTransitioning() {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  switch (state_) {
+    case kCreated:
+    case kStarted:
+    case kStopped:
+      return false;
 
-  if (last_stage_status != PIPELINE_OK) {
-    // Currently only VideoDecoders have a recoverable error code.
-    if (state_ == kInitVideoDecoder &&
-        last_stage_status == DECODER_ERROR_NOT_SUPPORTED) {
-      state_ = kInitAudioRenderer;
-    } else {
-      SetError(last_stage_status);
-    }
+    case kInitDemuxer:
+    case kInitAudioDecoder:
+    case kInitAudioRenderer:
+    case kInitVideoDecoder:
+    case kInitVideoRenderer:
+    case kPausing:
+    case kFlushing:
+    case kSeeking:
+    case kStarting:
+    case kStopping:
+      return true;
+
+    // Catch any left out states.
   }
+  NOTREACHED();
+  return false;
+}
+
+void Pipeline::OnStateTransition(PipelineStatus status) {
+  // Force-post a task on state transitions since to avoid reentrancy between

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

s/since/

+  // states.
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &Pipeline::DoStateTransition, this, status));
+}
+
+void Pipeline::DoStateTransition(PipelineStatus status) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
 
-  // If we have received the stop or error signal, return immediately.
-  if (IsPipelineStopPending() || IsPipelineStopped() || !IsPipelineOk())
+  // Preserve existing abnormal status, otherwise update based on the result of
+  // the previous operation.
+  status_ = (status_ != PIPELINE_OK ? status_ : status);
+
+  PipelineStatusCB done_cb = base::Bind(&Pipeline::OnStateTransition, this);

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

Can move south of the error-handling case.

+  base::Closure done_cb_no_status = base::Bind(

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

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?

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

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

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

d_c_no_status is a funny name for a thing that has a status_ in it...

+      &Pipeline::OnStateTransition, this, status_);
+
+  // Check if we need to stop due to an error or due to |stop_cb_| being set.

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

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.

+  if (state_ != kStopping && state_ != kStopped &&
+      (status_ != PIPELINE_OK || !stop_cb_.is_null())) {
+    SetState(kStopping);
+    DoStop(done_cb_no_status);
     return;
+  }
 
-  DCHECK(state_ == kInitDemuxer ||
-         state_ == kInitAudioDecoder ||
-         state_ == kInitAudioRenderer ||
-         state_ == kInitVideoDecoder ||
-         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_))
+  switch(state_) {
+    case kCreated:
+      NOTREACHED() << "kCreated";

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

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

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

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

       return;
-  }
 
-  // Assuming audio decoder was created, create audio renderer.
-  if (state_ == kInitAudioDecoder) {
-    SetState(kInitAudioRenderer);
+    case kInitDemuxer:
+      {
+        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());
+      }
+      SetState(kInitAudioDecoder);
+      DoInitAudioDecoder(done_cb);

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

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.

+      return;
 
-    // Returns false if there's no audio stream.
-    if (InitializeAudioRenderer(pipeline_init_state_->audio_decoder)) {
-      base::AutoLock auto_lock(lock_);
-      has_audio_ = true;
+    case kInitAudioDecoder:
+      SetState(kInitAudioRenderer);
+      DoInitAudioRenderer(done_cb);
       return;
-    }
-  }
 
-  // Assuming audio renderer was created, create video decoder.
-  if (state_ == kInitAudioRenderer) {
-    // Then perform the stage of initialization, i.e. initialize video decoder.
-    SetState(kInitVideoDecoder);
-    if (InitializeVideoDecoder(demuxer_))
+    case kInitAudioRenderer:
+      SetState(kInitVideoDecoder);
+      DoInitVideoDecoder(done_cb);
       return;
-  }
 
-  // Assuming video decoder was created, create video renderer.
-  if (state_ == kInitVideoDecoder) {
-    SetState(kInitVideoRenderer);
-    if (InitializeVideoRenderer(pipeline_init_state_->video_decoder)) {
-      base::AutoLock auto_lock(lock_);
-      has_video_ = true;
+    case kInitVideoDecoder:
+      SetState(kInitVideoRenderer);
+      DoInitVideoRenderer(done_cb);
       return;
-    }
-  }
 
-  if (state_ == kInitVideoRenderer) {
-    if (!IsPipelineOk() || !(HasAudio() || HasVideo())) {
-      SetError(PIPELINE_ERROR_COULD_NOT_RENDER);
+    case kInitVideoRenderer: {
+      bool success = true;
+      {
+        base::AutoLock l(lock_);
+        has_audio_ = !!audio_renderer_ && !audio_disabled_;

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

hmm... this will actually cause us to not render the volume button for busted
sound cards...

+        has_video_ = !!video_renderer_;

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

Is it really worthwhile maintaining members for these?
(has_{audio,video}_)
Why can't they be computed on demand?

+
+        // We're still successful if we have audio but the sound card is busted.
+        success = !!audio_renderer_ || !!video_renderer_;
+      }
+      if (!success) {
+        DoStateTransition(PIPELINE_ERROR_COULD_NOT_RENDER);
+        return;
+      }
+
+      // Clear initialization state now that we're done.
+      filter_collection_.reset();
+      pipeline_init_state_.reset();
+
+      // Kick off initial preroll.
+      seek_timestamp_ = demuxer_->GetStartTime();
+      SetState(kSeeking);
+      DoSeek(true, done_cb);
       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());
-
-    // 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::OnFilterStateTransitionWithStatus, this));
+    case kPausing:
+      DCHECK(seek_timestamp_ != kNoTimestamp());
+      SetState(kFlushing);
+      DoFlush(done_cb_no_status);
+      return;
+
+    case kFlushing:
+      DCHECK(seek_timestamp_ != kNoTimestamp());
+      SetState(kSeeking);
+      DoSeek(false, done_cb);
+      return;
+
+    case kSeeking:
+      DCHECK(seek_timestamp_ != kNoTimestamp());
+      seek_timestamp_ = kNoTimestamp();
+
+      SetState(kStarting);
+      DoPlay(done_cb_no_status);
+      return;
+
+    case kStarting:
+      {
+        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();
+        }
+      }
+
+      SetState(kStarted);
+      if (!start_cb_.is_null()) {
+        DCHECK_EQ(status_, PIPELINE_OK);
+        base::ResetAndReturn(&start_cb_).Run(status_);
+        return;
+      }
+      if (!seek_cb_.is_null()) {
+        DCHECK_EQ(status_, PIPELINE_OK);
+        base::ResetAndReturn(&seek_cb_).Run(status_);
+      }
+      return;
+
+    case kStarted:
+      NOTREACHED() << "kStarted";
+      return;
+
+    case kStopping:
+      // Release all references.
+      pipeline_init_state_.reset();
+      filter_collection_.reset();
+      audio_renderer_ = NULL;
+      video_renderer_ = NULL;
+      demuxer_ = NULL;
+      {
+        base::AutoLock l(lock_);
+        running_ = false;
+      }
+      SetState(kStopped);
+
+      // Execute any client-initiated pending callbacks if we've got some,
+      // otherwise use the permanent callback |error_cb_|.
+      if (!start_cb_.is_null()) {

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

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

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

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?

+        base::ResetAndReturn(&start_cb_).Run(status_);
+        error_cb_.Reset();
+      }
+      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_);
+      }
+      return;
+
+    case kStopped:
+      NOTREACHED() << "kStopped";
+      return;
+
+    default:

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

drop

+      NOTREACHED() << "State has no transition: " << state_;
   }
+  NOTREACHED() << "You should return, not break!";
 }
 
-// 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(!IsPipelineStopPending());
-  DCHECK_NE(state_, kStopped);
+  DCHECK(stop_cb_.is_null());
 
-  if (video_decoder_) {
-    video_decoder_->PrepareForShutdownHack();
-    video_decoder_ = NULL;
-  }
-
-  if (IsPipelineTearingDown() && error_caused_teardown_) {
-    // 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;
-    error_caused_teardown_ = false;
+  if (state_ == kStopped) {
+    stop_cb.Run();
+    return;
   }
 
   stop_cb_ = stop_cb;
 
-  stop_pending_ = true;
-  if (!IsPipelineSeeking() && !IsPipelineTearingDown()) {
-    // 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();
-  }
+  if (IsTransitioning())
+    return;
+
+  DoStateTransition(PIPELINE_OK);

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

technically I think I can SetState(kStopping) here instead of having that bit of
code inside of DST() that inspects for stop_cb_

 }
 
 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 (IsPipelineStopped() || IsPipelineTearingDown()) {
+  // Preserve the error code.

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

s/the/an existing/

+  if (status_ != PIPELINE_OK)
     return;
-  }
 
-  base::AutoLock auto_lock(lock_);
   status_ = error;
 
-  error_caused_teardown_ = true;
+  if (IsTransitioning())
+    return;
 
-  // 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 TeadDownPipeline() can run.
-  tearing_down_ = true;
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::TearDownPipeline, this));
+  DoStateTransition(status_);
 }
 
 void Pipeline::PlaybackRateChangedTask(float playback_rate) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  if (!running_ || tearing_down_)
+  // Suppress rate change until we're playing.

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

Is it right to drop this?  Or does WMPI already cover for us?

+  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_)
@@ -791,7 +735,9 @@ void Pipeline::PlaybackRateChangedTask(float playback_rate) {
 
 void Pipeline::VolumeChangedTask(float volume) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  if (!running_ || tearing_down_)
+
+  // Suppress volume change until we're playing.

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

ditto

+  if (state_ != kStarting && state_ != kStarted)
     return;
 
   if (audio_renderer_)
@@ -800,10 +746,9 @@ void Pipeline::VolumeChangedTask(float volume) {
 
 void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(!IsPipelineStopPending());
 
   // Suppress seeking if we're not fully started.
-  if (state_ != kStarted && state_ != kEnded) {
+  if (state_ != kStarted) {
     // 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 "
@@ -811,17 +756,13 @@ 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/kEnded
-  //   kPausing (for each filter)
-  //   kSeeking (for each filter)
-  //   kStarting (for each filter)
   //   kStarted
-  SetState(kPausing);
+  //   kPausing
+  //   kSeeking
+  //   kStarting
+  //   kStarted
   seek_timestamp_ = std::max(time, demuxer_->GetStartTime());
   seek_cb_ = seek_cb;
 
@@ -831,7 +772,8 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
     if (clock_->IsPlaying())
       clock_->Pause();
   }
-  DoPause(base::Bind(&Pipeline::OnFilterStateTransition, this));
+  SetState(kPausing);
+  DoPause(base::Bind(&Pipeline::OnStateTransition, this, PIPELINE_OK));
 }
 
 void Pipeline::OnRendererEndedTask() {
@@ -861,14 +803,13 @@ void Pipeline::OnRendererEndedTask() {
     return;
   }
 
-  // Transition to ended, executing the callback if present.
-  SetState(kEnded);
   {
     base::AutoLock auto_lock(lock_);
     clock_->EndOfStream();
   }
 
-  ReportStatus(ended_cb_, status_);
+  DCHECK_EQ(status_, PIPELINE_OK);
+  ended_cb_.Run(status_);
 }
 
 void Pipeline::AudioDisabledTask() {
@@ -887,269 +828,110 @@ void Pipeline::AudioDisabledTask() {
   StartClockIfWaitingForTimeUpdate_Locked();
 }
 
-void Pipeline::FilterStateTransitionTask() {
+void Pipeline::DoInitDemuxer(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kInitDemuxer);
 
-  // No reason transitioning if we've errored or have stopped.
-  if (IsPipelineStopped()) {
-    return;
-  }
-
-  // If we are tearing down, don't allow any state changes. Teardown
-  // state changes will come in via TeardownStateTransitionTask().
-  if (IsPipelineTearingDown()) {
-    return;
-  }
-
-  if (!TransientState(state_)) {
-    NOTREACHED() << "Invalid current state: " << state_;
-    SetError(PIPELINE_ERROR_ABORT);
+  demuxer_ = filter_collection_->GetDemuxer();
+  if (!demuxer_) {

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

is this something other than programming error?  Can we promote this to a
DCHECK?

+    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_);
-    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::OnFilterStateTransitionWithStatus, 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) {
-    FinishInitialization();
-
-    // Finally, complete the seek.
-    seek_pending_ = false;
-
-    // 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();
-    }
-
-    if (IsPipelineStopPending()) {
-      // We had a pending stop request need to be honored right now.
-      TearDownPipeline();
-    }
-  } else {
-    NOTREACHED() << "Unexpected state: " << state_;
-  }
-}
-
-void Pipeline::TeardownStateTransitionTask() {
-  DCHECK(IsPipelineTearingDown());
-  switch (state_) {
-    case kStopping:
-      SetState(error_caused_teardown_ ? kError : 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 kError:
-    case kInitDemuxer:
-    case kInitAudioDecoder:
-    case kInitAudioRenderer:
-    case kInitVideoDecoder:
-    case kInitVideoRenderer:
-    case kSeeking:
-    case kStarting:
-    case kStopped:
-    case kStarted:
-    case kEnded:
-      NOTREACHED() << "Unexpected state for teardown: " << state_;
-      break;
-    // default: intentionally left out to force new states to cause compiler
-    // errors.
-  };
+  demuxer_->Initialize(this, done_cb);
 }
 
-void Pipeline::FinishDestroyingFiltersTask() {
+void Pipeline::DoInitAudioDecoder(const PipelineStatusCB& done_cb) {

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

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}?

   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineStopped());
-
-  audio_renderer_ = NULL;
-  video_renderer_ = NULL;
-  demuxer_ = NULL;
+  DCHECK_EQ(state_, kInitAudioDecoder);
 
-  if (error_caused_teardown_ && !IsPipelineOk() && !error_cb_.is_null())
-    error_cb_.Run(status_);
+  scoped_refptr<DemuxerStream> stream =
+      demuxer_->GetStream(DemuxerStream::AUDIO);
 
-  if (stop_pending_) {
-    stop_pending_ = false;
-    ResetState();
-    // Notify the client that stopping has finished.
-    base::ResetAndReturn(&stop_cb_).Run();
+  if (!stream) {
+    done_cb.Run(PIPELINE_OK);
+    return;
   }
 
-  tearing_down_ = false;
-  error_caused_teardown_ = false;
-}
-
-void Pipeline::InitializeDemuxer() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
-
-  demuxer_ = filter_collection_->GetDemuxer();
-  if (!demuxer_) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+  filter_collection_->SelectAudioDecoder(&pipeline_init_state_->audio_decoder);

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

This SelectFoo() api is funny in that it doesn't just return its selected
pointer...

+  if (!pipeline_init_state_->audio_decoder) {

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

ditto (here and below).  Promote to DCHECK?

+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return;
   }
 
-  demuxer_->Initialize(this, base::Bind(&Pipeline::OnDemuxerInitialized, this));
+  pipeline_init_state_->audio_decoder->Initialize(
+      stream, done_cb, base::Bind(&Pipeline::OnUpdateStatistics, this));
 }
 
-void Pipeline::OnDemuxerInitialized(PipelineStatus status) {
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &Pipeline::OnDemuxerInitialized, this, status));
-    return;
-  }
+void Pipeline::DoInitAudioRenderer(const PipelineStatusCB& done_cb) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kInitAudioRenderer);
 
-  if (status != PIPELINE_OK) {
-    SetError(status);
+  if (!pipeline_init_state_->audio_decoder) {
+    done_cb.Run(PIPELINE_OK);
     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());
+  filter_collection_->SelectAudioRenderer(&audio_renderer_);
+  if (!audio_renderer_) {
+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+    return;
   }
 
-  OnFilterInitialize(PIPELINE_OK);
+  audio_renderer_->Initialize(
+      pipeline_init_state_->audio_decoder,
+      done_cb,
+      base::Bind(&Pipeline::OnAudioUnderflow, this),
+      base::Bind(&Pipeline::OnAudioTimeUpdate, this),
+      base::Bind(&Pipeline::OnRendererEnded, this),
+      base::Bind(&Pipeline::OnAudioDisabled, this),
+      base::Bind(&Pipeline::SetError, this));
 }
 
-bool Pipeline::InitializeAudioDecoder(
-    const scoped_refptr<Demuxer>& demuxer) {
+void Pipeline::DoInitVideoDecoder(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
-  DCHECK(demuxer);
+  DCHECK_EQ(state_, kInitVideoDecoder);
 
   scoped_refptr<DemuxerStream> stream =
-      demuxer->GetStream(DemuxerStream::AUDIO);
-
-  if (!stream)
-    return false;
+      demuxer_->GetStream(DemuxerStream::VIDEO);
 
-  filter_collection_->SelectAudioDecoder(&pipeline_init_state_->audio_decoder);
-
-  if (!pipeline_init_state_->audio_decoder) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
-    return false;
+  if (!stream) {
+    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::InitializeVideoDecoder(
-    const scoped_refptr<Demuxer>& demuxer) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
-  DCHECK(demuxer);
-
-  scoped_refptr<DemuxerStream> stream =
-      demuxer->GetStream(DemuxerStream::VIDEO);
-
-  if (!stream)
-    return false;
-
   filter_collection_->SelectVideoDecoder(&pipeline_init_state_->video_decoder);
-
   if (!pipeline_init_state_->video_decoder) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
-    return false;
+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+    return;
   }
 
   pipeline_init_state_->video_decoder->Initialize(
-      stream,
-      base::Bind(&Pipeline::OnFilterInitialize, this),
-      base::Bind(&Pipeline::OnUpdateStatistics, this));
-
-  video_decoder_ = pipeline_init_state_->video_decoder;
-  return true;
+      stream, done_cb, base::Bind(&Pipeline::OnUpdateStatistics, this));
 }
 
-bool Pipeline::InitializeAudioRenderer(
-    const scoped_refptr<AudioDecoder>& decoder) {
+void Pipeline::DoInitVideoRenderer(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
+  DCHECK_EQ(state_, kInitVideoRenderer);
 
-  if (!decoder)
-    return false;
-
-  filter_collection_->SelectAudioRenderer(&audio_renderer_);
-  if (!audio_renderer_) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
-    return false;
+  if (!pipeline_init_state_->video_decoder) {
+    done_cb.Run(PIPELINE_OK);
+    return;
   }
 
-  audio_renderer_->Initialize(
-      decoder,
-      base::Bind(&Pipeline::OnFilterInitialize, this),
-      base::Bind(&Pipeline::OnAudioUnderflow, this),
-      base::Bind(&Pipeline::OnAudioTimeUpdate, this),
-      base::Bind(&Pipeline::OnRendererEnded, this),
-      base::Bind(&Pipeline::OnAudioDisabled, this),
-      base::Bind(&Pipeline::SetError, this));
-  return true;
-}
-
-bool Pipeline::InitializeVideoRenderer(
-    const scoped_refptr<VideoDecoder>& decoder) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(IsPipelineOk());
-
-  if (!decoder)
-    return false;
-
   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;
   }
 
+  // TODO(scherkus): This is for PrepareForShutdownHack(), see
+  // http://crbug.com/110228 for details.
+  video_decoder_ = pipeline_init_state_->video_decoder;

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

Drop this member.

+
   video_renderer_->Initialize(
-      decoder,
-      base::Bind(&Pipeline::OnFilterInitialize, this),
+      video_decoder_,
+      done_cb,
       base::Bind(&Pipeline::OnUpdateStatistics, this),
       base::Bind(&Pipeline::OnVideoTimeUpdate, this),
       base::Bind(&Pipeline::OnNaturalVideoSizeChanged, this),
@@ -1157,94 +939,34 @@ bool Pipeline::InitializeVideoRenderer(
       base::Bind(&Pipeline::SetError, this),
       base::Bind(&Pipeline::GetMediaTime, this),
       base::Bind(&Pipeline::GetMediaDuration, this));
-  return true;
 }
 
-void Pipeline::TearDownPipeline() {
+void Pipeline::DoSeek(bool skip_demuxer_seek,
+                      const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_NE(kStopped, state_);
-
-  DCHECK(!tearing_down_ ||  // Teardown on Stop().
-         (tearing_down_ && error_caused_teardown_) ||  // Teardown on error.
-         (tearing_down_ && stop_pending_));  // Stop during teardown by error.
-
-  // Mark that we already start tearing down operation.
-  tearing_down_ = true;
-
-  switch (state_) {
-    case kCreated:
-    case kError:
-      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 kInitVideoDecoder:
-    case kInitVideoRenderer:
-      // Make it look like initialization was successful.
-      filter_collection_.reset();
-      pipeline_init_state_.reset();
-
-      SetState(kStopping);
-      DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
+  DCHECK(state_ == kSeeking);
+  DCHECK(seek_timestamp_ != kNoTimestamp());
 
-      FinishInitialization();
-      break;
-
-    case kPausing:
-    case kSeeking:
-    case kFlushing:
-    case kStarting:
-      SetState(kStopping);
-      DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
-
-      if (seek_pending_) {
-        seek_pending_ = false;
-        FinishInitialization();
-      }
-
-      break;
-
-    case kStarted:
-    case kEnded:
-      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.
-  };
-}
+  {
+    base::AutoLock l(lock_);
+    clock_->SetTime(seek_timestamp_, seek_timestamp_);
+  }
 
-void Pipeline::DoSeek(base::TimeDelta seek_timestamp,
-                      bool skip_demuxer_seek,
-                      const PipelineStatusCB& done_cb) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
   scoped_ptr<std::queue<PipelineStatusCBFunc> > status_cbs(
       new std::queue<PipelineStatusCBFunc>());
 
   if (!skip_demuxer_seek)

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

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?)

-    status_cbs->push(base::Bind(&Demuxer::Seek, demuxer_, seek_timestamp));
+    status_cbs->push(base::Bind(&Demuxer::Seek, demuxer_, seek_timestamp_));
 
   if (audio_renderer_)
     status_cbs->push(base::Bind(
-        &AudioRenderer::Preroll, audio_renderer_, seek_timestamp));
+        &AudioRenderer::Preroll, audio_renderer_, seek_timestamp_));
 
   if (video_renderer_)
     status_cbs->push(base::Bind(
-        &VideoRenderer::Preroll, video_renderer_, seek_timestamp));
+        &VideoRenderer::Preroll, video_renderer_, seek_timestamp_));
 
-  RunInSeriesWithStatus(status_cbs.Pass(), base::Bind(
-      &Pipeline::ReportStatus, this, done_cb));
+  RunInSeriesWithStatus(status_cbs.Pass(), done_cb);
 }
 
 void Pipeline::OnAudioUnderflow() {