Prevent pending callbacks after an error from being interpreted as teardown callbacks.

- Moved teardown state transitions to use TeardownStateTransitionTask() so they can't be confused with normal playback transitions in FilterStateTransitionTask().
- Fixed code so seek callback is called if an error occurs during a Seek().

BUG=71087
TEST=PipelineImplTest.ErrorDuringSeek

Review URL: http://codereview.chromium.org/6250092

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

3 files changed, 120 insertions, 13 deletions

diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index e096b22..a0fda64 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -346,7 +346,8 @@ bool PipelineImpl::IsPipelineSeeking() {
   if (!seek_pending_)
     return false;
   DCHECK(kSeeking == state_ || kPausing == state_ ||
-         kFlushing == state_ || kStarting == state_);
+         kFlushing == state_ || kStarting == state_)
+      << "Current state : " << state_;
   return true;
 }
 
@@ -520,11 +521,15 @@ void PipelineImpl::OnFilterInitialize() {
 
 // Called from any thread.
 void PipelineImpl::OnFilterStateTransition() {
-  // Continue walking down the filters.
   message_loop_->PostTask(FROM_HERE,
       NewRunnableMethod(this, &PipelineImpl::FilterStateTransitionTask));
 }
 
+void PipelineImpl::OnTeardownStateTransition() {
+  message_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &PipelineImpl::TeardownStateTransitionTask));
+}
+
 void PipelineImpl::StartTask(FilterCollection* filter_collection,
                              const std::string& url,
                              PipelineCallback* start_callback) {
@@ -715,7 +720,14 @@ void PipelineImpl::ErrorChangedTask(PipelineError error) {
   error_ = error;
 
   error_caused_teardown_ = true;
-  TearDownPipeline();
+
+  // 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,
+      NewRunnableMethod(this, &PipelineImpl::TearDownPipeline));
 }
 
 void PipelineImpl::PlaybackRateChangedTask(float playback_rate) {
@@ -849,6 +861,12 @@ void PipelineImpl::FilterStateTransitionTask() {
     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);
@@ -903,13 +921,49 @@ void PipelineImpl::FilterStateTransitionTask() {
       // We had a pending stop request need to be honored right now.
       TearDownPipeline();
     }
-  } else if (IsPipelineStopped()) {
-    FinishDestroyingFiltersTask();
   } else {
     NOTREACHED() << "Unexpected state: " << state_;
   }
 }
 
+void PipelineImpl::TeardownStateTransitionTask() {
+  DCHECK(IsPipelineTearingDown());
+  switch(state_) {
+    case kStopping:
+      set_state(error_caused_teardown_ ? kError : kStopped);
+      FinishDestroyingFiltersTask();
+      break;
+    case kPausing:
+      set_state(kFlushing);
+      pipeline_filter_->Flush(
+          NewCallback(this, &PipelineImpl::OnTeardownStateTransition));
+      break;
+    case kFlushing:
+      set_state(kStopping);
+      pipeline_filter_->Stop(
+          NewCallback(this, &PipelineImpl::OnTeardownStateTransition));
+      break;
+
+    case kCreated:
+    case kError:
+    case kInitDataSource:
+    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.
+  };
+}
+
 void PipelineImpl::FinishDestroyingFiltersTask() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(IsPipelineStopped());
@@ -926,16 +980,15 @@ void PipelineImpl::FinishDestroyingFiltersTask() {
   }
 
   if (stop_pending_) {
+    stop_pending_ = false;
     ResetState();
-
+    scoped_ptr<PipelineCallback> stop_callback(stop_callback_.release());
     // Notify the client that stopping has finished.
-    if (stop_callback_.get()) {
-      stop_callback_->Run();
-      stop_callback_.reset();
+    if (stop_callback.get()) {
+      stop_callback->Run();
     }
   }
 
-  stop_pending_ = false;
   tearing_down_ = false;
   error_caused_teardown_ = false;
 }
@@ -1119,6 +1172,10 @@ void PipelineImpl::TearDownPipeline() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   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;
 
