GVDA: Ensure VDA is destroyed while the stub is still valid.

We should not be letting go of the stub while the VDA is still alive, as it
may require GL context while decoding, and/or to do its cleanup.

However, when using the IO thread message filter, we postpone destroying
VDA and return from OnWillDestroyStub, after which the stub will not valid.

To prevent this, move destruction of VDA to the stub destruction observer.
We have to be careful not to destroy it before the IO thread message filter
is removed however, because we cannot take the VDA away while messages may
still be serviced on IO thread. We cannot simply ensure the existence of
VDA on the IO thread to prevent this, because that would require
synchronization of the IO thread with the ChildThread. So we have to
synchronously wait on the ChildThread for the filter to be removed instead,
before returning from the stub destruction
observer.

Also, remove the DCHECK(stub_) checks, which don't really do anything, as
stub_ is DCHECKed in constructor and never changed afterwards.

Finally, fix a DCHECK in EVDA; StopDevicePoll, which may be called - when
the decoder thread is stopped - from the ChildThread.

BUG=326280, chrome-os-partner:24390
TEST=video playback, vdaunittest, confirming leaks are gone

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

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

3 files changed, 35 insertions, 20 deletions

diff --git a/content/common/gpu/media/exynos_video_decode_accelerator.cc b/content/common/gpu/media/exynos_video_decode_accelerator.cc
index cfa3443..b74d4d2 100644
--- a/content/common/gpu/media/exynos_video_decode_accelerator.cc
+++ b/content/common/gpu/media/exynos_video_decode_accelerator.cc
@@ -1483,7 +1483,8 @@ bool ExynosVideoDecodeAccelerator::StartDevicePoll() {
 
 bool ExynosVideoDecodeAccelerator::StopDevicePoll(bool keep_mfc_input_state) {
   DVLOG(3) << "StopDevicePoll()";
-  DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
+  if (decoder_thread_.IsRunning())
+    DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
 
   // Signal the DevicePollTask() to stop, and stop the device poll thread.
   if (!SetDevicePollInterrupt())
@@ -1897,6 +1898,8 @@ void ExynosVideoDecodeAccelerator::DestroyMfcOutputBuffers() {
   DCHECK(!mfc_output_streamon_);
 
   if (mfc_output_buffer_map_.size() != 0) {
+    // TODO(sheu, posciak): Making the context current should not be required
+    // anymore. Remove it and verify (crbug.com/327869).
     if (!make_context_current_.Run()) {
       DLOG(ERROR) << "DestroyMfcOutputBuffers(): "
                   << "could not make context current";
diff --git a/content/common/gpu/media/gpu_video_decode_accelerator.cc b/content/common/gpu/media/gpu_video_decode_accelerator.cc
index 32e6b67..b932258 100644
--- a/content/common/gpu/media/gpu_video_decode_accelerator.cc
+++ b/content/common/gpu/media/gpu_video_decode_accelerator.cc
@@ -132,6 +132,7 @@ GpuVideoDecodeAccelerator::GpuVideoDecodeAccelerator(
       host_route_id_(host_route_id),
       stub_(stub),
       texture_target_(0),
+      filter_removed_(true, false),
       io_message_loop_(io_message_loop),
       weak_factory_for_io_(this) {
   DCHECK(stub_);
@@ -143,14 +144,15 @@ GpuVideoDecodeAccelerator::GpuVideoDecodeAccelerator(
 }
 
 GpuVideoDecodeAccelerator::~GpuVideoDecodeAccelerator() {
-  if (video_decode_accelerator_)
-    video_decode_accelerator_.release()->Destroy();
+  // This class can only be self-deleted from OnWillDestroyStub(), which means
+  // the VDA has already been destroyed in there.
+  CHECK(!video_decode_accelerator_.get());
 }
 
 bool GpuVideoDecodeAccelerator::OnMessageReceived(const IPC::Message& msg) {
-  DCHECK(stub_);
   if (!video_decode_accelerator_)
     return false;
+
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(GpuVideoDecodeAccelerator, msg)
     IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_Decode, OnDecode)
@@ -245,7 +247,6 @@ void GpuVideoDecodeAccelerator::NotifyError(
 void GpuVideoDecodeAccelerator::Initialize(
     const media::VideoCodecProfile profile,
     IPC::Message* init_done_msg) {
-  DCHECK(stub_);
   DCHECK(!video_decode_accelerator_.get());
   DCHECK(!init_done_msg_);
   DCHECK(init_done_msg);
@@ -329,7 +330,6 @@ void GpuVideoDecodeAccelerator::OnDecode(
 void GpuVideoDecodeAccelerator::OnAssignPictureBuffers(
     const std::vector<int32>& buffer_ids,
     const std::vector<uint32>& texture_ids) {
-  DCHECK(stub_);
   if (buffer_ids.size() != texture_ids.size()) {
     NotifyError(media::VideoDecodeAccelerator::INVALID_ARGUMENT);
     return;
@@ -427,7 +427,7 @@ void GpuVideoDecodeAccelerator::OnDestroy() {
 void GpuVideoDecodeAccelerator::OnFilterRemoved() {
   // We're destroying; cancel all callbacks.
   weak_factory_for_io_.InvalidateWeakPtrs();
-  child_message_loop_->DeleteSoon(FROM_HERE, this);
+  filter_removed_.Signal();
 }
 
 void GpuVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer(
@@ -459,24 +459,29 @@ void GpuVideoDecodeAccelerator::NotifyResetDone() {
 }
 
 void GpuVideoDecodeAccelerator::OnWillDestroyStub() {
-  DCHECK(stub_);
-  stub_->channel()->RemoveRoute(host_route_id_);
-  stub_->RemoveDestructionObserver(this);
-  {
-    DebugAutoLock auto_lock(debug_uncleared_textures_lock_);
-    uncleared_textures_.clear();
-  }
+  // The stub is going away, so we have to stop and destroy VDA here, before
+  // returning, because the VDA may need the GL context to run and/or do its
+  // cleanup. We cannot destroy the VDA before the IO thread message filter is
+  // removed however, since we cannot service incoming messages with VDA gone.
+  // We cannot simply check for existence of VDA on IO thread though, because
+  // we don't want to synchronize the IO thread with the ChildThread.
+  // So we have to wait for the RemoveFilter callback here instead and remove
+  // the VDA after it arrives and before returning.
   if (filter_.get()) {
-    // Remove the filter first because the member variables can be accessed on
-    // IO thread. When filter is removed, OnFilterRemoved will delete |this|.
     stub_->channel()->RemoveFilter(filter_.get());
-  } else {
-    delete this;
+    filter_removed_.Wait();
   }
+
+  stub_->channel()->RemoveRoute(host_route_id_);
+  stub_->RemoveDestructionObserver(this);
+
+  if (video_decode_accelerator_)
+    video_decode_accelerator_.release()->Destroy();
+
+  delete this;
 }
 
 bool GpuVideoDecodeAccelerator::Send(IPC::Message* message) {
-  DCHECK(stub_);
   if (filter_.get() && io_message_loop_->BelongsToCurrentThread())
     return filter_->SendOnIOThread(message);
   DCHECK(child_message_loop_->BelongsToCurrentThread());
diff --git a/content/common/gpu/media/gpu_video_decode_accelerator.h b/content/common/gpu/media/gpu_video_decode_accelerator.h
index 47f6425..c22608b 100644
--- a/content/common/gpu/media/gpu_video_decode_accelerator.h
+++ b/content/common/gpu/media/gpu_video_decode_accelerator.h
@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
+#include "base/synchronization/waitable_event.h"
 #include "content/common/gpu/gpu_command_buffer_stub.h"
 #include "content/common/gpu/media/video_decode_accelerator_impl.h"
 #include "gpu/command_buffer/service/texture_manager.h"
@@ -38,7 +39,6 @@ class GpuVideoDecodeAccelerator
       int32 host_route_id,
       GpuCommandBufferStub* stub,
       const scoped_refptr<base::MessageLoopProxy>& io_message_loop);
-  virtual ~GpuVideoDecodeAccelerator();
 
   // IPC::Listener implementation.
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
@@ -71,6 +71,9 @@ class GpuVideoDecodeAccelerator
  private:
   class MessageFilter;
 
+  // We only allow self-delete, from OnWillDestroyStub(), after cleanup there.
+  virtual ~GpuVideoDecodeAccelerator();
+
   // Handlers for IPC messages.
   void OnDecode(base::SharedMemoryHandle handle, int32 id, uint32 size);
   void OnAssignPictureBuffers(const std::vector<int32>& buffer_ids,
@@ -113,6 +116,10 @@ class GpuVideoDecodeAccelerator
   // The message filter to run VDA::Decode on IO thread if VDA supports it.
   scoped_refptr<MessageFilter> filter_;
 
+  // Used to wait on for |filter_| to be removed, before we can safely
+  // destroy the VDA.
+  base::WaitableEvent filter_removed_;
+
   // GPU child message loop.
   scoped_refptr<base::MessageLoopProxy> child_message_loop_;