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

media/base/pipeline.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/pipeline.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #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"
 #include "media/base/video_decoder.h"
 #include "media/base/video_decoder_config.h"
 #include "media/base/video_renderer.h"
 
 using base::TimeDelta;
 
 namespace media {
@@ skipping 24 matching lines @@
   while (!notified_)
     cv_.Wait();
 }
 
 media::PipelineStatus PipelineStatusNotification::status() {
   base::AutoLock auto_lock(lock_);
   DCHECK(notified_);
   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),
       creation_time_(base::Time::Now()) {
   media_log_->AddEvent(media_log_->CreatePipelineStateChangedEvent(kCreated));
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::PIPELINE_CREATED));
 }
 
 Pipeline::~Pipeline() {
   base::AutoLock auto_lock(lock_);

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

lock in dtor ==> bug

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

   DCHECK(!running_) << "Stop() must complete before destroying object";
   DCHECK(stop_cb_.is_null());
-  DCHECK(!seek_pending_);
+  DCHECK(seek_cb_.is_null());
 
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::PIPELINE_DESTROYED));
 }
 
 void Pipeline::Start(scoped_ptr<FilterCollection> collection,
                      const PipelineStatusCB& ended_cb,
                      const PipelineStatusCB& error_cb,
                      const PipelineStatusCB& seek_cb,
                      const BufferingStateCB& buffering_state_cb) {
@@ skipping 138 matching lines @@
   if (state_ != kStarted && next_state == kStarted &&
       !creation_time_.is_null()) {
     UMA_HISTOGRAM_TIMES(
         "Media.TimeToPipelineStarted", base::Time::Now() - creation_time_);
     creation_time_ = base::Time();
   }
   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) \

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

s/R_S/MAYBE_RETURN_STRING/?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Can't think of a good name and MAYBE_R_S seems funny. RETURN_STRING_FOR_STATE(),
perhaps?!

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

SGTM.  Or leave as-is, given the short-livedness of the definition.

+  case state: return #state;
+
+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);
+  }
+  return "";

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

NOTREACHED?

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

nit: How about "INVALID_STATE" instead of "" so this is a little more obvious in
log lines?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

 }
 
-bool Pipeline::IsPipelineSeeking() {
+#undef RETURN_STRING
+
+Pipeline::State Pipeline::GetNextState() const {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  if (!seek_pending_)
-    return false;
-  DCHECK(kSeeking == state_ || kPausing == state_ ||
-         kFlushing == state_ || kStarting == state_)
-      << "Current state : " << state_;
-  return true;
-}
+  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";

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

emit status_

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

 
-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();
-}
+  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;
 
-void Pipeline::FinishSeek() {
-  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;
+    case kStarted:
+    case kStopping:
+    case kStopped:
+      break;
   }
+  NOTREACHED() << "State has no transition: " << state_;

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

Put this and the next line at l.303?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

+  return state_;
 }
 
 void Pipeline::OnDemuxerError(PipelineStatus error) {
   SetError(error);
 }
 
 void Pipeline::SetError(PipelineStatus error) {
   DCHECK(IsRunning());
   DCHECK_NE(PIPELINE_OK, error);
   VLOG(1) << "Media pipeline error: " << error;
@@ skipping 76 matching lines @@
   // Since the byte->time calculation is approximate, fudge the beginning &
   // ending areas to look better.
   TimeDelta epsilon = clock_->Duration() / 100;
   if (time_offset < epsilon)
     return TimeDelta();
   if (time_offset + epsilon > clock_->Duration())
     return clock_->Duration();
   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::DoStateTransition, this, status));
