Remove reference counting from media::VideoDecoder and friends.

media/filters/gpu_video_decoder.cc

Index: media/filters/gpu_video_decoder.cc
diff --git a/media/filters/gpu_video_decoder.cc b/media/filters/gpu_video_decoder.cc
index cbad87d37051b4dba304e580c4ed5582fdccd187..08a6810af922be1543ff8d6e6debd6a7f3b83672 100644
--- a/media/filters/gpu_video_decoder.cc
+++ b/media/filters/gpu_video_decoder.cc
@@ -9,6 +9,7 @@
 #include "base/cpu.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
+#include "base/task_runner_util.h"
 #include "media/base/bind_to_loop.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/demuxer_stream.h"
@@ -54,6 +55,7 @@ GpuVideoDecoder::GpuVideoDecoder(
     const scoped_refptr<base::MessageLoopProxy>& message_loop,
     const scoped_refptr<Factories>& factories)
     : gvd_loop_proxy_(message_loop),
+      weak_factory_(this),
       vda_loop_proxy_(factories->GetMessageLoop()),
       factories_(factories),
       state_(kNormal),
@@ -71,7 +73,7 @@ void GpuVideoDecoder::Reset(const base::Closure& closure)  {
 
   if (state_ == kDrainingDecoder) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::Reset, this, closure));
+        &GpuVideoDecoder::Reset, weak_this_, closure));
     // NOTE: if we're deferring Reset() until a Flush() completes, return
     // queued pictures to the VDA so they can be used to finish that Flush().
     if (pending_read_cb_.is_null())
@@ -110,6 +112,8 @@ void GpuVideoDecoder::Initialize(const scoped_refptr<DemuxerStream>& stream,
                                  const PipelineStatusCB& orig_status_cb,
                                  const StatisticsCB& statistics_cb) {
   DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());
+  weak_this_ = weak_factory_.GetWeakPtr();
+
   PipelineStatusCB status_cb = CreateUMAReportingPipelineCB(
       "Media.GpuVideoDecoderInitializeStatus",
       BindToCurrentLoop(orig_status_cb));
@@ -161,17 +165,21 @@ void GpuVideoDecoder::Initialize(const scoped_refptr<DemuxerStream>& stream,
   statistics_cb_ = statistics_cb;
 
   DVLOG(1) << "GpuVideoDecoder::Initialize() succeeded.";
-  vda_loop_proxy_->PostTaskAndReply(
-      FROM_HERE,
-      base::Bind(&GpuVideoDecoder::SetVDA, this, vda),
-      base::Bind(status_cb, PIPELINE_OK));
+  PostTaskAndReplyWithResult(
+      vda_loop_proxy_, FROM_HERE,
+      base::Bind(&VideoDecodeAccelerator::AsWeakPtr, base::Unretained(vda)),
+      base::Bind(&GpuVideoDecoder::SetVDA, weak_this_, status_cb, vda));
 }
 
-void GpuVideoDecoder::SetVDA(VideoDecodeAccelerator* vda) {
-  DCHECK(vda_loop_proxy_->BelongsToCurrentThread());
+void GpuVideoDecoder::SetVDA(
+    const PipelineStatusCB& status_cb,
+    VideoDecodeAccelerator* vda,
+    base::WeakPtr<VideoDecodeAccelerator> weak_vda) {
+  DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());
   DCHECK(!vda_.get());
   vda_.reset(vda);
-  weak_vda_ = vda->AsWeakPtr();
+  weak_vda_ = weak_vda;
+  status_cb.Run(PIPELINE_OK);
 }
 
 void GpuVideoDecoder::DestroyTextures() {
@@ -186,14 +194,21 @@ void GpuVideoDecoder::DestroyTextures() {
 void GpuVideoDecoder::DestroyVDA() {
   DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());
   VideoDecodeAccelerator* vda ALLOW_UNUSED = vda_.release();
-  // Tricky: |this| needs to stay alive until after VDA::Destroy is actually
-  // called, not just posted, so we take an artificial ref to |this| and release
-  // it as |reply| after VDA::Destroy() returns.
-  AddRef();
-  vda_loop_proxy_->PostTaskAndReply(
-      FROM_HERE,
-      base::Bind(&VideoDecodeAccelerator::Destroy, weak_vda_),
-      base::Bind(&GpuVideoDecoder::Release, this));
+
+  // Note: I don't think it's possible to guarantee that VDA *won't* call us

[scherkus (not reviewing), 2013/03/28 02:00:01]

FYI

It's a bit late in the day, but I'm not sure if there's a way to solve this w/o
involving refcounting somehow.

If that's the case, we could introduce a ref-counted proxy class (sigh...) that
implements VDA::Client + target message loop.

We could then remove trampolining code inside GVD and null out the pointer to
GVD during Stop().


I wonder if we can get WMPI to help out here, perhaps via
GpuVideoDecoder::Factories. It would be nice to not rely on a ref-counted proxy
and figure out a proper shutdown + deletion sequence.

Hm.....

[Ami GONE FROM CHROMIUM, 2013/03/29 23:55:19]

Yes, this is the key point of this CL :)
Today GVD _relies_ on being refcounted as a means for staying alive until the
VDA::Destroy() is actually called (and pending callbacks in the VDA loop's queue
being handled), b/c that's the guarantee of no-more-callbacks.

I think you can still get what you want by replacing the current sequence of:

AddRef()
PostTaskAndReply(Destroy, Release)

with:
GVD* self = VRB::decoder_.release();
PostTaskAndReply(Destroy, Bind(Delete, self));

WDYT?

[scherkus (not reviewing), 2013/04/05 01:18:30]

I'd prefer to not mess around w/ transferring ownership from VRB to GVD :\

FWIW I think it'd have to be a scoped_ptr<T> instead of T* otherwise we can risk
leaking when the reply thread has been stopped:
  * VDA runs on render thread
  * WMPI is blocking render thread on Stop()
  * GVD receives Stop() on media thread
  * GVD posts VDA::Destroy() to render thread w/ reply to media thread
  * WMPI + GVD are destroyed
  **** Media thread is stopped ****
  * WMPI returns control to render thread
  * VDA::Destroy() is run
  * PostTaskAndReply() posts task to stopped media thread to destroy GVD
  * MessageLoopProxy drops task


I wrote up a client proxy. It's not too bad ... but I can't say I'm super happy.

+  // after GVD has been destroyed.
+  //
+  // Example:
+  //   * VDA runs on render thread
+  //   * WMPI is blocking render thread on Stop()
+  //   * GVD receives Stop() on media thread
+  //   * GVD posts VDA::Destroy() to render thread
+  //   * GVD completes Stop() and WMPI becomes unblocked
+  //   * WMPI + GVD are destroyed
+  //   * VDA executes pending work on GVD on render thread
+  //   * -- boom --
+  vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+      &VideoDecodeAccelerator::Destroy, weak_vda_));
 
   DestroyTextures();
 }
