Fix flaky behavior in pipeline teardown.

media/base/pipeline_impl.cc

 // Copyright (c) 2010 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.
 //
 // TODO(scherkus): clean up PipelineImpl... too many crazy function names,
 // potential deadlocks, etc...
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/stl_util-inl.h"
@@ skipping 282 matching lines @@
   return current_bytes_;
 }
 
 void PipelineImpl::ResetState() {
   AutoLock auto_lock(lock_);
   const base::TimeDelta kZero;
   running_          = false;
   stop_pending_     = false;
   seek_pending_     = false;
   tearing_down_     = false;
+  error_caused_teardown_ = false;
   duration_         = kZero;
   buffered_time_    = kZero;
   buffered_bytes_   = 0;
   streaming_        = false;
   loaded_           = false;
   total_bytes_      = 0;
   video_width_      = 0;
   video_height_     = 0;
   volume_           = 1.0f;
   playback_rate_    = 0.0f;
   error_            = PIPELINE_OK;
   waiting_for_clock_update_ = false;
   audio_disabled_   = false;
   clock_->SetTime(kZero);
   rendered_mime_types_.clear();
 }
 
 void PipelineImpl::set_state(State next_state) {
   state_ = next_state;
 }
 
 bool PipelineImpl::IsPipelineOk() {
   return PIPELINE_OK == GetError();
 }
 
 bool PipelineImpl::IsPipelineInitializing() {

[scherkus (not reviewing), 2011/01/13 00:47:38]

I wonder if we still need this method given the only other place is in a
DCHECK...

[acolwell GONE FROM CHROMIUM, 2011/01/13 01:39:16]

Moved method body to DCHECK()and removed method.

On 2011/01/13 00:47:38, scherkus wrote:

   DCHECK_EQ(MessageLoop::current(), message_loop_);
   return state_ == kInitDataSource ||
          state_ == kInitDemuxer ||
          state_ == kInitAudioDecoder ||
          state_ == kInitAudioRenderer ||
          state_ == kInitVideoDecoder ||
          state_ == kInitVideoRenderer;
 }
 
 bool PipelineImpl::IsPipelineStopped() {
@@ skipping 47 matching lines @@
   } 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() || IsPipelineStopPending());
     return IsPipelineSeeking() ? kSeeking : kStopping;
   } else if (current == kSeeking) {
     return kStarting;
   } else if (current == kStarting) {
     return kStarted;
   } else if (current == kStopping) {
-    return kStopped;
+    return error_caused_teardown_ ? kError : kStopped;
   } else {
     return current;
   }
 }
 
 void PipelineImpl::SetError(PipelineError error) {
   DCHECK(IsRunning());
   DCHECK(error != PIPELINE_OK) << "PIPELINE_OK isn't an error!";
   VLOG(1) << "Media pipeline error: " << error;
 
@@ skipping 256 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 PipelineImpl::StopTask(PipelineCallback* stop_callback) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  PipelineError error = GetError();
+  DCHECK(!IsPipelineStopPending());
+  DCHECK_NE(state_, kStopped);
 
-  if (state_ == kStopped || (IsPipelineStopPending() && error == PIPELINE_OK)) {
-    // If we are already stopped or stopping normally, return immediately.
+  if (state_ == kStopped) {
+    // Already stopped so just run callback.
+    stop_callback->Run();
     delete stop_callback;
     return;
-  } else if (state_ == kError ||
-             (IsPipelineStopPending() && error != PIPELINE_OK)) {
+  }
+
+  if (IsPipelineTearingDown() && error_caused_teardown_) {

[scherkus (not reviewing), 2011/01/13 00:47:38]

to confirm: this happens in the case someone calls Stop() while we've errored?

in which case we want to override the error with normal stop behaviour?

[acolwell GONE FROM CHROMIUM, 2011/01/13 01:39:16]

This happens in the case where an error occurs that causes the pipeline to
shutdown. Before that teardown completes and the OnError() callback is called,
the pipeline calls Stop(). 
 This basically prevents the OnError() call from occurring since the pipeline
already decided to stop before it even knew about the error.

     // If we are stopping due to SetError(), stop normally instead of
-    // going to error state.
+    // going to error state and calling |error_callback_|. This converts
+    // the teardown in progress from an error teardown into one that acts
+    // like the error never occurred.
     AutoLock auto_lock(lock_);
     error_ = PIPELINE_OK;
+    error_caused_teardown_ = false;
   }
 
   stop_callback_.reset(stop_callback);
 
   stop_pending_ = true;
-  if (!IsPipelineSeeking()) {
+  if (!IsPipelineSeeking() && !IsPipelineTearingDown()) {
     // We will tear down pipeline immediately when there is no seek operation
-    // pending. This should include the case where we are partially initialized.
-    // Ideally this case should use SetError() rather than Stop() to tear down.
-    DCHECK(!IsPipelineTearingDown());
+    // pending and no teardown in progress. This should include the case where
+    // we are partially initialized.
     TearDownPipeline();
   }
 }
 
 void PipelineImpl::ErrorChangedTask(PipelineError error) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   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()) {
     return;
   }
 
   AutoLock auto_lock(lock_);
   error_ = error;
 
+  error_caused_teardown_ = true;
   TearDownPipeline();
 }
 
 void PipelineImpl::PlaybackRateChangedTask(float playback_rate) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   {
     AutoLock auto_lock(lock_);
     clock_->SetPlaybackRate(playback_rate);
   }
 