+}
+
+void Pipeline::DoStateTransition(PipelineStatus status) {

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

why not StateTransitionTask?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Somewhere along the way I started mixing task-post-function-name styles. I'll
keep it FooTask() for now.

+  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.
+  if (state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) {
+    DCHECK(pending_callbacks_.get())

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

nit: !pending_callbacks_.is_null() ?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done (see new code / response to fischman)

+        << "Filter state transitions must be completed via pending_callbacks_";

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

wat?
Does this want to
DCHECK_EQ(pending_callbacks.get(), 
          state_ == kInitPrerolling || state_ == kStarting || state_ ==
kSeeking)
    << state_;
?  But if it does, why are we dropping pending_callbacks_ immediately below?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Yes
pending_callbacks_ is only used for those states
if we ended up here then pending_callbacks_ finished executing, meaning we can
reset it

the dcheck verifies that:
  * For states where we don't expect it, we didn't have an active
pending_callbacks_
  * For states where we do expect it, someone else didn't reset the object (see
other instances of pending_callbacks_.reset())

Since that's not completely obvious from an all-in-one DCHECK I split it into if
/ else so I can have clearer debug messages, but I admit it looks a little iffy

+  }
+  pending_callbacks_.reset();
+
+  PipelineStatusCB done_cb = base::Bind(&Pipeline::OnStateTransition, this);
+
+  DVLOG(2) << GetStateString(state_) << " -> "

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

Put this idea in SetState() ?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

+           << GetStateString(GetNextState());
+
+  SetState(GetNextState());
+  switch (state_) {
+    case kInitDemuxer:
+      InitializeDemuxer(done_cb);

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

FWIW I'm pretty sure all our toolchains support returning a void function, so
you can say:
return InitializeDemuxer(done_cb);
to make it easier to see what the last statement in each case is.

[scherkus (not reviewing), 2012/09/05 15:04:17]

I find it odd looking but will give it a shot.

+      return;
+
+    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());

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

What can race with this? 
IOW why the lock?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Pipeline::GetMediaTime() is called from any thread (renderer, various media
threads)

+      }
+      InitializeAudioDecoder(done_cb);
+      return;
+
+    case kInitAudioRenderer:
+      InitializeAudioRenderer(done_cb);
+      return;
+
+    case kInitVideoRenderer:
+      InitializeVideoRenderer(done_cb);
+      return;
+
+    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_;

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

Are the !!'s really necessary?  (ditto for next line)

[scherkus (not reviewing), 2012/09/05 15:04:17]

AFAIK it gets around MSVC performance warning -- see timely discussion on
chromium-dev:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/di06BqaudD0/dis...

I'll remove it and see what trybots say.

+        has_video_ = !!video_renderer_;
+      }
+      if (!audio_renderer_ && !video_renderer_) {
+        done_cb.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
+        return;
+      }
+
+      buffering_state_cb_.Run(kHaveMetadata);
+
+      DoInitialPreroll(demuxer_->GetStartTime(), done_cb);
+      return;
+
+    case kStarting:
+      DoPlay(done_cb);
+      return;
+
+    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)
+        buffering_state_cb_.Run(kPrerollCompleted);
+
+      DCHECK(!seek_cb_.is_null());
+      DCHECK_EQ(status_, PIPELINE_OK);
+      base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
+      return;
+
+    case kStopping:
+    case kStopped:
+    case kCreated:
+    case kSeeking:
+      NOTREACHED() << "State has no transition: " << state_;
+      break;

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

You should see the comment two lines down and return instead of breaking!

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

+  }
+  NOTREACHED() << "You should return after transitioning, not break!";

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

A smart compiler should flag this as dead code.  Not sure it adds value.

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

+}
+
+void Pipeline::DoInitialPreroll(
+    const base::TimeDelta& seek_timestamp,
+    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_));
+  // Preroll renderers.
+  if (audio_renderer_) {
+    bound_fns.Push(base::Bind(
+        &AudioRenderer::Preroll, audio_renderer_, seek_timestamp));
+  }
 