@@ -245,7 +260,7 @@ void GpuVideoDecoder::RequestBufferDecode(
 
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::RequestBufferDecode, this, status, buffer));
+        &GpuVideoDecoder::RequestBufferDecode, weak_this_, status, buffer));
     return;
   }
   demuxer_read_in_progress_ = false;
@@ -351,7 +366,7 @@ void GpuVideoDecoder::ProvidePictureBuffers(uint32 count,
                                             uint32 texture_target) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::ProvidePictureBuffers, this, count, size,
+        &GpuVideoDecoder::ProvidePictureBuffers, weak_this_, count, size,
         texture_target));
     return;
   }
@@ -386,7 +401,7 @@ void GpuVideoDecoder::ProvidePictureBuffers(uint32 count,
 void GpuVideoDecoder::DismissPictureBuffer(int32 id) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::DismissPictureBuffer, this, id));
+        &GpuVideoDecoder::DismissPictureBuffer, weak_this_, id));
     return;
   }
   std::map<int32, PictureBuffer>::iterator it =
@@ -402,7 +417,7 @@ void GpuVideoDecoder::DismissPictureBuffer(int32 id) {
 void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::PictureReady, this, picture));
+        &GpuVideoDecoder::PictureReady, weak_this_, picture));
     return;
   }
   std::map<int32, PictureBuffer>::iterator it =
@@ -428,8 +443,9 @@ void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
           base::Bind(&Factories::ReadPixels, factories_, pb.texture_id(),
                      decoder_texture_target_,
                      gfx::Size(visible_rect.width(), visible_rect.height())),
-          base::Bind(&GpuVideoDecoder::ReusePictureBuffer, this,
-                     picture.picture_buffer_id())));
+          BindToCurrentLoop(base::Bind(
+              &GpuVideoDecoder::ReusePictureBuffer, weak_this_,
+              picture.picture_buffer_id()))));
   CHECK_GT(available_pictures_, 0);
   available_pictures_--;
 
@@ -458,11 +474,7 @@ void GpuVideoDecoder::EnqueueFrameAndTriggerFrameDelivery(
 }
 
 void GpuVideoDecoder::ReusePictureBuffer(int64 picture_buffer_id) {
-  if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
-    gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::ReusePictureBuffer, this, picture_buffer_id));
-    return;
-  }
+  DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());
   CHECK_GE(available_pictures_, 0);
   available_pictures_++;
 
@@ -495,7 +507,7 @@ void GpuVideoDecoder::PutSHM(SHMBuffer* shm_buffer) {
 void GpuVideoDecoder::NotifyEndOfBitstreamBuffer(int32 id) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::NotifyEndOfBitstreamBuffer, this, id));
+        &GpuVideoDecoder::NotifyEndOfBitstreamBuffer, weak_this_, id));
     return;
   }
 
@@ -547,13 +559,13 @@ void GpuVideoDecoder::EnsureDemuxOrDecode() {
   demuxer_read_in_progress_ = true;
   gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
       &DemuxerStream::Read, demuxer_stream_.get(),
-      base::Bind(&GpuVideoDecoder::RequestBufferDecode, this)));
+      base::Bind(&GpuVideoDecoder::RequestBufferDecode, weak_this_)));
 }
 
 void GpuVideoDecoder::NotifyFlushDone() {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::NotifyFlushDone, this));
+        &GpuVideoDecoder::NotifyFlushDone, weak_this_));
     return;
   }
   DCHECK_EQ(state_, kDrainingDecoder);
@@ -564,7 +576,7 @@ void GpuVideoDecoder::NotifyFlushDone() {
 void GpuVideoDecoder::NotifyResetDone() {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::NotifyResetDone, this));
+        &GpuVideoDecoder::NotifyResetDone, weak_this_));
     return;
   }
 
@@ -587,7 +599,7 @@ void GpuVideoDecoder::NotifyResetDone() {
 void GpuVideoDecoder::NotifyError(media::VideoDecodeAccelerator::Error error) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::NotifyError, this, error));
+        &GpuVideoDecoder::NotifyError, weak_this_, error));
     return;
   }
   if (!vda_.get())