Remove threading from RendererGpuVideoAcceleratorFactories

content/renderer/media/renderer_gpu_video_accelerator_factories.cc

Index: content/renderer/media/renderer_gpu_video_accelerator_factories.cc
diff --git a/content/renderer/media/renderer_gpu_video_accelerator_factories.cc b/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
index 8df76b0931141eb1c20c6df16cedb75fb977b5fc..5ba28845fbfd6edce5c769cb16afe10a7b9f2dc3 100644
--- a/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
+++ b/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
@@ -25,16 +25,13 @@ RendererGpuVideoAcceleratorFactories::RendererGpuVideoAcceleratorFactories(
     : message_loop_(message_loop),
       gpu_channel_host_(gpu_channel_host),
       context_provider_(context_provider),
-      thread_safe_sender_(ChildThread::current()->thread_safe_sender()),
-      aborted_waiter_(true, false),
-      message_loop_async_waiter_(false, false) {
+      thread_safe_sender_(ChildThread::current()->thread_safe_sender()) {
   // |context_provider_| is only required to support HW-accelerated decode.
   if (!context_provider_)
     return;
 
   if (message_loop_->BelongsToCurrentThread()) {
     AsyncBindContext();
-    message_loop_async_waiter_.Reset();
     return;
   }
   // Wait for the context to be acquired.
@@ -45,13 +42,8 @@ RendererGpuVideoAcceleratorFactories::RendererGpuVideoAcceleratorFactories(
                  // stored in a scoped_refptr.  Safe because the Wait() below
                  // keeps us alive until this task completes.

[Ami GONE FROM CHROMIUM, 2013/10/17 22:22:17]

That the comment is a lie is a problem.
The Unretained() in the next line is made unsafe by the lack of a wait following
the post (deleted l.48).

[sheu, 2013/10/21 05:17:35]

Done.

                  base::Unretained(this)));
-  message_loop_async_waiter_.Wait();
 }
 
-RendererGpuVideoAcceleratorFactories::RendererGpuVideoAcceleratorFactories()
-    : aborted_waiter_(true, false),
-      message_loop_async_waiter_(false, false) {}
-
 WebGraphicsContext3DCommandBufferImpl*
 RendererGpuVideoAcceleratorFactories::GetContext3d() {
   DCHECK(message_loop_->BelongsToCurrentThread());
@@ -71,40 +63,21 @@ void RendererGpuVideoAcceleratorFactories::AsyncBindContext() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   if (!context_provider_->BindToCurrentThread())
     context_provider_ = NULL;
-  message_loop_async_waiter_.Signal();
 }
 
 scoped_ptr<media::VideoDecodeAccelerator>
 RendererGpuVideoAcceleratorFactories::CreateVideoDecodeAccelerator(
     media::VideoCodecProfile profile,
     media::VideoDecodeAccelerator::Client* client) {
-  if (message_loop_->BelongsToCurrentThread()) {
-    AsyncCreateVideoDecodeAccelerator(profile, client);
-    message_loop_async_waiter_.Reset();
-    return vda_.Pass();
-  }
-  // The VDA is returned in the vda_ member variable by the
-  // AsyncCreateVideoDecodeAccelerator() function.
-  message_loop_->PostTask(FROM_HERE,
-                          base::Bind(&RendererGpuVideoAcceleratorFactories::
-                                         AsyncCreateVideoDecodeAccelerator,
-                                     this,
-                                     profile,
-                                     client));
-
-  base::WaitableEvent* objects[] = {&aborted_waiter_,
-                                    &message_loop_async_waiter_};
-  if (base::WaitableEvent::WaitMany(objects, arraysize(objects)) == 0) {
-    // If we are aborting and the VDA is created by the
-    // AsyncCreateVideoDecodeAccelerator() function later we need to ensure
-    // that it is destroyed on the same thread.
-    message_loop_->PostTask(FROM_HERE,
-                            base::Bind(&RendererGpuVideoAcceleratorFactories::
-                                           AsyncDestroyVideoDecodeAccelerator,
-                                       this));
-    return scoped_ptr<media::VideoDecodeAccelerator>();
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  WebGraphicsContext3DCommandBufferImpl* context = GetContext3d();
+  if (context && context->GetCommandBufferProxy()) {
+    return gpu_channel_host_->CreateVideoDecoder(
+        context->GetCommandBufferProxy()->GetRouteID(), profile, client);
   }
-  return vda_.Pass();
+
+  return scoped_ptr<media::VideoDecodeAccelerator>();
 }
 
 scoped_ptr<media::VideoEncodeAccelerator>
@@ -115,19 +88,6 @@ RendererGpuVideoAcceleratorFactories::CreateVideoEncodeAccelerator(
   return gpu_channel_host_->CreateVideoEncoder(client);
 }
 
-void RendererGpuVideoAcceleratorFactories::AsyncCreateVideoDecodeAccelerator(
-    media::VideoCodecProfile profile,
-    media::VideoDecodeAccelerator::Client* client) {
-  DCHECK(message_loop_->BelongsToCurrentThread());
-
-  WebGraphicsContext3DCommandBufferImpl* context = GetContext3d();
-  if (context && context->GetCommandBufferProxy()) {
-    vda_ = gpu_channel_host_->CreateVideoDecoder(
-        context->GetCommandBufferProxy()->GetRouteID(), profile, client);
-  }
-  message_loop_async_waiter_.Signal();
-}
-
 uint32 RendererGpuVideoAcceleratorFactories::CreateTextures(
     int32 count,
     const gfx::Size& size,
@@ -204,37 +164,32 @@ void RendererGpuVideoAcceleratorFactories::WaitSyncPoint(uint32 sync_point) {
 void RendererGpuVideoAcceleratorFactories::ReadPixels(uint32 texture_id,

[Ami GONE FROM CHROMIUM, 2013/10/17 22:22:17]

I believe this is called from the compositing thread (or the renderer's
main/Child thread if the threaded compositor is not enabled).
I don't think the conditional trampoline is necessary (this should never be
getting called from the media thread).

[sheu, 2013/10/21 05:17:35]

Gonna refactor the trampolines out of this class.

                                                       const gfx::Size& size,
                                                       const SkBitmap& pixels) {
-  // SkBitmaps use the SkPixelRef object to refcount the underlying pixels.
-  // Multiple SkBitmaps can share a SkPixelRef instance. We use this to
-  // ensure that the underlying pixels in the SkBitmap passed in remain valid
-  // until the AsyncReadPixels() call completes.
-  read_pixels_bitmap_.setPixelRef(pixels.pixelRef());
-
+  base::WaitableEvent waiter(true, false);
   if (!message_loop_->BelongsToCurrentThread()) {
     message_loop_->PostTask(
         FROM_HERE,
         base::Bind(&RendererGpuVideoAcceleratorFactories::AsyncReadPixels,
                    this,
                    texture_id,
-                   size));
-    base::WaitableEvent* objects[] = {&aborted_waiter_,
-                                      &message_loop_async_waiter_};
-    if (base::WaitableEvent::WaitMany(objects, arraysize(objects)) == 0)
-      return;
+                   size,
+                   pixels,
+                   &waiter));
+    waiter.Wait();

[Ami GONE FROM CHROMIUM, 2013/10/17 22:22:17]

This is only deadlock-safe if the media thread is guaranteed never to be waiting
on the compositing/render thread.
It used to be that CreateSharedMemory would be that deadlock-causer, because it
trampolined to the main thread from the media thread, but that's no longer the
case.
I *think* this is safe now, but I am not _confident_ it the safety :(

+    return;
   } else {
-    AsyncReadPixels(texture_id, size);
-    message_loop_async_waiter_.Reset();
+    AsyncReadPixels(texture_id, size, pixels, &waiter);
   }
-  read_pixels_bitmap_.setPixelRef(NULL);
 }
 
 void RendererGpuVideoAcceleratorFactories::AsyncReadPixels(
     uint32 texture_id,
-    const gfx::Size& size) {
+    const gfx::Size& size,
+    const SkBitmap& pixels,
+    base::WaitableEvent* waiter) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   WebGraphicsContext3DCommandBufferImpl* context = GetContext3d();
   if (!context) {
-    message_loop_async_waiter_.Signal();
+    waiter->Signal();
     return;
   }
 
@@ -262,11 +217,11 @@ void RendererGpuVideoAcceleratorFactories::AsyncReadPixels(
                     size.height(),
                     GL_BGRA_EXT,
                     GL_UNSIGNED_BYTE,
-                    read_pixels_bitmap_.pixelRef()->pixels());
+                    pixels.pixelRef()->pixels());
   gles2->DeleteFramebuffers(1, &fb);
   gles2->DeleteTextures(1, &tmp_texture);
   DCHECK_EQ(gles2->GetError(), static_cast<GLenum>(GL_NO_ERROR));
-  message_loop_async_waiter_.Signal();
+  waiter->Signal();
 }
 
 base::SharedMemory* RendererGpuVideoAcceleratorFactories::CreateSharedMemory(
@@ -280,28 +235,4 @@ RendererGpuVideoAcceleratorFactories::GetMessageLoop() {
   return message_loop_;
 }
 
-void RendererGpuVideoAcceleratorFactories::Abort() { aborted_waiter_.Signal(); }
-
-bool RendererGpuVideoAcceleratorFactories::IsAborted() {
-  return aborted_waiter_.IsSignaled();
-}
-
-scoped_refptr<RendererGpuVideoAcceleratorFactories>
-RendererGpuVideoAcceleratorFactories::Clone() {
-  scoped_refptr<RendererGpuVideoAcceleratorFactories> factories =
-      new RendererGpuVideoAcceleratorFactories();
-  factories->message_loop_ = message_loop_;
-  factories->gpu_channel_host_ = gpu_channel_host_;
-  factories->context_provider_ = context_provider_;
-  factories->thread_safe_sender_ = thread_safe_sender_;
-  return factories;
-}
-
-void
-RendererGpuVideoAcceleratorFactories::AsyncDestroyVideoDecodeAccelerator() {
-  // OK to release because Destroy() will delete the VDA instance.
-  if (vda_)
-    vda_.release()->Destroy();
-}
-
 }  // namespace content