-  if (video_renderer_)
-    bound_fns.Push(base::Bind(&VideoRenderer::Pause, video_renderer_));
+  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(
+    const 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::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_) {
+    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::DoPlay(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(!pending_callbacks_.get());
   SerialRunner::Queue bound_fns;
 
+  PlaybackRateChangedTask(GetPlaybackRate());

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

Why was this moved from before initial preroll to here? Won't this cause the
wrong data to get prerolled for cases where PlaybackRate != 0 && PlaybackRange
!= 1? The audio_renderer_algorithm won't be engaged during preroll if you do
this right?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Good catch -- playback rate should always get set prior to prerolling and not
prior to playing.

I inserted a playback-rate-updating task prior to prerolling audio renderers,
but I'm not totally happy w/ the solution (the Times(2) gmock business, etc...).
Let me know what you think.

The larger Q is what to do w/ playback rate changes in general. i.e., would it
make sense for them to be a parameter to Preroll()?

+  VolumeChangedTask(GetVolume());
+
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Play, audio_renderer_));
 
   if (video_renderer_)
     bound_fns.Push(base::Bind(&VideoRenderer::Play, video_renderer_));
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
 
 void Pipeline::DoStop(const PipelineStatusCB& done_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(!pending_callbacks_.get());
   SerialRunner::Queue bound_fns;
 
   if (demuxer_)
     bound_fns.Push(base::Bind(&Demuxer::Stop, demuxer_));
 
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Stop, audio_renderer_));
 
   if (video_renderer_)
     bound_fns.Push(base::Bind(&VideoRenderer::Stop, video_renderer_));
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
 
+void Pipeline::OnStopCompleted(PipelineStatus status) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kStopping);
+  SetState(kStopped);
+
+  pending_callbacks_.reset();

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

Shouldn't you be able to DCHECK that this already IsNull()?

[scherkus (not reviewing), 2012/09/05 15:04:17]

No -- pending_callbacks_ is non-NULL but has completed (i.e. it doesn't
magically self delete itself after completing)

+  filter_collection_.reset();
+  audio_decoder_ = NULL;
+  audio_renderer_ = NULL;
+  video_renderer_ = NULL;
+  demuxer_ = NULL;
+  {
+    base::AutoLock l(lock_);
+    running_ = false;

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

You sure you don't want to do this first?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Not sure, but seems reasonable!

+  }
+
+  if (!seek_cb_.is_null()) {
+    base::ResetAndReturn(&seek_cb_).Run(status_);
+    error_cb_.Reset();

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

nit: early return here and below & de-indent the error_cb case?

[scherkus (not reviewing), 2012/09/05 15:04:17]

If we call Stop() during initialization/seek we'll actually want to fire both
seek_cb_ followed by stop_cb_

Comment added.

+  }
+  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_);
   buffered_byte_ranges_.Add(start, end);
   did_loading_progress_ = true;
 }
 
 void Pipeline::AddBufferedTimeRange(base::TimeDelta start,
                                     base::TimeDelta end) {
   DCHECK(IsRunning());
@@ skipping 19 matching lines @@
 }
 
 void Pipeline::OnVideoRendererEnded() {
   // Force post to process ended messages after current execution frame.
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::DoVideoRendererEnded, this));
   media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::VIDEO_ENDED));
 }
 
 // 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;
   statistics_.video_bytes_decoded += stats.video_bytes_decoded;
   statistics_.video_frames_decoded += stats.video_frames_decoded;
   statistics_.video_frames_dropped += stats.video_frames_dropped;
 }
 
 void Pipeline::StartTask(scoped_ptr<FilterCollection> filter_collection,
                          const PipelineStatusCB& ended_cb,
                          const PipelineStatusCB& error_cb,
                          const PipelineStatusCB& seek_cb,
                          const BufferingStateCB& buffering_state_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   CHECK_EQ(kCreated, state_)
       << "Media pipeline cannot be started more than once";
 
   filter_collection_ = filter_collection.Pass();
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
   seek_cb_ = seek_cb;
   buffering_state_cb_ = buffering_state_cb;
 
-  // Kick off initialization.
-  pipeline_init_state_.reset(new PipelineInitState());
-
-  SetState(kInitDemuxer);
-  InitializeDemuxer();
+  DoStateTransition(PIPELINE_OK);
 }
 
-// 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));
-  }
-}
-
-// 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());
 
   if (state_ == kStopped) {
     stop_cb.Run();
     return;
   }
 
