Remove Pipeline::IsInitialized() and replace stop_pending_ with checks for stop_cb_.

Unfortunately until bug 110228 is fixed I have to keep IsInitializedForTesting() around but atleast it enforces that state_ is accessed on the same thread instead of locking.

Review URL: https://chromiumcodereview.appspot.com/10823261

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150899 0039d316-1c4b-4281-b951-d872f2087c98

media/base/pipeline.cc

@@ -71,7 +71,6 @@ 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),
@@ -100,7 +99,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(
@@ -142,23 +141,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_;
@@ -256,6 +238,21 @@ PipelineStatistics Pipeline::GetStatistics() const {
   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);
 }
@@ -290,11 +287,6 @@ bool Pipeline::IsPipelineTearingDown() {
   return tearing_down_;
 }
 
-bool Pipeline::IsPipelineStopPending() {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  return stop_pending_;
-}
-
 bool Pipeline::IsPipelineSeeking() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   if (!seek_pending_)
@@ -342,7 +334,7 @@ Pipeline::State Pipeline::FindNextState(State current) {
     // We will always honor Seek() before Stop(). This is based on the
     // assumption that we never accept Seek() after Stop().
     DCHECK(IsPipelineSeeking() ||
-           IsPipelineStopPending() ||
+           !stop_cb_.is_null() ||
            IsPipelineTearingDown());
     return IsPipelineSeeking() ? kSeeking : kStopping;
   } else if (current == kSeeking) {
@@ -632,7 +624,7 @@ void Pipeline::InitializeTask(PipelineStatus last_stage_status) {
   }
 
   // If we have received the stop or error signal, return immediately.
-  if (IsPipelineStopPending() || IsPipelineStopped() || !IsPipelineOk())
+  if (!stop_cb_.is_null() || IsPipelineStopped() || !IsPipelineOk())
     return;
 
   DCHECK(state_ == kInitDemuxer ||
@@ -713,7 +705,7 @@ void Pipeline::InitializeTask(PipelineStatus last_stage_status) {
 // 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(stop_cb_.is_null());
   DCHECK_NE(state_, kStopped);
 
   if (video_decoder_) {
@@ -733,7 +725,6 @@ void Pipeline::StopTask(const base::Closure& stop_cb) {
 
   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
@@ -806,7 +797,7 @@ void Pipeline::VolumeChangedTask(float volume) {
 
 void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(!IsPipelineStopPending());
+  DCHECK(stop_cb_.is_null());
 
   // Suppress seeking if we're not fully started.
   if (state_ != kStarted) {
@@ -976,8 +967,8 @@ void Pipeline::FilterStateTransitionTask() {
       StartClockIfWaitingForTimeUpdate_Locked();
     }
 
-    if (IsPipelineStopPending()) {
-      // We had a pending stop request need to be honored right now.
+    // Check if we have a pending stop request that needs to be honored.
+    if (!stop_cb_.is_null()) {
       TearDownPipeline();
     }
   } else {
@@ -1034,8 +1025,7 @@ void Pipeline::FinishDestroyingFiltersTask() {
   if (error_caused_teardown_ && !IsPipelineOk() && !error_cb_.is_null())
     error_cb_.Run(status_);
 
-  if (stop_pending_) {
-    stop_pending_ = false;
+  if (!stop_cb_.is_null()) {
     {
       base::AutoLock l(lock_);
       running_ = false;
@@ -1194,9 +1184,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 during that time
+  DCHECK(!tearing_down_ ||
+         (tearing_down_ && error_caused_teardown_) ||
+         (tearing_down_ && !stop_cb_.is_null()));
 
   // Mark that we already start tearing down operation.
   tearing_down_ = true;

media/base/pipeline.h

@@ -83,7 +83,6 @@ class MEDIA_EXPORT PipelineStatusNotification {
 //                                                        ^  SetError()
 //                                                        |
 //                                         [ Any State Other Than InitXXX ]
-
 //
 // Initialization is a series of state transitions from "Created" through each
 // filter initialization state.  When all filter initialization states have
@@ -103,24 +102,15 @@ class MEDIA_EXPORT Pipeline
   Pipeline(MessageLoop* message_loop, MediaLog* media_log);
 
   // Build a pipeline to using the given filter collection to construct a filter
-  // chain.
-  //
-  // Pipeline initialization is an inherently asynchronous process.  Clients can
-  // either poll the IsInitialized() method (discouraged) or optionally pass in
-  // |start_cb|, which will be executed when initialization completes.
+  // chain, executing |start_cb| when initialization has completed.
   //
-  // The following permanent callbacks will be executed as follows:
-  //   |start_cb_| will be executed when Start is done (successfully or not).
+  // The following permanent callbacks will be executed as follows up until
+  // Stop() has completed:
   //   |ended_cb| will be executed whenever the media reaches the end.
-  //   |error_cb_| will be executed whenever an error occurs but hasn't
+  //   |error_cb| will be executed whenever an error occurs but hasn't
   //              been reported already through another callback.
   //
-  // These callbacks are only executed after Start() has been called and until
-  // Stop() has completed.
-  //
   // It is an error to call this method after the pipeline has already started.
-  //
-  // TODO(scherkus): remove IsInitialized() and force clients to use callbacks.
   void Start(scoped_ptr<FilterCollection> filter_collection,
              const PipelineStatusCB& ended_cb,
              const PipelineStatusCB& error_cb,
@@ -155,11 +145,6 @@ class MEDIA_EXPORT Pipeline
   // the pipeline.
   bool IsRunning() const;
 
-  // Returns true if the pipeline has been started and fully initialized to a
-  // point where playback controls will be respected.  Note that it is possible
-  // for a pipeline to be started but not initialized (i.e., an error occurred).
-  bool IsInitialized() const;
-
   // Returns true if the media has audio.
   bool HasAudio() const;
 
@@ -216,6 +201,9 @@ class MEDIA_EXPORT Pipeline
   // Gets the current pipeline statistics.
   PipelineStatistics GetStatistics() const;
 
+  // TODO(scherkus): Remove IsInitializedForTesting() after stop/error teardown
+  // paths are sane, see http://crbug.com/110228
+  bool IsInitializedForTesting();
   void SetClockForTesting(Clock* clock);
   void SetErrorForTesting(PipelineStatus status);
 
@@ -261,9 +249,6 @@ class MEDIA_EXPORT Pipeline
   // Helper method to tell whether we are in transition to stop state.
   bool IsPipelineTearingDown();
 
-  // We could also be delayed by a transition during seek is performed.
-  bool IsPipelineStopPending();
-
   // Helper method to tell whether we are in transition to seek state.
   bool IsPipelineSeeking();
 
@@ -447,9 +432,6 @@ class MEDIA_EXPORT Pipeline
   // Whether or not the pipeline is in transition for a seek operation.
   bool seek_pending_;
 
-  // Whether or not the pipeline is pending a stop operation.
-  bool stop_pending_;
-
   // Whether or not the pipeline is perform a stop operation.
   bool tearing_down_;
 

media/base/pipeline_unittest.cc

@@ -103,7 +103,12 @@ class PipelineTest : public ::testing::Test {
     }
 
     // Shutdown sequence.
-    if (pipeline_->IsInitialized()) {
+    //
+    // TODO(scherkus): This check is required because in certain teardown
+    // cases the pipeline is still "running" but has already stopped due to
+    // errors. In an ideal world we stop running when we teardown, but that
+    // requires cleaning up shutdown path, see http://crbug.com/110228
+    if (pipeline_->IsInitializedForTesting()) {
       EXPECT_CALL(*mocks_->demuxer(), Stop(_))
           .WillOnce(RunClosure());
 
@@ -319,7 +324,6 @@ TEST_F(PipelineTest, NotStarted) {
   const base::TimeDelta kZero;
 
   EXPECT_FALSE(pipeline_->IsRunning());
-  EXPECT_FALSE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());
   EXPECT_FALSE(pipeline_->HasVideo());
 
@@ -366,7 +370,6 @@ TEST_F(PipelineTest, NeverInitializes) {
         base::Bind(&CallbackHelper::OnStart, base::Unretained(&callbacks_)));
   message_loop_.RunAllPending();
 
-  EXPECT_FALSE(pipeline_->IsInitialized());
 
   // Because our callback will get executed when the test tears down, we'll
   // verify that nothing has been called, then set our expectation for the call
@@ -387,7 +390,6 @@ TEST_F(PipelineTest, RequiredFilterMissing) {
       base::Bind(&CallbackHelper::OnError, base::Unretained(&callbacks_)),
       base::Bind(&CallbackHelper::OnStart, base::Unretained(&callbacks_)));
   message_loop_.RunAllPending();
-  EXPECT_FALSE(pipeline_->IsInitialized());
 }
 
 TEST_F(PipelineTest, URLNotFound) {
@@ -397,7 +399,6 @@ TEST_F(PipelineTest, URLNotFound) {
       .WillOnce(RunClosure());
 
   InitializePipeline(PIPELINE_ERROR_URL_NOT_FOUND);
-  EXPECT_FALSE(pipeline_->IsInitialized());
 }
 
 TEST_F(PipelineTest, NoStreams) {
@@ -407,7 +408,6 @@ TEST_F(PipelineTest, NoStreams) {
       .WillOnce(RunClosure());
 
   InitializePipeline(PIPELINE_ERROR_COULD_NOT_RENDER);
-  EXPECT_FALSE(pipeline_->IsInitialized());
 }
 
 TEST_F(PipelineTest, AudioStream) {
@@ -420,7 +420,6 @@ TEST_F(PipelineTest, AudioStream) {
   InitializeAudioRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_TRUE(pipeline_->HasAudio());
   EXPECT_FALSE(pipeline_->HasVideo());
 }
@@ -435,7 +434,6 @@ TEST_F(PipelineTest, VideoStream) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 }
@@ -454,7 +452,6 @@ TEST_F(PipelineTest, AudioVideoStream) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_TRUE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 }
@@ -510,7 +507,6 @@ TEST_F(PipelineTest, Properties) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_EQ(kDuration.ToInternalValue(),
             pipeline_->GetMediaDuration().ToInternalValue());
   EXPECT_EQ(kTotalBytes, pipeline_->GetTotalBytes());
@@ -528,7 +524,6 @@ TEST_F(PipelineTest, GetBufferedTimeRanges) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
 
   EXPECT_EQ(0u, pipeline_->GetBufferedTimeRanges().size());
 
@@ -584,7 +579,6 @@ TEST_F(PipelineTest, DisableAudioRenderer) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_TRUE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 
@@ -613,7 +607,6 @@ TEST_F(PipelineTest, DisableAudioRendererDuringInit) {
               OnAudioRendererDisabled());
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 
@@ -808,7 +801,6 @@ TEST_F(PipelineTest, StartTimeIsZero) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 
@@ -831,7 +823,6 @@ TEST_F(PipelineTest, StartTimeIsNonZero) {
   InitializeVideoRenderer();
 
   InitializePipeline(PIPELINE_OK);
-  EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 
@@ -905,7 +896,6 @@ TEST_F(PipelineTest, InitFailure_Demuxer) {
   EXPECT_CALL(*mocks_->demuxer(), Stop(_))
       .WillOnce(RunClosure());
   InitializePipeline(expected_status);
-  EXPECT_FALSE(pipeline_->IsInitialized());
 }
 
 TEST_F(PipelineTest, InitFailure_AudioDecoder) {
@@ -924,7 +914,6 @@ TEST_F(PipelineTest, InitFailure_AudioDecoder) {
       .WillOnce(RunClosure());
 
   InitializePipeline(expected_status);
-  EXPECT_FALSE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());
 }
 
@@ -948,7 +937,6 @@ TEST_F(PipelineTest, InitFailure_AudioRenderer) {
       .WillOnce(RunClosure());
 
   InitializePipeline(expected_status);
-  EXPECT_FALSE(pipeline_->IsInitialized());
   EXPECT_TRUE(pipeline_->HasAudio());
 }
 
@@ -975,7 +963,6 @@ TEST_F(PipelineTest, InitFailure_VideoDecoder) {
       .WillOnce(RunClosure());
 
   InitializePipeline(expected_status);
-  EXPECT_FALSE(pipeline_->IsInitialized());
   EXPECT_TRUE(pipeline_->HasAudio());
   EXPECT_FALSE(pipeline_->HasVideo());
 }
@@ -1006,7 +993,6 @@ TEST_F(PipelineTest, InitFailure_VideoRenderer) {
       .WillOnce(RunClosure());
 
   InitializePipeline(expected_status);
-  EXPECT_FALSE(pipeline_->IsInitialized());
   EXPECT_TRUE(pipeline_->HasAudio());
   EXPECT_TRUE(pipeline_->HasVideo());
 }