Merge Pipeline's kError state with kStopped: a baby step towards bringing sanity to 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"
@@ skipping 54 matching lines @@
   scoped_refptr<AudioDecoder> audio_decoder;
   scoped_refptr<VideoDecoder> video_decoder;
 };
 
 Pipeline::Pipeline(MessageLoop* message_loop, MediaLog* media_log)
     : message_loop_(message_loop->message_loop_proxy()),
       media_log_(media_log),
       running_(false),
       seek_pending_(false),
       tearing_down_(false),
-      error_caused_teardown_(false),

[scherkus (not reviewing), 2012/08/09 19:26:53]

this is synonymous with (status_ != PIPELINE_OK && tearing_down_)

       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),
@@ skipping 28 matching lines @@
   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));
 }
 
 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));
 }
 
 void Pipeline::Seek(TimeDelta time, const PipelineStatusCB& seek_cb) {
   base::AutoLock auto_lock(lock_);
   CHECK(running_) << "Media pipeline isn't running";
 
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::SeekTask, this, time, seek_cb));
@@ skipping 94 matching lines @@
   bool ret = did_loading_progress_;
   did_loading_progress_ = false;
   return ret;
 }
 
 PipelineStatistics Pipeline::GetStatistics() const {
   base::AutoLock auto_lock(lock_);
   return statistics_;
 }
 
-bool Pipeline::IsInitializedForTesting() {
-  DCHECK(message_loop_->BelongsToCurrentThread())
-      << "Tests should run on the same thread as Pipeline";
-  switch (state_) {
-    case kPausing:
-    case kFlushing:
-    case kSeeking:
-    case kStarting:
-    case kStarted:
-      return true;
-    default:
-      return false;
-  }
-}
-
 void Pipeline::SetClockForTesting(Clock* clock) {
   clock_.reset(clock);
 }
 
 void Pipeline::SetErrorForTesting(PipelineStatus status) {
   SetError(status);
 }
 
 void Pipeline::SetState(State next_state) {
   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;
 }
 
-bool Pipeline::IsPipelineStopped() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  return state_ == kStopped || state_ == kError;
-}
-
-bool Pipeline::IsPipelineTearingDown() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  return tearing_down_;
-}
-
 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;
 }
 
@@ skipping 28 matching lines @@
 // 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() ||
-           IsPipelineTearingDown());
+           tearing_down_);
     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;
+    return kStopped;
   } else {
     return current;
   }
 }
 
 void Pipeline::OnDemuxerError(PipelineStatus error) {
   SetError(error);
 }
 
 void Pipeline::SetError(PipelineStatus error) {
@@ skipping 261 matching lines @@
     if (state_ == kInitVideoDecoder &&
         last_stage_status == DECODER_ERROR_NOT_SUPPORTED) {
       state_ = kInitAudioRenderer;
     } else {
       SetError(last_stage_status);
       return;
     }
   }
 
   // If we have received the stop or error signal, return immediately.
-  if (!stop_cb_.is_null() || IsPipelineStopped() || !IsPipelineOk())
+  if (!stop_cb_.is_null() || state_ == kStopped || !IsPipelineOk())
     return;
 
   DCHECK(state_ == kInitDemuxer ||
          state_ == kInitAudioDecoder ||
          state_ == kInitAudioRenderer ||
          state_ == kInitVideoDecoder ||
          state_ == kInitVideoRenderer);
 
   // Demuxer created, create audio decoder.
   if (state_ == kInitDemuxer) {
@@ skipping 61 matching lines @@
 // 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());
-  DCHECK_NE(state_, kStopped);
+
+  if (state_ == kStopped) {
+    stop_cb.Run();
+    return;
+  }
 
   if (video_decoder_) {
     video_decoder_->PrepareForShutdownHack();
     video_decoder_ = NULL;
   }
 
-  if (IsPipelineTearingDown() && error_caused_teardown_) {
+  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.

[Ami GONE FROM CHROMIUM, 2012/08/09 20:55:41]

This seems crazy.   Why do we want this?

[scherkus (not reviewing), 2012/08/09 22:07:49]

It _is_ a bit crazy but not _that_ crazy -- it basically suppresses the error_cb
from firing since we're trying to stop_cb

Check out my original brainstorming CL on this matter -- it's an open question
as to which CBs should fire:
http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode655

     base::AutoLock auto_lock(lock_);
     status_ = PIPELINE_OK;
-    error_caused_teardown_ = false;
   }
 
   stop_cb_ = stop_cb;
 
-  if (!IsPipelineSeeking() && !IsPipelineTearingDown()) {
+  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();
   }
 }
 
 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()) {
+  if (state_ == kStopped || tearing_down_) {
     return;
   }
 
   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
-  // don't modify the state before TeadDownPipeline() can run.
+  // don't modify the state before TearDownPipeline() can run.
   tearing_down_ = true;
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::TearDownPipeline, this));
 }
 
 void Pipeline::PlaybackRateChangedTask(float playback_rate) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (!running_ || tearing_down_)