+#if 0
+  // TODO(scherkus): Can I remove this?!

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

Not before testing it!  (and the CL that removes the call to it should also
remove the function in VRB & GVD).

I can't test this for you until I get back to SEA, unfortch.
Probably want to hold off on this part of the change for now.

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

No. The GPU video decoder & WebRTC decoders need this to avoid blocking the
pipeline on shutdown.

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

   if (video_renderer_)
     video_renderer_->PrepareForShutdownHack();
+#endif
 
-  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;

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

I'm pretty sure you could get rid of stop_cb_ if you just included stop_cb in
the bind below. It might also simplify OnStopCompleted()

[scherkus (not reviewing), 2012/09/05 15:04:17]

It's admittedly used in a few spots for DCHECK-ing so I'd like to keep it around
for the time being

 
-  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));
 }
 
 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_)
+  if (state_ == kStopping || state_ == kStopped)

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

This can be removed since it is covered by the condition  below this.

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

     return;
 
-  // Suppress rate change until after seeking.
-  if (IsPipelineSeeking()) {
-    pending_playback_rate_ = playback_rate;
-    playback_rate_change_pending_ = true;
+  // Playback rate changes are only carried out while playing.

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

We no longer track pending PR changes?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Since playback_rate_ is set under lock there's no need to cache a copy of the
pending value on the pipeline thread

Instead we now set the PR and Volume prior to calling Play() on everyone
regardless of whether the value changed (see DoPlay() and gmock expectation
changes in pipeline_unittest).

+  if (state_ != kStarting && state_ != kStarted)
     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_)
     audio_renderer_->SetPlaybackRate(playback_rate_);
   if (video_renderer_)
     video_renderer_->SetPlaybackRate(playback_rate_);
 }
 
 void Pipeline::VolumeChangedTask(float volume) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  if (state_ == kStopped || tearing_down_)
+  if (state_ == kStopping || state_ == kStopped)

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

ditto. covered by check below

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

+    return;
+
+  // Volume changes are only carried out while playing.
+  if (state_ != kStarting && state_ != kStarted)
     return;
 
   if (audio_renderer_)
     audio_renderer_->SetVolume(volume);
 }
 
 void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(stop_cb_.is_null());
 
   // Suppress seeking if we're not fully started.
   if (state_ != kStarted) {

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

Add DCHECK(state_ == kStopping || state_ == kStopped)?  These seem like the only
states where this would not be a bug.

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

     // 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 "
              << time.InMicroseconds() << " (current state: " << state_ << ")";
     return;
   }
 

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

DCHECK_EQ(state_, kStarted) ?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Hrmm.. unlike seek_cb_ it doesn't seem to be a high-value DCHECK considering
we've got the if statement right above

-  DCHECK(!seek_pending_);
-  seek_pending_ = true;
+  DCHECK(seek_cb_.is_null());
 
-  // 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);
+  SetState(kSeeking);
+  base::TimeDelta seek_timestamp = std::max(time, demuxer_->GetStartTime());

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

Should this also be capped by duration?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Today we rely on demuxers to cap the seek -- I'd rather not slip that change in
w/ this CL

+  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() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (state_ != kStarted)
     return;
 
   DCHECK(!audio_ended_);
   audio_ended_ = true;
@@ skipping 27 matching lines @@
     return;
 
   if (video_renderer_ && !video_ended_)
     return;
 
   {
     base::AutoLock auto_lock(lock_);
     clock_->EndOfStream();
   }
 
-  ReportStatus(ended_cb_, status_);
+  ended_cb_.Run(status_);

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

Are there plans to change this to a closure? It seems like status_ ==
PIPELINE_OK here all the time.
DCHECK_EQ(state_, PIPELINE_OK) for now?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

 }
 
 void Pipeline::AudioDisabledTask() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   base::AutoLock auto_lock(lock_);
   has_audio_ = false;
   audio_disabled_ = true;
 
   // 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_) {
