Implement proper synchronization between HW video decode IPC and CommandBuffer.

content/renderer/gpu/gpu_video_decode_accelerator_host.cc

Index: content/renderer/gpu/gpu_video_decode_accelerator_host.cc
diff --git a/content/renderer/gpu/gpu_video_decode_accelerator_host.cc b/content/renderer/gpu/gpu_video_decode_accelerator_host.cc
index 4f08dab0826428afed664c7df677c2a05b086342..bd4e0874ee30ece6d712fb7dd4de96ea277c5790 100644
--- a/content/renderer/gpu/gpu_video_decode_accelerator_host.cc
+++ b/content/renderer/gpu/gpu_video_decode_accelerator_host.cc
@@ -4,14 +4,18 @@
 
 #include "content/renderer/gpu/gpu_video_decode_accelerator_host.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "base/shared_memory.h"
 #include "base/task.h"
 #include "content/common/gpu/gpu_messages.h"
 #include "content/common/view_messages.h"
 #include "content/renderer/render_thread.h"
+#include "gpu/command_buffer/client/cmd_buffer_helper.h"
 #include "ipc/ipc_message_macros.h"
 #include "ipc/ipc_message_utils.h"
+#include "ipc/ipc_platform_file.h"
 
 using media::VideoDecodeAccelerator;
 
@@ -19,14 +23,17 @@ GpuVideoDecodeAcceleratorHost::GpuVideoDecodeAcceleratorHost(
     MessageRouter* router,
     IPC::Message::Sender* ipc_sender,
     int32 decoder_host_id,
-    uint32 command_buffer_route_id,
+    int32 command_buffer_route_id,
+    gpu::CommandBufferHelper* cmd_buffer_helper,
     VideoDecodeAccelerator::Client* client)
     : router_(router),
       ipc_sender_(ipc_sender),
       decoder_host_id_(decoder_host_id),
-      decoder_id_(0),
+      decoder_id_(-1),
       command_buffer_route_id_(command_buffer_route_id),
+      cmd_buffer_helper_(cmd_buffer_helper),
       client_(client) {
+  message_loop_ = MessageLoop::current();
 }
 
 GpuVideoDecodeAcceleratorHost::~GpuVideoDecodeAcceleratorHost() {}
@@ -65,6 +72,14 @@ bool GpuVideoDecodeAcceleratorHost::OnMessageReceived(const IPC::Message& msg) {
   return handled;
 }
 
+std::pair<int32, int32> GpuVideoDecodeAcceleratorHost::SyncTokens() {
+  std::pair<int32, int32> tokens;
+  tokens.first = cmd_buffer_helper_->last_token_read();
+  tokens.second = cmd_buffer_helper_->InsertToken();
+  cmd_buffer_helper_->Flush();
+  return tokens;
+}
+
 bool GpuVideoDecodeAcceleratorHost::GetConfigs(
     const std::vector<uint32>& requested_configs,
     std::vector<uint32>* matched_configs) {
@@ -75,34 +90,45 @@ bool GpuVideoDecodeAcceleratorHost::GetConfigs(
 
 bool GpuVideoDecodeAcceleratorHost::Initialize(
     const std::vector<uint32>& configs) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
   router_->AddRoute(decoder_host_id_, this);
 
   // Temporarily save configs for after create is done and we're
   // ready to initialize.
   configs_ = configs;
-
   if (!ipc_sender_->Send(new GpuChannelMsg_CreateVideoDecoder(
-          decoder_id_, command_buffer_route_id_, configs))) {
+          decoder_host_id_, command_buffer_route_id_, configs))) {
     LOG(ERROR) << "Send(GpuChannelMsg_CreateVideoDecoder) failed";
     return false;
   }
   return true;
 }
 
-bool GpuVideoDecodeAcceleratorHost::Decode(
+void GpuVideoDecodeAcceleratorHost::Decode(
     const media::BitstreamBuffer& bitstream_buffer) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &GpuVideoDecodeAcceleratorHost::Decode,
+        base::Unretained(this), bitstream_buffer));
+    return;
+  }

[piman, 2011/06/28 01:19:22]

I've seen this pattern before, and I can't say that I like it. It would be
better to have explicit "DecodeOnIOThread"/"DecodeOnRenderThread" functions,
making sure the Render thread one calls the IO thread one.
Also, posting a task with Unretained(this) seems dangerous at best. What ensures
that this will not go away before the task runs ? Most likely you need to add a
(thread-safe) refcount to GpuVideoDecodeAcceleratorHost and remove Unretained.
Is it even valid to copy bitstream_buffer to another thread ?
Lastly, it seems that the root of the problem is that ipc_sender_ is the raw
channel, instead of the SyncChannel represented by the GpuChannelHost. Wouldn't
it be much simpler to use the latter, and send everything from the Render
thread, skipping thread hopping (already handled by the SyncChannel) ?

