VideoDecodeAccelerator now SupportsWeakPtr instead of being RefCountedThreadSafe.

media/filters/gpu_video_decoder.cc

 // 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.
 
 #include "media/filters/gpu_video_decoder.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
@@ skipping 50 matching lines @@
   DCHECK(gvd_loop_proxy_ && factories_);
 }
 
 void GpuVideoDecoder::Reset(const base::Closure& closure)  {
   if (!gvd_loop_proxy_->BelongsToCurrentThread() ||
       state_ == kDrainingDecoder) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::Reset, this, closure));
     return;
   }
 

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

So the invariant here is that:

gvd_loop_proxy->BelongsToCurrentThread()

   // Throw away any already-decoded, not-yet-delivered frames.
   ready_video_frames_.clear();
 
-  if (!vda_) {
+  if (!vda_.get()) {
     closure.Run();
     return;
   }
 
   DCHECK(pending_reset_cb_.is_null());
   DCHECK(!closure.is_null());
 
   // VideoRendererBase::Flush() can't complete while it has a pending read to
   // us, so we fulfill such a read here.
   if (!pending_read_cb_.is_null())
     EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
 
   if (shutting_down_) {
     // Immediately fire the callback instead of waiting for the reset to
     // complete (which will happen after PipelineImpl::Stop() completes).
     gvd_loop_proxy_->PostTask(FROM_HERE, closure);
   } else {
     pending_reset_cb_ = 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) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &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();
 }
 
 void GpuVideoDecoder::Initialize(const scoped_refptr<DemuxerStream>& stream,
                                  const PipelineStatusCB& orig_status_cb,
                                  const StatisticsCB& statistics_cb) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::Initialize,
         this, stream, orig_status_cb, statistics_cb));
@@ skipping 11 matching lines @@
 
   const VideoDecoderConfig& config = stream->video_decoder_config();
   // TODO(scherkus): this check should go in Pipeline prior to creating
   // decoder objects.
   if (!config.IsValidConfig()) {
     DLOG(ERROR) << "Invalid video stream - " << config.AsHumanReadableString();
     status_cb.Run(PIPELINE_ERROR_DECODE);
     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;
   }
 
   demuxer_stream_ = stream;
   statistics_cb_ = statistics_cb;
 
   demuxer_stream_->EnableBitstreamConverter();
 
   natural_size_ = config.natural_size();
   config_frame_duration_ = GetFrameDuration(config);
 
   DVLOG(1) << "GpuVideoDecoder::Initialize() succeeded.";
   status_cb.Run(PIPELINE_OK);
 }
 
 void GpuVideoDecoder::Read(const ReadCB& read_cb) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::Read, this, read_cb));
     return;
   }
 
   if (error_occured_) {
     read_cb.Run(kDecodeError, NULL);
     return;
   }
 
-  if (!vda_) {
+  if (!vda_.get()) {
     read_cb.Run(kOk, VideoFrame::CreateEmptyFrame());
     return;
   }
 
   DCHECK(pending_reset_cb_.is_null());
   DCHECK(pending_read_cb_.is_null());
   pending_read_cb_ = read_cb;
 
   if (!ready_video_frames_.empty()) {
     EnqueueFrameAndTriggerFrameDelivery(NULL);
@@ skipping 26 matching lines @@
   if (!buffer) {
     if (pending_read_cb_.is_null())
       return;
 
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         pending_read_cb_, kOk, scoped_refptr<VideoFrame>()));
     pending_read_cb_.Reset();
     return;
   }
 
