VideoDecodeAccelerator now SupportsWeakPtr instead of being RefCountedThreadSafe.

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 7415db87ac0bb4b4513313a1683fbfc82ea49a8c..fa68dbf885f0819bd75099307198b4cfe9d379c4 100644
--- a/media/filters/gpu_video_decoder.cc
+++ b/media/filters/gpu_video_decoder.cc
@@ -72,7 +72,7 @@ void GpuVideoDecoder::Reset(const base::Closure& closure)  {
   // Throw away any already-decoded, not-yet-delivered frames.
   ready_video_frames_.clear();
 
-  if (!vda_) {
+  if (!vda_.get()) {
     closure.Run();
     return;
   }
@@ -94,7 +94,7 @@ void GpuVideoDecoder::Reset(const base::Closure& closure)  {
   }
 
   vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-      &VideoDecodeAccelerator::Reset, vda_));
+      &VideoDecodeAccelerator::Reset, vda_->AsWeakPtr()));

[Ryan Sleevi, 2012/07/16 21:26:07]

Now, you're posing vda_ as a WeakPtr to vda_loop_proxy_. If vda_loop_proxy_ ==
gvd_loop_proxy_, this is generally safe, because
vda_loop_proxy_->BelongsToCurrentThread() must also be true.

However, from this code, it would seem that
gvd_loop_proxy_->BelongsToCurrentThread() is false.

If vda_ has no outstanding WeakPtr<>s, or they have all been invalidated, then
AsWeakPtr<> is going to return a WeakPtr<> that is bound to the /current/ thread
(gvd_loop_proxy_), and such a WeakPtr /should not/ be de-referenced on
vda_loop_proxy_, which is exactly what will happen, since vda_ is bound as the
first argument.

If there /is/ an existing WeakPtr<>, and /if/ it was first created on
vda_loop_proxy_, then this is safe. But that's a very big if, and one that
cannot be seen just from looking at this code.

For example, when I look to where vda_ is created (line 145-146), I see that
it's on gvd_loop_proxy_ (line 120), and factories_->CreateVideoDecodeAccelerator
is a synchronous call, so I /know/ that vda_loop_proxy_ cannot have been invoked
to bind this object.

[Ami GONE FROM CHROMIUM, 2012/07/16 21:31:58]

Blort, you were too quick for me.
This file in rietveld reflects an out-of-date version.
In the coming code I'm making vda_ be set on vda_loop_proxy_ and tracking a
(new) weak_vda_ ptr on that thread to do the posting from GVD->VDA.
While it's not easy to see, CreateVDA appears synchronous but really does create
the object on the right thread.

 }
 
 void GpuVideoDecoder::Stop(const base::Closure& closure) {
@@ -103,13 +103,14 @@ void GpuVideoDecoder::Stop(const base::Closure& closure) {
         &GpuVideoDecoder::Stop, this, closure));
     return;
   }
-  if (!vda_) {
+  if (!vda_.get()) {
     closure.Run();
     return;
   }
+  VideoDecodeAccelerator* vda = vda_.get();
   vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-      &VideoDecodeAccelerator::Destroy, vda_));
-  vda_ = NULL;
+      &VideoDecodeAccelerator::Destroy, base::Unretained(vda),
+      base::Passed(&vda_)));
   closure.Run();
 }
 
@@ -141,8 +142,8 @@ void GpuVideoDecoder::Initialize(const scoped_refptr<DemuxerStream>& stream,
     return;
   }
 