[Ami GONE FROM CHROMIUM, 2012/08/09 20:55:41]

This (and other places) read running_ not under lock.  Is that safe?

[scherkus (not reviewing), 2012/08/09 22:07:49]

probably not!

     return;
 
   // Suppress rate change until after seeking.
   if (IsPipelineSeeking()) {
     pending_playback_rate_ = playback_rate;
     playback_rate_change_pending_ = true;
     return;
   }
 
   {
@@ skipping 121 matching lines @@
   clock_->SetMaxTime(clock_->Duration());
   StartClockIfWaitingForTimeUpdate_Locked();
 }
 
 void Pipeline::FilterStateTransitionTask() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(pending_callbacks_.get())
       << "Filter state transitions must be completed via pending_callbacks_";
   pending_callbacks_.reset();
 
-  // No reason transitioning if we've errored or have stopped.
-  if (IsPipelineStopped()) {
+  // No reason transitioning if we're stopped or tearing down as teardown

[Ami GONE FROM CHROMIUM, 2012/08/09 20:55:41]

English this up.

[scherkus (not reviewing), 2012/08/09 22:07:49]

Done.

+  // state transitions are handled via TeardownStateTransitionTask().
+  //
+  // TODO(scherkus): Merge all state machinery!
+  if (state_ == kStopped || tearing_down_) {
     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);
     return;
   }
 
   // Decrement the number of remaining transitions, making sure to transition
   // to the next state if needed.
   SetState(FindNextState(state_));
   if (state_ == kSeeking) {
@@ skipping 43 matching lines @@
     // 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(IsPipelineTearingDown());
+  DCHECK(tearing_down_);
   DCHECK(pending_callbacks_.get())
       << "Teardown state transitions must be completed via pending_callbacks_";
   pending_callbacks_.reset();
 
   switch (state_) {
     case kStopping:
-      SetState(error_caused_teardown_ ? kError : kStopped);
+      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 kError:
     case kInitDemuxer:
     case kInitAudioDecoder:
     case kInitAudioRenderer:
     case kInitVideoDecoder:
     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(IsPipelineStopped());
+  DCHECK_EQ(state_, kStopped);
 
   audio_renderer_ = NULL;
   video_renderer_ = NULL;
   demuxer_ = NULL;
+  tearing_down_ = false;
+  {
+    base::AutoLock l(lock_);
+    running_ = false;
+  }
 
-  if (error_caused_teardown_ && !IsPipelineOk() && !error_cb_.is_null())
+  if (!IsPipelineOk() && !error_cb_.is_null())
     error_cb_.Run(status_);
 
-  if (!stop_cb_.is_null()) {
-    {
-      base::AutoLock l(lock_);
-      running_ = false;
-    }
-
-    // Notify the client that stopping has finished.
+  if (!stop_cb_.is_null())
     base::ResetAndReturn(&stop_cb_).Run();
-  }
-
-  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);
     return;
@@ skipping 132 matching lines @@
 
 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_ && error_caused_teardown_) ||
+         (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:
-    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:
@@ skipping 79 matching lines @@
 void Pipeline::StartClockIfWaitingForTimeUpdate_Locked() {
   lock_.AssertAcquired();
   if (!waiting_for_clock_update_)
     return;
 
   waiting_for_clock_update_ = false;
   clock_->Play();
 }
 
 }  // namespace media