Fix deadlock in WebMediaPlayerImpl::Destroy() when HW video decode is enabled.

Several unfortunate factors combine to require this CL:
1) WebMediaPlayerImpl::Destroy() holds the renderer thread
   hostage for the duration of PipelineImpl::Stop()
2) PipelineImpl::Stop() walks all its filters through Pause,
   Flush, and Stop, not starting one transition until the
   previous is complete.
3) VideoRendererBase::Flush() doesn't complete until any pending
   read to the video decoder is satisfied.


on the render thread (because of PipelineImpl locking preventing
re-entrancy, being triggered on stats update).  Therefore GVD
must have its own thread.  Because GpuVideoDecodeAcceleratorHost
expects to run on the renderer thread (or be rewritten), that
means trampolining vda_ calls from GVD through the renderer
thread.  #2 & #1 mean that such trampolining must be
fire-and-forget during shutdown.

The new VideoDecoder::PrepareForShutdownHack method is
reminiscent of WebMediaPlayerProxy::AbortDataSources(), but
thankfully at least this version of that hack can be contained
within media/.

The OmxVideoDecodeAccelerator change to DCHECKs is unrelated to
the feature here but was uncovered during testing.  Basically the
already-allowed current_state_change_ values for when Destroy()
is called implied a wider range of legal current_state_ values
than the DCHECK was asserting.  Fixed that.

BUG=109397
TEST=manual test that reload during playback works.


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

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

4 files changed, 27 insertions, 9 deletions

diff --git a/media/base/filters.cc b/media/base/filters.cc
index ead0823..df7cfdf 100644
--- a/media/base/filters.cc
+++ b/media/base/filters.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -79,6 +79,8 @@ bool VideoDecoder::HasAlpha() const {
   return false;
 }
 
+void VideoDecoder::PrepareForShutdownHack() {}
+
 AudioDecoder::AudioDecoder() {}
 
 AudioDecoder::~AudioDecoder() {}
diff --git a/media/base/filters.h b/media/base/filters.h
index adb6aff..8f186d6 100644
--- a/media/base/filters.h
+++ b/media/base/filters.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -153,6 +153,14 @@ class MEDIA_EXPORT VideoDecoder : public Filter {
   // that return formats with an alpha channel.
   virtual bool HasAlpha() const;
 
+  // Prepare decoder for shutdown.  This is a HACK needed because
+  // PipelineImpl::Stop() goes through a Pause/Flush/Stop dance to all its
+  // filters, waiting for each state transition to complete before starting the
+  // next, but WebMediaPlayerImpl::Destroy() holds the renderer loop hostage for
+  // the duration.  Default implementation does nothing; derived decoders may
+  // override as needed.  http://crbug.com/110228 tracks removing this.
+  virtual void PrepareForShutdownHack();
+
  protected:
   VideoDecoder();
   virtual ~VideoDecoder();
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index e322299..320b133 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -130,7 +130,10 @@ void PipelineImpl::Stop(const PipelineStatusCB& stop_callback) {
     return;
   }
 
-  // Stop the pipeline, which will set |running_| to false on behalf.
+  if (video_decoder_)
+    video_decoder_->PrepareForShutdownHack();
+
+  // Stop the pipeline, which will set |running_| to false on our behalf.
   message_loop_->PostTask(FROM_HERE,
       base::Bind(&PipelineImpl::StopTask, this, stop_callback));
 }
@@ -1214,19 +1217,18 @@ bool PipelineImpl::InitializeVideoDecoder(
       return false;
   }
 
-  scoped_refptr<VideoDecoder> video_decoder;
-  filter_collection_->SelectVideoDecoder(&video_decoder);
+  filter_collection_->SelectVideoDecoder(&video_decoder_);
 
-  if (!video_decoder) {
+  if (!video_decoder_) {
     SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return false;
   }
 
-  if (!PrepareFilter(video_decoder))
+  if (!PrepareFilter(video_decoder_))
     return false;
 
-  pipeline_init_state_->video_decoder_ = video_decoder;
-  video_decoder->Initialize(
+  pipeline_init_state_->video_decoder_ = video_decoder_;
+  video_decoder_->Initialize(
       stream,
       base::Bind(&PipelineImpl::OnFilterInitialize, this),
       base::Bind(&PipelineImpl::OnUpdateStatistics, this));
diff --git a/media/base/pipeline_impl.h b/media/base/pipeline_impl.h
index 36a9175..5b9ee4e 100644
--- a/media/base/pipeline_impl.h
+++ b/media/base/pipeline_impl.h
@@ -477,6 +477,12 @@ class MEDIA_EXPORT PipelineImpl
   // Reference to the filter(s) that constitute the pipeline.
   scoped_refptr<Filter> pipeline_filter_;
 
+  // Decoder reference used for signalling imminent shutdown.
+  // This is a HACK necessary because WebMediaPlayerImpl::Destroy() holds the
+  // renderer thread loop hostage for until PipelineImpl::Stop() calls its
+  // callback.  http://crbug.com/110228 tracks removing this hack.
+  scoped_refptr<VideoDecoder> video_decoder_;
+
   // Renderer references used for setting the volume and determining
   // when playback has finished.
   scoped_refptr<AudioRenderer> audio_renderer_;