-  if (!vda_) {
+  if (!vda_.get()) {
     EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
     return;
   }
 
   if (buffer->IsEndOfStream()) {
     if (state_ == kNormal) {
       state_ = kDrainingDecoder;
       vda_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-          &VideoDecodeAccelerator::Flush, vda_));
+          &VideoDecodeAccelerator::Flush, vda_->AsWeakPtr()));
     }
     return;
   }
 
   size_t size = buffer->GetDataSize();
   SHMBuffer* shm_buffer = GetSHM(size);
   memcpy(shm_buffer->shm->memory(), buffer->GetData(), size);
   BitstreamBuffer bitstream_buffer(
       next_bitstream_buffer_id_++, shm_buffer->shm->handle(), size);
   bool inserted = bitstream_buffers_in_decoder_.insert(std::make_pair(
       bitstream_buffer.id(), BufferPair(shm_buffer, buffer))).second;
   DCHECK(inserted);
   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(
     const BitstreamBuffer& bitstream_buffer, const Buffer& buffer) {
   base::TimeDelta duration = buffer.GetDuration();
   if (duration == base::TimeDelta())
     duration = config_frame_duration_;
   input_buffer_time_data_.push_front(BufferTimeData(
       bitstream_buffer.id(), buffer.GetTimestamp(), duration));
   // Why this value?  Because why not.  avformat.h:MAX_REORDER_DELAY is 16, but
@@ skipping 55 matching lines @@
   }
 
   std::vector<uint32> texture_ids;
   decoder_texture_target_ = texture_target;
   if (!factories_->CreateTextures(
       count, size, &texture_ids, decoder_texture_target_)) {
     NotifyError(VideoDecodeAccelerator::PLATFORM_FAILURE);
     return;
   }
 
-  if (!vda_)
+  if (!vda_.get())
     return;
 
   std::vector<PictureBuffer> picture_buffers;
   for (size_t i = 0; i < texture_ids.size(); ++i) {
     picture_buffers.push_back(PictureBuffer(
         next_picture_buffer_id_++, size, texture_ids[i]));
     bool inserted = picture_buffers_in_decoder_.insert(std::make_pair(
         picture_buffers.back().id(), picture_buffers.back())).second;
     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) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::DismissPictureBuffer, this, id));
     return;
   }
   std::map<int32, PictureBuffer>::iterator it =
       picture_buffers_in_decoder_.find(id);
@@ skipping 57 matching lines @@
   pending_read_cb_.Reset();
   ready_video_frames_.pop_front();
 }
 
 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;
   }
-  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) {
   DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());
   if (available_shm_segments_.empty() ||
       available_shm_segments_.back()->size < min_size) {
     size_t size_to_allocate = std::max(min_size, kSharedMemorySegmentBytes);
     base::SharedMemory* shm = factories_->CreateSharedMemory(size_to_allocate);
     DCHECK(shm);
     return new SHMBuffer(shm, size_to_allocate);
@@ skipping 34 matching lines @@
 
   if (!pending_read_cb_.is_null() && pending_reset_cb_.is_null() &&
       state_ != kDrainingDecoder &&
       bitstream_buffers_in_decoder_.empty()) {
     DCHECK(ready_video_frames_.empty());
     EnsureDemuxOrDecode();
   }
 }
 
 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();
     delete available_shm_segments_[i];
   }
   available_shm_segments_.clear();
   for (std::map<int32, BufferPair>::iterator it =
            bitstream_buffers_in_decoder_.begin();
        it != bitstream_buffers_in_decoder_.end(); ++it) {
     it->second.shm_buffer->shm->Close();
@@ skipping 22 matching lines @@
   EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
 }
 
 void GpuVideoDecoder::NotifyResetDone() {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::NotifyResetDone, this));
     return;
   }
 
-  if (!vda_)
+  if (!vda_.get())
     return;
 
   DCHECK(ready_video_frames_.empty());
 
   // This needs to happen after the Reset() on vda_ is done to ensure pictures
   // delivered during the reset can find their time data.
   input_buffer_time_data_.clear();
 
   if (!pending_reset_cb_.is_null())
     base::ResetAndReturn(&pending_reset_cb_).Run();
 
   if (!pending_read_cb_.is_null())
     EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
 }
 
 void GpuVideoDecoder::NotifyError(media::VideoDecodeAccelerator::Error error) {
   if (!gvd_loop_proxy_->BelongsToCurrentThread()) {
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
         &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;
 
   if (!pending_read_cb_.is_null()) {
     base::ResetAndReturn(&pending_read_cb_).Run(kDecodeError, NULL);
     return;
   }
 }
 
 }  // namespace media