Clean up Pipeline terminal state logic by merging kError into kStopped state.

media/base/pipeline.cc

Index: media/base/pipeline.cc
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 92f0199bd6a675b10b8956ef2aab8f0e7e73ce38..7ba148c4a7ec8318e69b3cd7c2634fd9ed21059e 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -71,9 +71,7 @@ Pipeline::Pipeline(MessageLoop* message_loop, MediaLog* media_log)
       media_log_(media_log),
       running_(false),
       seek_pending_(false),
-      stop_pending_(false),
       tearing_down_(false),
-      error_caused_teardown_(false),
       playback_rate_change_pending_(false),
       did_loading_progress_(false),
       total_bytes_(0),
@@ -98,7 +96,7 @@ Pipeline::Pipeline(MessageLoop* message_loop, MediaLog* media_log)
 Pipeline::~Pipeline() {
   base::AutoLock auto_lock(lock_);
   DCHECK(!running_) << "Stop() must complete before destroying object";
-  DCHECK(!stop_pending_);
+  DCHECK(stop_cb_.is_null());
   DCHECK(!seek_pending_);
 
   media_log_->AddEvent(
@@ -119,10 +117,6 @@ void Pipeline::Start(scoped_ptr<FilterCollection> collection,
 }
 
 void Pipeline::Stop(const base::Closure& stop_cb) {
-  base::AutoLock auto_lock(lock_);
-  CHECK(running_) << "Media pipeline isn't running";
-
-  // Stop the pipeline, which will set |running_| to false on our behalf.
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::StopTask, this, stop_cb));
 }
@@ -140,23 +134,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:
-      return true;
-    default:
-      return false;
-  }
-}
-
 bool Pipeline::HasAudio() const {
   base::AutoLock auto_lock(lock_);
   return has_audio_;
@@ -276,7 +253,7 @@ bool Pipeline::IsPipelineOk() {
 
 bool Pipeline::IsPipelineStopped() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  return state_ == kStopped || state_ == kError;
+  return state_ == kStopped;
 }
 
 bool Pipeline::IsPipelineTearingDown() {
@@ -286,7 +263,7 @@ bool Pipeline::IsPipelineTearingDown() {
 
 bool Pipeline::IsPipelineStopPending() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  return stop_pending_;
+  return !stop_cb_.is_null();
 }
 
 bool Pipeline::IsPipelineSeeking() {
@@ -343,7 +320,7 @@ Pipeline::State Pipeline::FindNextState(State current) {
   } else if (current == kStarting) {
     return kStarted;
   } else if (current == kStopping) {
-    return error_caused_teardown_ ? kError : kStopped;
+    return kStopped;
   } else {
     return current;
   }
@@ -689,36 +666,32 @@ void Pipeline::InitializeTask(PipelineStatus last_stage_status) {
   }
 }
 
-// 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 (state_ == kStopped) {
+    stop_cb.Run();
+    return;
+  }
 
   if (video_decoder_) {
     video_decoder_->PrepareForShutdownHack();
     video_decoder_ = NULL;
   }
 
-  if (IsPipelineTearingDown() && error_caused_teardown_) {
+  if (IsPipelineTearingDown() && 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;
-    error_caused_teardown_ = false;
   }
 
   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
@@ -741,8 +714,6 @@ void Pipeline::ErrorChangedTask(PipelineStatus error) {
   base::AutoLock auto_lock(lock_);
   status_ = error;
 
-  error_caused_teardown_ = true;
-
   // 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
@@ -962,7 +933,7 @@ void Pipeline::TeardownStateTransitionTask() {
 
   switch (state_) {
     case kStopping:
-      SetState(error_caused_teardown_ ? kError : kStopped);
+      SetState(kStopped);
       FinishDestroyingFiltersTask();
       break;
     case kPausing:
@@ -975,7 +946,6 @@ void Pipeline::TeardownStateTransitionTask() {
       break;
 
     case kCreated:
-    case kError:
     case kInitDemuxer:
     case kInitAudioDecoder:
     case kInitAudioRenderer:
@@ -999,23 +969,20 @@ void Pipeline::FinishDestroyingFiltersTask() {
   audio_renderer_ = NULL;
   video_renderer_ = NULL;
   demuxer_ = NULL;
+  {
+    base::AutoLock l(lock_);
+    running_ = false;
+  }
 
-  if (error_caused_teardown_ && !IsPipelineOk() && !error_cb_.is_null())
-    error_cb_.Run(status_);
-
-  if (stop_pending_) {
-    stop_pending_ = false;
-    {
-      base::AutoLock l(lock_);
-      running_ = false;
-    }
-
-    // Notify the client that stopping has finished.
+  // We prioritize executing stop callbacks over error callbacks if they're
+  // present.
+  if (!stop_cb_.is_null()) {
     base::ResetAndReturn(&stop_cb_).Run();
+  } else if (!error_cb_.is_null() && status_ != PIPELINE_OK) {
+    error_cb_.Run(status_);
   }
 
   tearing_down_ = false;
-  error_caused_teardown_ = false;
 }
 
 void Pipeline::InitializeDemuxer() {
@@ -1163,9 +1130,13 @@ void Pipeline::TearDownPipeline() {
   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.
+  // 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
+  DCHECK(!tearing_down_ && !stop_cb_.is_null() ||
+         (tearing_down_ && status_ != PIPELINE_OK) ||
+         (tearing_down_ && !stop_cb_.is_null()));
 
   // Mark that we already start tearing down operation.
   tearing_down_ = true;
@@ -1175,7 +1146,6 @@ void Pipeline::TearDownPipeline() {
 
   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.