Remove reference counting from media::VideoRenderer and friends.

BUG=173313

Review URL: https://codereview.chromium.org/12114024

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

13 files changed, 75 insertions, 67 deletions

diff --git a/media/base/filter_collection.cc b/media/base/filter_collection.cc
index f82a61b..7824b1e 100644
--- a/media/base/filter_collection.cc
+++ b/media/base/filter_collection.cc
@@ -29,15 +29,20 @@ void FilterCollection::AddAudioRenderer(AudioRenderer* audio_renderer) {
   audio_renderers_.push_back(audio_renderer);
 }
 
-void FilterCollection::AddVideoRenderer(VideoRenderer* video_renderer) {
-  video_renderers_.push_back(video_renderer);
+void FilterCollection::SetVideoRenderer(
+    scoped_ptr<VideoRenderer> video_renderer) {
+  video_renderer_ = video_renderer.Pass();
+}
+
+scoped_ptr<VideoRenderer> FilterCollection::GetVideoRenderer() {
+  return video_renderer_.Pass();
 }
 
 void FilterCollection::Clear() {
   audio_decoders_.clear();
   video_decoders_.clear();
   audio_renderers_.clear();
-  video_renderers_.clear();
+  video_renderer_.reset();
 }
 
 void FilterCollection::SelectAudioRenderer(scoped_refptr<AudioRenderer>* out) {
@@ -49,15 +54,6 @@ void FilterCollection::SelectAudioRenderer(scoped_refptr<AudioRenderer>* out) {
   audio_renderers_.pop_front();
 }
 
-void FilterCollection::SelectVideoRenderer(scoped_refptr<VideoRenderer>* out) {
-  if (video_renderers_.empty()) {
-    *out = NULL;
-    return;
-  }
-  *out = video_renderers_.front();
-  video_renderers_.pop_front();
-}
-
 FilterCollection::AudioDecoderList* FilterCollection::GetAudioDecoders() {
   return &audio_decoders_;
 }
diff --git a/media/base/filter_collection.h b/media/base/filter_collection.h
index e9f2be5..100cd64 100644
--- a/media/base/filter_collection.h
+++ b/media/base/filter_collection.h
@@ -8,6 +8,7 @@
 #include <list>
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "media/base/media_export.h"
 
 namespace media {
@@ -36,9 +37,10 @@ class MEDIA_EXPORT FilterCollection {
   const scoped_refptr<Demuxer>& GetDemuxer();
 
   // Adds a filter to the collection.
-  void AddAudioDecoder(AudioDecoder* audio_decoder);
   void AddAudioRenderer(AudioRenderer* audio_renderer);
-  void AddVideoRenderer(VideoRenderer* video_renderer);
+
+  void SetVideoRenderer(scoped_ptr<VideoRenderer> video_renderer);
+  scoped_ptr<VideoRenderer> GetVideoRenderer();
 
   // Remove remaining filters.
   void Clear();
@@ -48,7 +50,6 @@ class MEDIA_EXPORT FilterCollection {
   // If a filter is returned it is removed from the collection.
   // Filters are selected in FIFO order.
   void SelectAudioRenderer(scoped_refptr<AudioRenderer>* out);
-  void SelectVideoRenderer(scoped_refptr<VideoRenderer>* out);
 
   AudioDecoderList* GetAudioDecoders();
   VideoDecoderList* GetVideoDecoders();
@@ -58,7 +59,7 @@ class MEDIA_EXPORT FilterCollection {
   AudioDecoderList audio_decoders_;
   VideoDecoderList video_decoders_;
   std::list<scoped_refptr<AudioRenderer> > audio_renderers_;
-  std::list<scoped_refptr<VideoRenderer> > video_renderers_;
+  scoped_ptr<VideoRenderer> video_renderer_;
 
   DISALLOW_COPY_AND_ASSIGN(FilterCollection);
 };
diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h
index 3050fb5..2050e87 100644
--- a/media/base/mock_filters.h
+++ b/media/base/mock_filters.h
@@ -134,6 +134,7 @@ class MockAudioDecoder : public AudioDecoder {
 class MockVideoRenderer : public VideoRenderer {
  public:
   MockVideoRenderer();
+  virtual ~MockVideoRenderer();
 
   // VideoRenderer implementation.
   MOCK_METHOD10(Initialize, void(const scoped_refptr<DemuxerStream>& stream,
@@ -153,9 +154,6 @@ class MockVideoRenderer : public VideoRenderer {
   MOCK_METHOD1(Stop, void(const base::Closure& callback));
   MOCK_METHOD1(SetPlaybackRate, void(float playback_rate));
 
- protected:
-  virtual ~MockVideoRenderer();
-
  private:
   DISALLOW_COPY_AND_ASSIGN(MockVideoRenderer);
 };
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index cc6419a..cdc0aa6 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -538,7 +538,8 @@ void Pipeline::DoInitialPreroll(const PipelineStatusCB& done_cb) {
 
   if (video_renderer_) {
     bound_fns.Push(base::Bind(
-        &VideoRenderer::Preroll, video_renderer_, seek_timestamp));
+        &VideoRenderer::Preroll, base::Unretained(video_renderer_.get()),
+        seek_timestamp));
   }
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
@@ -554,14 +555,18 @@ void Pipeline::DoSeek(
   // Pause.
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Pause, audio_renderer_));
-  if (video_renderer_)
-    bound_fns.Push(base::Bind(&VideoRenderer::Pause, video_renderer_));
+  if (video_renderer_) {
+    bound_fns.Push(base::Bind(
+        &VideoRenderer::Pause, base::Unretained(video_renderer_.get())));
+  }
 
   // Flush.
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Flush, audio_renderer_));
-  if (video_renderer_)
-    bound_fns.Push(base::Bind(&VideoRenderer::Flush, video_renderer_));
+  if (video_renderer_) {
+    bound_fns.Push(base::Bind(
+        &VideoRenderer::Flush, base::Unretained(video_renderer_.get())));
+  }
 
   // Seek demuxer.
   bound_fns.Push(base::Bind(
@@ -575,7 +580,8 @@ void Pipeline::DoSeek(
 
   if (video_renderer_) {
     bound_fns.Push(base::Bind(
-        &VideoRenderer::Preroll, video_renderer_, seek_timestamp));
+        &VideoRenderer::Preroll, base::Unretained(video_renderer_.get()),
+        seek_timestamp));
   }
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
@@ -592,8 +598,10 @@ void Pipeline::DoPlay(const PipelineStatusCB& done_cb) {
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Play, audio_renderer_));
 
-  if (video_renderer_)
-    bound_fns.Push(base::Bind(&VideoRenderer::Play, video_renderer_));
+  if (video_renderer_) {
+    bound_fns.Push(base::Bind(
+        &VideoRenderer::Play, base::Unretained(video_renderer_.get())));
+  }
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
@@ -609,8 +617,10 @@ void Pipeline::DoStop(const PipelineStatusCB& done_cb) {
   if (audio_renderer_)
     bound_fns.Push(base::Bind(&AudioRenderer::Stop, audio_renderer_));
 
-  if (video_renderer_)
-    bound_fns.Push(base::Bind(&VideoRenderer::Stop, video_renderer_));
+  if (video_renderer_) {
+    bound_fns.Push(base::Bind(
+        &VideoRenderer::Stop, base::Unretained(video_renderer_.get())));
+  }
 
   pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
 }
@@ -627,7 +637,7 @@ void Pipeline::OnStopCompleted(PipelineStatus status) {
   pending_callbacks_.reset();
   filter_collection_.reset();
   audio_renderer_ = NULL;
-  video_renderer_ = NULL;
+  video_renderer_.reset();
   demuxer_ = NULL;
 
   // If we stop during initialization/seeking we want to run |seek_cb_|
@@ -912,7 +922,7 @@ void Pipeline::InitializeVideoRenderer(const PipelineStatusCB& done_cb) {
     natural_size_ = stream->video_decoder_config().natural_size();
   }
 
-  filter_collection_->SelectVideoRenderer(&video_renderer_);
+  video_renderer_ = filter_collection_->GetVideoRenderer();
   video_renderer_->Initialize(
       stream,
       *filter_collection_->GetVideoDecoders(),
diff --git a/media/base/pipeline.h b/media/base/pipeline.h
index 25e4dcb..54d7239 100644
--- a/media/base/pipeline.h
+++ b/media/base/pipeline.h
@@ -452,7 +452,7 @@ class MEDIA_EXPORT Pipeline
   // Renderer references used for setting the volume, playback rate, and
   // determining when playback has finished.
   scoped_refptr<AudioRenderer> audio_renderer_;
-  scoped_refptr<VideoRenderer> video_renderer_;
+  scoped_ptr<VideoRenderer> video_renderer_;
 
   // Demuxer reference used for setting the preload value.
   scoped_refptr<Demuxer> demuxer_;
diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc
index 2d5b63b..cfe41b5 100644
--- a/media/base/pipeline_unittest.cc
+++ b/media/base/pipeline_unittest.cc
@@ -91,7 +91,8 @@ class PipelineTest : public ::testing::Test {
     filter_collection_->GetAudioDecoders()->push_back(audio_decoder_);
 
     video_renderer_ = new MockVideoRenderer();
-    filter_collection_->AddVideoRenderer(video_renderer_);
+    scoped_ptr<VideoRenderer> video_renderer(video_renderer_);
+    filter_collection_->SetVideoRenderer(video_renderer.Pass());
 
     audio_renderer_ = new MockAudioRenderer();
     filter_collection_->AddAudioRenderer(audio_renderer_);
@@ -299,7 +300,7 @@ class PipelineTest : public ::testing::Test {
   scoped_refptr<MockDemuxer> demuxer_;
   scoped_refptr<MockVideoDecoder> video_decoder_;
   scoped_refptr<MockAudioDecoder> audio_decoder_;
-  scoped_refptr<MockVideoRenderer> video_renderer_;
+  MockVideoRenderer* video_renderer_;
   scoped_refptr<MockAudioRenderer> audio_renderer_;
   scoped_refptr<StrictMock<MockDemuxerStream> > audio_stream_;
   scoped_refptr<StrictMock<MockDemuxerStream> > video_stream_;
diff --git a/media/base/video_renderer.h b/media/base/video_renderer.h
index c19c8f9..c609b68 100644
--- a/media/base/video_renderer.h
+++ b/media/base/video_renderer.h
@@ -22,8 +22,7 @@ namespace media {
 class DemuxerStream;
 class VideoDecoder;
 
-class MEDIA_EXPORT VideoRenderer
-    : public base::RefCountedThreadSafe<VideoRenderer> {
+class MEDIA_EXPORT VideoRenderer {
  public:
   typedef std::list<scoped_refptr<VideoDecoder> > VideoDecoderList;
 
@@ -37,6 +36,9 @@ class MEDIA_EXPORT VideoRenderer
   // Used to query the current time or duration of the media.
   typedef base::Callback<base::TimeDelta()> TimeDeltaCB;
 
+  VideoRenderer();
+  virtual ~VideoRenderer();
+
   // Initialize a VideoRenderer with the given DemuxerStream and
   // VideoDecoderList, executing |init_cb| callback upon completion.
   //
@@ -91,12 +93,6 @@ class MEDIA_EXPORT VideoRenderer
   // Updates the current playback rate.
   virtual void SetPlaybackRate(float playback_rate) = 0;
 
- protected:
-  friend class base::RefCountedThreadSafe<VideoRenderer>;
-
-  VideoRenderer();
-  virtual ~VideoRenderer();
-
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoRenderer);
 };
diff --git a/media/filters/pipeline_integration_test_base.cc b/media/filters/pipeline_integration_test_base.cc
index b065b57..54c4fdf 100644
--- a/media/filters/pipeline_integration_test_base.cc
+++ b/media/filters/pipeline_integration_test_base.cc
@@ -211,7 +211,7 @@ PipelineIntegrationTestBase::CreateFilterCollection(
   collection->GetVideoDecoders()->push_back(vpx_decoder);
 
   // Disable frame dropping if hashing is enabled.
-  renderer_ = new VideoRendererBase(
+  scoped_ptr<VideoRenderer> renderer(new VideoRendererBase(
       message_loop_.message_loop_proxy(),
       base::Bind(&PipelineIntegrationTestBase::SetDecryptor,
                  base::Unretained(this), decryptor),
@@ -219,8 +219,9 @@ PipelineIntegrationTestBase::CreateFilterCollection(
                  base::Unretained(this)),
       base::Bind(&PipelineIntegrationTestBase::OnSetOpaque,
                  base::Unretained(this)),
-      !hashing_enabled_);
-  collection->AddVideoRenderer(renderer_);
+      !hashing_enabled_));
+  collection->SetVideoRenderer(renderer.Pass());
+
   audio_sink_ = new NullAudioSink();
   if (hashing_enabled_)
     audio_sink_->StartAudioHashForTesting();
diff --git a/media/filters/pipeline_integration_test_base.h b/media/filters/pipeline_integration_test_base.h
index 411e564..23c0339 100644
--- a/media/filters/pipeline_integration_test_base.h
+++ b/media/filters/pipeline_integration_test_base.h
@@ -74,7 +74,6 @@ class PipelineIntegrationTestBase {
   base::MD5Context md5_context_;
   bool hashing_enabled_;
   scoped_refptr<Pipeline> pipeline_;
-  scoped_refptr<VideoRendererBase> renderer_;
   scoped_refptr<NullAudioSink> audio_sink_;
   bool ended_;
   PipelineStatus pipeline_status_;
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index d2c577b..0ae775a 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -29,6 +29,7 @@ VideoRendererBase::VideoRendererBase(
     const SetOpaqueCB& set_opaque_cb,
     bool drop_frames)
     : message_loop_(message_loop),
+      weak_factory_(this),
       set_decryptor_ready_cb_(set_decryptor_ready_cb),
       frame_available_(&lock_),
       state_(kUninitialized),
@@ -42,6 +43,12 @@ VideoRendererBase::VideoRendererBase(
   DCHECK(!paint_cb_.is_null());
 }
 
+VideoRendererBase::~VideoRendererBase() {
+  base::AutoLock auto_lock(lock_);
+  CHECK(state_ == kUninitialized || state_ == kStopped) << state_;
+  CHECK_EQ(thread_, base::kNullThreadHandle);
+}
+
 void VideoRendererBase::Play(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
@@ -67,17 +74,19 @@ void VideoRendererBase::Flush(const base::Closure& callback) {
 
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
-        &VideoRendererBase::ResetDecoder, this));
+        &VideoRendererBase::ResetDecoder, weak_this_));
     return;
   }
 
-  decoder_->Reset(base::Bind(&VideoRendererBase::OnDecoderResetDone, this));
+  decoder_->Reset(base::Bind(
+      &VideoRendererBase::OnDecoderResetDone, weak_this_));
 }
 
 void VideoRendererBase::ResetDecoder() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
-  decoder_->Reset(base::Bind(&VideoRendererBase::OnDecoderResetDone, this));
+  decoder_->Reset(base::Bind(
+      &VideoRendererBase::OnDecoderResetDone, weak_this_));
 }
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
@@ -110,7 +119,7 @@ void VideoRendererBase::Stop(const base::Closure& callback) {
 
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
-        &VideoRendererBase::StopDecoder, this, callback));
+        &VideoRendererBase::StopDecoder, weak_this_, callback));
     return;
   }
 
@@ -168,6 +177,7 @@ void VideoRendererBase::Initialize(const scoped_refptr<DemuxerStream>& stream,
   DCHECK(!get_duration_cb.is_null());
   DCHECK_EQ(kUninitialized, state_);
 
+  weak_this_ = weak_factory_.GetWeakPtr();
   init_cb_ = init_cb;
   statistics_cb_ = statistics_cb;
   max_time_cb_ = max_time_cb;
@@ -189,7 +199,7 @@ void VideoRendererBase::Initialize(const scoped_refptr<DemuxerStream>& stream,
   decoder_selector_ptr->SelectVideoDecoder(
       stream,
       statistics_cb,
-      base::Bind(&VideoRendererBase::OnDecoderSelected, this,
+      base::Bind(&VideoRendererBase::OnDecoderSelected, weak_this_,
                  base::Passed(&decoder_selector)));
 }
 
@@ -337,7 +347,7 @@ void VideoRendererBase::PaintNextReadyFrame_Locked() {
   paint_cb_.Run(next_frame);
 
   message_loop_->PostTask(FROM_HERE, base::Bind(
-      &VideoRendererBase::AttemptRead, this));
+      &VideoRendererBase::AttemptRead, weak_this_));
 }
 
 void VideoRendererBase::DropNextReadyFrame_Locked() {
@@ -351,13 +361,7 @@ void VideoRendererBase::DropNextReadyFrame_Locked() {
   statistics_cb_.Run(statistics);
 
   message_loop_->PostTask(FROM_HERE, base::Bind(
-      &VideoRendererBase::AttemptRead, this));
-}
-
-VideoRendererBase::~VideoRendererBase() {
-  base::AutoLock auto_lock(lock_);
-  CHECK(state_ == kUninitialized || state_ == kStopped) << state_;
-  CHECK_EQ(thread_, base::kNullThreadHandle);
+      &VideoRendererBase::AttemptRead, weak_this_));
 }
 
 void VideoRendererBase::FrameReady(VideoDecoder::Status status,
@@ -504,7 +508,7 @@ void VideoRendererBase::AttemptRead_Locked() {
     case kPrerolling:
     case kPlaying:
       pending_read_ = true;
-      decoder_->Read(base::Bind(&VideoRendererBase::FrameReady, this));
+      decoder_->Read(base::Bind(&VideoRendererBase::FrameReady, weak_this_));
       return;
 
     case kUninitialized:
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index ed0177b..1234ab8 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -8,6 +8,7 @@
 #include <deque>
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
@@ -60,6 +61,7 @@ class MEDIA_EXPORT VideoRendererBase
                     const PaintCB& paint_cb,
                     const SetOpaqueCB& set_opaque_cb,
                     bool drop_frames);
+  virtual ~VideoRendererBase();
 
   // VideoRenderer implementation.
   virtual void Initialize(const scoped_refptr<DemuxerStream>& stream,
@@ -83,9 +85,6 @@ class MEDIA_EXPORT VideoRendererBase
   // PlatformThread::Delegate implementation.
   virtual void ThreadMain() OVERRIDE;
 
- protected:
-  virtual ~VideoRendererBase();
-
  private:
   // Called when |decoder_selector_| selected the |selected_decoder|.
   // |decrypting_demuxer_stream| was also populated if a DecryptingDemuxerStream
@@ -158,6 +157,8 @@ class MEDIA_EXPORT VideoRendererBase
                          PipelineStatus status);
 
   scoped_refptr<base::MessageLoopProxy> message_loop_;
+  base::WeakPtrFactory<VideoRendererBase> weak_factory_;
+  base::WeakPtr<VideoRendererBase> weak_this_;
 
   // Used for accessing data members.
   base::Lock lock_;
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index 1049735..79b3bf9 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -44,12 +44,12 @@ class VideoRendererBaseTest : public ::testing::Test {
         demuxer_stream_(new MockDemuxerStream()),
         video_config_(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN, kVideoFormat,
                       kCodedSize, kVisibleRect, kNaturalSize, NULL, 0, false) {
-    renderer_ = new VideoRendererBase(
+    renderer_.reset(new VideoRendererBase(
         message_loop_.message_loop_proxy(),
         media::SetDecryptorReadyCB(),
         base::Bind(&VideoRendererBaseTest::OnPaint, base::Unretained(this)),
         base::Bind(&VideoRendererBaseTest::OnSetOpaque, base::Unretained(this)),
-        true);
+        true));
 
     EXPECT_CALL(*demuxer_stream_, type())
         .WillRepeatedly(Return(DemuxerStream::VIDEO));
@@ -290,7 +290,7 @@ class VideoRendererBaseTest : public ::testing::Test {
 
  protected:
   // Fixture members.
-  scoped_refptr<VideoRendererBase> renderer_;
+  scoped_ptr<VideoRendererBase> renderer_;
   scoped_refptr<MockVideoDecoder> decoder_;
   scoped_refptr<MockDemuxerStream> demuxer_stream_;
   MockStatisticsCB statistics_cb_object_;
diff --git a/media/tools/player_x11/player_x11.cc b/media/tools/player_x11/player_x11.cc
index aca065d..7128479 100644
--- a/media/tools/player_x11/player_x11.cc
+++ b/media/tools/player_x11/player_x11.cc
@@ -111,12 +111,13 @@ bool InitPipeline(const scoped_refptr<base::MessageLoopProxy>& message_loop,
       message_loop));
 
   // Create our video renderer and save a reference to it for painting.
-  collection->AddVideoRenderer(new media::VideoRendererBase(
+  scoped_ptr<media::VideoRenderer> video_renderer(new media::VideoRendererBase(
       message_loop,
       media::SetDecryptorReadyCB(),
       base::Bind(&Paint, paint_message_loop, paint_cb),
       base::Bind(&SetOpaque),
       true));
+  collection->SetVideoRenderer(video_renderer.Pass());
 
   collection->AddAudioRenderer(new media::AudioRendererImpl(
       message_loop,