-  vda_ = factories_->CreateVideoDecodeAccelerator(config.profile(), this);
-  if (!vda_) {
+  vda_.reset(factories_->CreateVideoDecodeAccelerator(config.profile(), this));
+  if (!vda_.get()) {
     status_cb.Run(DECODER_ERROR_NOT_SUPPORTED);
     return;
   }
@@ -171,7 +172,7 @@ void GpuVideoDecoder::Read(const ReadCB& read_cb) {
     return;
   }
 
-  if (!vda_) {
+  if (!vda_.get()) {
     read_cb.Run(kOk, VideoFrame::CreateEmptyFrame());
     return;
   }
@@ -218,7 +219,7 @@ void GpuVideoDecoder::RequestBufferDecode(
     return;
   }
 
-  if (!vda_) {
+  if (!vda_.get()) {
     EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
     return;
   }
@@ -227,7 +228,7 @@ void GpuVideoDecoder::RequestBufferDecode(
     if (state_ == kNormal) {
       state_ = kDrainingDecoder;
       vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-          &VideoDecodeAccelerator::Flush, vda_));
+          &VideoDecodeAccelerator::Flush, vda_->AsWeakPtr()));
     }
     return;
   }
@@ -243,7 +244,7 @@ void GpuVideoDecoder::RequestBufferDecode(
   RecordBufferTimeData(bitstream_buffer, *buffer);
 
   vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-      &VideoDecodeAccelerator::Decode, vda_, bitstream_buffer));
+      &VideoDecodeAccelerator::Decode, vda_->AsWeakPtr(), bitstream_buffer));
 }
 
 void GpuVideoDecoder::RecordBufferTimeData(
@@ -319,7 +320,7 @@ void GpuVideoDecoder::ProvidePictureBuffers(uint32 count,
     return;
   }
 
-  if (!vda_)
+  if (!vda_.get())
     return;
 
   std::vector<PictureBuffer> picture_buffers;
@@ -331,7 +332,8 @@ void GpuVideoDecoder::ProvidePictureBuffers(uint32 count,
     DCHECK(inserted);
   }
   vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-      &VideoDecodeAccelerator::AssignPictureBuffers, vda_, picture_buffers));
+      &VideoDecodeAccelerator::AssignPictureBuffers, vda_->AsWeakPtr(),
+      picture_buffers));
 }
 
 void GpuVideoDecoder::DismissPictureBuffer(int32 id) {
@@ -409,10 +411,11 @@ void GpuVideoDecoder::ReusePictureBuffer(int64 picture_buffer_id) {
         &GpuVideoDecoder::ReusePictureBuffer, this, picture_buffer_id));
     return;
   }
-  if (!vda_)
+  if (!vda_.get())
     return;
   vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-      &VideoDecodeAccelerator::ReusePictureBuffer, vda_, picture_buffer_id));
+      &VideoDecodeAccelerator::ReusePictureBuffer, vda_->AsWeakPtr(),
+      picture_buffer_id));
 }
 
 GpuVideoDecoder::SHMBuffer* GpuVideoDecoder::GetSHM(size_t min_size) {
@@ -467,7 +470,7 @@ void GpuVideoDecoder::NotifyEndOfBitstreamBuffer(int32 id) {
 }
 
 GpuVideoDecoder::~GpuVideoDecoder() {
-  DCHECK(!vda_);  // Stop should have been already called.
+  DCHECK(!vda_.get());  // Stop should have been already called.
   DCHECK(pending_read_cb_.is_null());
   for (size_t i = 0; i < available_shm_segments_.size(); ++i) {
     available_shm_segments_[i]->shm->Close();
@@ -510,7 +513,7 @@ void GpuVideoDecoder::NotifyResetDone() {
     return;
   }
 
-  if (!vda_)
+  if (!vda_.get())
     return;
 
   DCHECK(ready_video_frames_.empty());
@@ -532,9 +535,9 @@ void GpuVideoDecoder::NotifyError(media::VideoDecodeAccelerator::Error error) {
         &GpuVideoDecoder::NotifyError, this, error));
     return;
   }
-  if (!vda_)
+  if (!vda_.get())
     return;
-  vda_ = NULL;
+  vda_loop_proxy_->DeleteSoon(FROM_HERE, vda_.release());
   DLOG(ERROR) << "VDA Error: " << error;
 
   error_occured_ = true;