[Ami GONE FROM CHROMIUM, 2011/06/28 21:00:53]

As you tried to explain to me on friday, but I didn't actually grok until this
morning, the way the code was set up here was insane.  We were taking extra
pains during construction to situate the GVDAH on the IO thread, and
trampolining all the Send()'s to the IO thread (b/c we expect to be called from
the render thread, b/c what else could a plugin do?), *and* trampolining all the
callbacks from the IO thread to the render thread (b/c where else could the
plugin expect to receive them?).  That's basically reimplementing what
SyncChannel/ChannelProxy is for, and GpuChannelHost already has one of them.  So
instead, I ripped out all of this explicit thread-hopping and replaced with
using GpuChannelHost's SyncChannel.  

   if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_Decode(
-          decoder_id_, bitstream_buffer.id(),
-          bitstream_buffer.handle(), bitstream_buffer.size()))) {
+          decoder_id_, SyncTokens(), bitstream_buffer.handle(),
+          bitstream_buffer.id(), bitstream_buffer.size()))) {
     DLOG(ERROR) << "Send(AcceleratedVideoDecoderMsg_Decode) failed";
-    return false;
+    // TODO(fischman/vrk): signal error to client.
+    return;
   }
-
-  return true;
 }
 
 void GpuVideoDecodeAcceleratorHost::AssignGLESBuffers(
     const std::vector<media::GLESBuffer>& buffers) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &GpuVideoDecodeAcceleratorHost::AssignGLESBuffers,
+        base::Unretained(this), buffers));
+    return;
+  }
   // Rearrange data for IPC command.
   std::vector<int32> buffer_ids;
   std::vector<uint32> texture_ids;
@@ -113,40 +139,66 @@ void GpuVideoDecodeAcceleratorHost::AssignGLESBuffers(
     buffer_ids.push_back(buffer.id());
     sizes.push_back(buffer.size());
   }
-  if (!ipc_sender_->Send(new GpuChannelMsg_AssignTexturesToVideoDecoder(
-          decoder_id_, buffer_ids, texture_ids, sizes))) {
+  if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_AssignTextures(
+          decoder_id_, SyncTokens(), buffer_ids, texture_ids, sizes))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderMsg_AssignGLESBuffers) failed";
   }
 }
 
 void GpuVideoDecodeAcceleratorHost::AssignSysmemBuffers(
     const std::vector<media::SysmemBuffer>& buffers) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &GpuVideoDecodeAcceleratorHost::AssignSysmemBuffers,
+        base::Unretained(this), buffers));
+    return;
+  }
   // TODO(vrk): Implement.
   NOTIMPLEMENTED();
 }
 
 void GpuVideoDecodeAcceleratorHost::ReusePictureBuffer(
     int32 picture_buffer_id) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &GpuVideoDecodeAcceleratorHost::ReusePictureBuffer,
+        base::Unretained(this), picture_buffer_id));
+    return;
+  }
   if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_ReusePictureBuffer(
-          decoder_id_, picture_buffer_id))) {
+          decoder_id_, SyncTokens(), picture_buffer_id))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderMsg_ReusePictureBuffer) failed";
   }
 }
 
-bool GpuVideoDecodeAcceleratorHost::Flush() {
-  if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_Flush(decoder_id_))) {
+void GpuVideoDecodeAcceleratorHost::Flush() {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &GpuVideoDecodeAcceleratorHost::Flush,
+        base::Unretained(this)));
+    return;
+  }
+  if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_Flush(
+          decoder_id_, SyncTokens()))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderMsg_Flush) failed";
-    return false;
+    // TODO(fischman/vrk): signal error to client.
+    return;
   }
-  return true;
 }
 
-bool GpuVideoDecodeAcceleratorHost::Abort() {
-  if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_Abort(decoder_id_))) {
+void GpuVideoDecodeAcceleratorHost::Abort() {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &GpuVideoDecodeAcceleratorHost::Abort,
+        base::Unretained(this)));
+    return;
+  }
+  if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_Abort(
+          decoder_id_, SyncTokens()))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderMsg_Abort) failed";
-    return false;
+    // TODO(fischman/vrk): signal error to client.
+    return;
   }
-  return true;
 }
 
 void GpuVideoDecodeAcceleratorHost::OnBitstreamBufferProcessed(
@@ -172,7 +224,7 @@ void GpuVideoDecodeAcceleratorHost::OnDismissPictureBuffer(
 void GpuVideoDecodeAcceleratorHost::OnCreateDone(int32 decoder_id) {
   decoder_id_ = decoder_id;
   if (!ipc_sender_->Send(new AcceleratedVideoDecoderMsg_Initialize(
-      decoder_id_, configs_))) {
+          decoder_id_, SyncTokens(), configs_))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderMsg_Initialize) failed";
   }
 }