+  demuxer_ = filter_collection_->GetDemuxer();
+  if (!demuxer_) {

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

DCHECK(demuxer_)? I can't think of a case where this would fail and it isn't the
sign of a coding error.

[scherkus (not reviewing), 2012/09/05 15:04:17]

Done.

+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return;
   }
 
-  if (!TransientState(state_)) {
-    NOTREACHED() << "Invalid current state: " << state_;
-    SetError(PIPELINE_ERROR_ABORT);
+  demuxer_->Initialize(this, done_cb);
+}
+
+void Pipeline::InitializeAudioDecoder(const PipelineStatusCB& done_cb) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  scoped_refptr<DemuxerStream> stream =
+      demuxer_->GetStream(DemuxerStream::AUDIO);
+
+  if (!stream) {
+    done_cb.Run(PIPELINE_OK);
     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_);
-  }
+  filter_collection_->SelectAudioDecoder(&audio_decoder_);
 
-  // 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_;
-  }
-}
-
-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() {
-  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());
-
-  demuxer_ = filter_collection_->GetDemuxer();
-  if (!demuxer_) {
-    SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
+  if (!audio_decoder_) {
+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return;
   }
 
-  demuxer_->Initialize(this, base::Bind(&Pipeline::OnDemuxerInitialized, this));
+  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));
+void Pipeline::InitializeAudioRenderer(const PipelineStatusCB& done_cb) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (!audio_decoder_) {

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

nit: seems strange that this check is here instead of in the caller.  

[scherkus (not reviewing), 2012/09/05 15:04:17]

I wanted to keep the state machine switch statement simpler.

I suppose this can be a DCHECK() and GetNextState() could inspect for the
presence of audio_decoder_ to skip over this state.

Hmmmmmmmmmmm

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

Yes, that's what I meant.

+    done_cb.Run(PIPELINE_OK);
     return;
   }
 
-  if (status != PIPELINE_OK) {
-    SetError(status);
+  filter_collection_->SelectAudioRenderer(&audio_renderer_);
+  if (!audio_renderer_) {

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

Should we DCHECK() instead?

[scherkus (not reviewing), 2012/09/05 15:04:17]

Maybe. IIRC today we require a demuxer (i.e., otherwise we'll crash)

Do we want to have the same requirement for all filters? (I'm fine w/ that -- we
can likely delete the error code too)

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

Yes please.

+    done_cb.Run(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     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);
-}
-
-bool Pipeline::InitializeAudioDecoder(
-    const scoped_refptr<Demuxer>& demuxer) {
-  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;
-  }
-
-  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;
-  }
-
   audio_renderer_->Initialize(
-      decoder,
-      base::Bind(&Pipeline::OnFilterInitialize, this),
+      audio_decoder_,

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

std::swap() to a local variable here so we can clear Pipeline's reference as
early as possible?

[scherkus (not reviewing), 2012/09/05 15:04:17]

I think it's OK for now -- member variable will go away in
https://chromiumcodereview.appspot.com/10918022/

+      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) {

[Ami GONE FROM CHROMIUM, 2012/09/02 20:29:29]

ditto

+    done_cb.Run(PIPELINE_OK);
+    return;
+  }
 
   filter_collection_->SelectVideoRenderer(&video_renderer_);
   if (!video_renderer_) {

[acolwell GONE FROM CHROMIUM, 2012/09/05 13:22:22]

Should we put a DCHECK() here since this failing is likely a coding error?

-    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),
       base::Bind(&Pipeline::OnVideoRendererEnded, this),
       base::Bind(&Pipeline::SetError, this),
       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() {
   if (!message_loop_->BelongsToCurrentThread()) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &Pipeline::OnAudioUnderflow, this));
     return;
   }
 
   if (state_ != kStarted)
     return;
 
   if (audio_renderer_)
     audio_renderer_->ResumeAfterUnderflow(true);
 }
 
 void Pipeline::StartClockIfWaitingForTimeUpdate_Locked() {
   lock_.AssertAcquired();
   if (!waiting_for_clock_update_)
     return;
 
   waiting_for_clock_update_ = false;
   clock_->Play();
 }
 
 }  // namespace media