@@ skipping 185 matching lines @@
 void PipelineImpl::FinishDestroyingFiltersTask() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(IsPipelineStopped());
 
   // Clear renderer references.
   audio_renderer_ = NULL;
   video_renderer_ = NULL;
 
   pipeline_filter_ = NULL;
 
-  stop_pending_ = false;
-  tearing_down_ = false;
+  if (error_caused_teardown_  && GetError() != PIPELINE_OK &&

[scherkus (not reviewing), 2011/01/13 00:47:38]

nit: get rid of extra space between error_caused_teardown_ and &&

[acolwell GONE FROM CHROMIUM, 2011/01/13 01:39:16]

Done.

+      error_callback_.get()) {
+    error_callback_->Run();
+  }
 
-  if (PIPELINE_OK == GetError()) {
-    // Destroying filters due to Stop().
+  if (stop_pending_) {
     ResetState();
 
     // Notify the client that stopping has finished.
     if (stop_callback_.get()) {
       stop_callback_->Run();
       stop_callback_.reset();
     }
-  } else {
-    // Destroying filters due to SetError().
-    set_state(kError);
-    // If our owner has requested to be notified of an error.
-    if (error_callback_.get()) {
-      error_callback_->Run();
-    }
   }
+
+  stop_pending_ = false;
+  tearing_down_ = false;
+  error_caused_teardown_ = false;
 }
 
 bool PipelineImpl::PrepareFilter(scoped_refptr<Filter> filter) {
   bool ret = pipeline_init_state_->composite_->AddFilter(filter.get());
 
   if (!ret) {
     SetError(PIPELINE_ERROR_INITIALIZATION_FAILED);
   }
   return ret;
 }
@@ skipping 164 matching lines @@
   return NULL;
 }
 
 void PipelineImpl::TearDownPipeline() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK_NE(kStopped, state_);
 
   // Mark that we already start tearing down operation.
   tearing_down_ = true;
 
-  if (IsPipelineInitializing()) {
-    // Make it look like initialization was successful.
-    pipeline_filter_ = pipeline_init_state_->composite_;
-    pipeline_init_state_.reset();
+  switch(state_) {
+    case kCreated:
+    case kError:
+      set_state(kStopped);
+      message_loop_->PostTask(FROM_HERE,
+          NewRunnableMethod(this, &PipelineImpl::FinishDestroyingFiltersTask));
+      break;
 
-    set_state(kStopping);
-    pipeline_filter_->Stop(NewCallback(
-        this, &PipelineImpl::OnFilterStateTransition));
+    case kInitDataSource:
+    case kInitDemuxer:
+    case kInitAudioDecoder:
+    case kInitAudioRenderer:
+    case kInitVideoDecoder:
+    case kInitVideoRenderer:
+      // Make it look like initialization was successful.
+      pipeline_filter_ = pipeline_init_state_->composite_;
+      pipeline_init_state_.reset();
 
-    FinishInitialization();
-  } else if (pipeline_filter_.get()) {
-    set_state(kPausing);
-    pipeline_filter_->Pause(NewCallback(
-        this, &PipelineImpl::OnFilterStateTransition));
-  } else {
-    set_state(kStopped);
-    message_loop_->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &PipelineImpl::FinishDestroyingFiltersTask));
-  }
+      set_state(kStopping);
+      pipeline_filter_->Stop(
+          NewCallback(this, &PipelineImpl::OnFilterStateTransition));
+
+      FinishInitialization();
+      break;
+
+    case kPausing:
+    case kSeeking:
+    case kFlushing:
+    case kStarting:
+      set_state(kStopping);
+      pipeline_filter_->Stop(NewCallback(

[scherkus (not reviewing), 2011/01/13 00:47:38]

drop NewCallback to next line

[acolwell GONE FROM CHROMIUM, 2011/01/13 01:39:16]

Done.

+          this, &PipelineImpl::OnFilterStateTransition));
+      break;
+
+    case kStarted:
+    case kEnded:
+      set_state(kPausing);
+      pipeline_filter_->Pause(NewCallback(

[scherkus (not reviewing), 2011/01/13 00:47:38]

drop NewCallback to next line

[acolwell GONE FROM CHROMIUM, 2011/01/13 01:39:16]

Done.

+          this, &PipelineImpl::OnFilterStateTransition));
+      break;
+
+    case kStopping:
+    case kStopped:
+      NOTREACHED() << "Unexpected state for teardown: " << state_;
+      break;
+    // default: intentionally left out to force new states to cause compiler
+    // errors.
+  };
 }
 
 }  // namespace media