@@ -1126,6 +1183,8 @@ void PipelineImpl::TearDownPipeline() {
     case kCreated:
     case kError:
       set_state(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,
           NewRunnableMethod(this, &PipelineImpl::FinishDestroyingFiltersTask));
       break;
@@ -1142,7 +1201,7 @@ void PipelineImpl::TearDownPipeline() {
 
       set_state(kStopping);
       pipeline_filter_->Stop(
-          NewCallback(this, &PipelineImpl::OnFilterStateTransition));
+          NewCallback(this, &PipelineImpl::OnTeardownStateTransition));
 
       FinishInitialization();
       break;
@@ -1153,14 +1212,20 @@ void PipelineImpl::TearDownPipeline() {
     case kStarting:
       set_state(kStopping);
       pipeline_filter_->Stop(
-          NewCallback(this, &PipelineImpl::OnFilterStateTransition));
+          NewCallback(this, &PipelineImpl::OnTeardownStateTransition));
+
+      if (seek_pending_) {
+        seek_pending_ = false;
+        FinishInitialization();
+      }
+
       break;
 
     case kStarted:
     case kEnded:
       set_state(kPausing);
       pipeline_filter_->Pause(
-          NewCallback(this, &PipelineImpl::OnFilterStateTransition));
+          NewCallback(this, &PipelineImpl::OnTeardownStateTransition));
       break;
 
     case kStopping:
diff --git a/media/base/pipeline_impl.h b/media/base/pipeline_impl.h
index addd1ce..3611416 100644
--- a/media/base/pipeline_impl.h
+++ b/media/base/pipeline_impl.h
@@ -183,6 +183,9 @@ class PipelineImpl : public Pipeline, public FilterHost {
   // or Stop().
   void OnFilterStateTransition();
 
+  // Callback executed by filters when completing teardown operations.
+  void OnTeardownStateTransition();
+
   // The following "task" methods correspond to the public methods, but these
   // methods are run as the result of posting a task to the PipelineInternal's
   // message loop.
@@ -224,6 +227,9 @@ class PipelineImpl : public Pipeline, public FilterHost {
   // Carries out advancing to the next filter during Play()/Pause()/Seek().
   void FilterStateTransitionTask();
 
+  // Carries out advancing to the next teardown operation.
+  void TeardownStateTransitionTask();
+
   // Carries out stopping filter threads, deleting filters, running
   // appropriate callbacks, and setting the appropriate pipeline state
   // depending on whether we performing Stop() or SetError().
diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index eff9498..54e4dbf 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -12,6 +12,7 @@
 #include "media/base/media_format.h"
 #include "media/base/filters.h"
 #include "media/base/filter_host.h"
+#include "media/base/mock_callback.h"
 #include "media/base/mock_filters.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -801,4 +802,39 @@ TEST_F(PipelineImplTest, AudioStreamShorterThanVideo) {
   host->NotifyEnded();
 }
 
+TEST_F(PipelineImplTest, ErrorDuringSeek) {
+  CreateAudioStream();
+  MockDemuxerStreamVector streams;
+  streams.push_back(audio_stream());
+
+  InitializeDataSource();
+  InitializeDemuxer(&streams, base::TimeDelta::FromSeconds(10));
+  InitializeAudioDecoder(audio_stream());
+  InitializeAudioRenderer();
+  InitializePipeline();
+
+  float playback_rate = 1.0f;
+  EXPECT_CALL(*mocks_->data_source(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->audio_decoder(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(playback_rate));
+  pipeline_->SetPlaybackRate(playback_rate);
+  message_loop_.RunAllPending();
+
+  InSequence s;
+
+  base::TimeDelta seek_time = base::TimeDelta::FromSeconds(5);
+  EXPECT_CALL(*mocks_->data_source(), Seek(seek_time, NotNull()))
+      .WillOnce(Invoke(&RunFilterCallback));
+
+  EXPECT_CALL(*mocks_->demuxer(), Seek(seek_time, NotNull()))
+      .WillOnce(DoAll(SetError(mocks_->demuxer(),
+                               PIPELINE_ERROR_READ),
+                      Invoke(&RunFilterCallback)));
+
+  pipeline_->Seek(seek_time, NewExpectedCallback());
+  EXPECT_CALL(callbacks_, OnError());
+  message_loop_.RunAllPending();
+}
+
 }  // namespace media