Implement proper synchronization between HW video decode IPC and CommandBuffer.

content/common/gpu/media/gpu_video_decode_accelerator.cc

Index: content/common/gpu/media/gpu_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/gpu_video_decode_accelerator.cc b/content/common/gpu/media/gpu_video_decode_accelerator.cc
index ffe8096386242301427ec069726d1805a4f3122f..7798d252520e9a70267ebaafda15f303e2e7ae43 100644
--- a/content/common/gpu/media/gpu_video_decode_accelerator.cc
+++ b/content/common/gpu/media/gpu_video_decode_accelerator.cc
@@ -11,25 +11,98 @@
 #include "ipc/ipc_message_macros.h"
 #include "ipc/ipc_message_utils.h"
 #include "content/common/gpu/gpu_channel.h"
+#include "content/common/gpu/gpu_command_buffer_stub.h"
 #include "content/common/gpu/gpu_messages.h"
+#include "content/common/gpu/media/gpu_video_service.h"
 #include "ui/gfx/size.h"
 
 GpuVideoDecodeAccelerator::GpuVideoDecodeAccelerator(
     IPC::Message::Sender* sender,
-    int32 host_route_id)
+    int32 host_route_id,
+    int32 decoder_route_id,
+    GpuCommandBufferStub* stub)
     : sender_(sender),
-      route_id_(host_route_id),
+      host_route_id_(host_route_id),
+      decoder_route_id_(decoder_route_id),
+      stub_(stub),
       video_decode_accelerator_(NULL) {
+  stub_->AddSetTokenCallback(base::Bind(
+      &GpuVideoDecodeAccelerator::OnSetToken, base::Unretained(this)));

[scherkus (not reviewing), 2011/06/27 22:54:47]

follow-up from previous question...

if the lifetime of GCBS > GVDA, then do we need to worry about callbacks
executing into this class post-deletion?

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

Done.

 }
 
 GpuVideoDecodeAccelerator::~GpuVideoDecodeAccelerator() {}

[scherkus (not reviewing), 2011/06/27 22:54:47]

is it possible to get deleted w/ deferred_messages_ non-empty?

if so we need to delete them pointers

check out base/stl_util-inl.h ;)

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

In the glorious future when the BUG is fixed, it will not be possible to end up
in this situation.  Until then I'll add explicit deletion.

 
+// Return true iff |value| is in [min..max), accounting for (up to) one
+// wrap-around.
+static bool CheckInRangeWithWrap(int32 min, int32 max, int32 value) {
+  DCHECK_GE(min, 0);
+  DCHECK_GE(max, 0);
+  if (min <= max) {
+    // value should be in [min .. max)
+    return (value >= min) && (value < max);
+  } else {
+    // value should be in [0 .. max) or [min .. wrap value)
+    return (value >= min) || (value < max);
+  }
+}
+
+void GpuVideoDecodeAccelerator::OnSetToken(int32 token) {
+  // Note: this always retries all deferred messages on every token arrival.

[scherkus (not reviewing), 2011/06/27 22:54:47]

s/Note/TODO ?

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

I'm not sure it's necessary, which is why I didn't TODO it.

+  // There's an optimization to be done here by only trying messages which are
+  // waiting for tokens which are earlier than |token|.
+  std::queue<IPC::Message*> deferred_messages_copy;
+  std::swap(deferred_messages_copy, deferred_messages_);
+  while (!deferred_messages_copy.empty()) {
+    scoped_ptr<IPC::Message> message(deferred_messages_copy.front());
+    deferred_messages_copy.pop();
+    OnMessageReceived(*message);
+  }
+}
+
+bool GpuVideoDecodeAccelerator::DeferMessageIfNeeded(
+    const IPC::Message& msg, bool* deferred) {
+  // Only consider deferring for message types that need it.
+  switch (msg.type()) {
+    case AcceleratedVideoDecoderMsg_GetConfigs::ID:
+    case AcceleratedVideoDecoderMsg_Initialize::ID:
+    case AcceleratedVideoDecoderMsg_Decode::ID:
+    case AcceleratedVideoDecoderMsg_AssignTextures::ID:
+    case AcceleratedVideoDecoderMsg_AssignSysmemBuffers::ID:
+    case AcceleratedVideoDecoderMsg_ReusePictureBuffer::ID:
+    case AcceleratedVideoDecoderMsg_Flush::ID:
+    case AcceleratedVideoDecoderMsg_Abort::ID:
+      break;
+    default:
+      return false;
+  }
+
+  std::pair<int32, int32> tokens;
+  void* iter = NULL;
+  if (!IPC::ParamTraits<std::pair<int32, int32> >::Read(&msg, &iter, &tokens))
+    return false;
+  if (CheckInRangeWithWrap(tokens.first, tokens.second, stub_->token())) {
+    deferred_messages_.push(new IPC::Message(msg));
+    *deferred = true;
+  } else {
+    *deferred = false;
+  }
+  return true;
+}
+
 bool GpuVideoDecodeAccelerator::OnMessageReceived(const IPC::Message& msg) {
+  bool deferred = false;
+  if (!DeferMessageIfNeeded(msg, &deferred))
+    return false;
+  if (deferred)
+    return true;
+
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(GpuVideoDecodeAccelerator, msg)
     IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_GetConfigs, OnGetConfigs)
     IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_Initialize, OnInitialize)
     IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_Decode, OnDecode)
+    IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_AssignTextures,
+                        OnAssignTextures)
     IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_AssignSysmemBuffers,
                         OnAssignSysmemBuffers)
     IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_ReusePictureBuffer,
@@ -54,7 +127,7 @@ void GpuVideoDecodeAccelerator::ProvidePictureBuffers(
     const gfx::Size& dimensions,
     media::VideoDecodeAccelerator::MemoryType type) {
   if (!Send(new AcceleratedVideoDecoderHostMsg_ProvidePictureBuffers(
-          route_id_, requested_num_of_buffers, dimensions, type))) {
+          host_route_id_, requested_num_of_buffers, dimensions, type))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_ProvidePictureBuffers) "
                << "failed";
   }
@@ -64,7 +137,7 @@ void GpuVideoDecodeAccelerator::DismissPictureBuffer(
     int32 picture_buffer_id) {
   // Notify client that picture buffer is now unused.
   if (!Send(new AcceleratedVideoDecoderHostMsg_DismissPictureBuffer(
-          route_id_, picture_buffer_id))) {
+          host_route_id_, picture_buffer_id))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_DismissPictureBuffer) "
                << "failed";
   }
@@ -73,7 +146,7 @@ void GpuVideoDecodeAccelerator::DismissPictureBuffer(
 void GpuVideoDecodeAccelerator::PictureReady(
     const media::Picture& picture) {
   if (!Send(new AcceleratedVideoDecoderHostMsg_PictureReady(
-          route_id_,
+          host_route_id_,
           picture.picture_buffer_id(),
           picture.bitstream_buffer_id(),
           picture.visible_size(),
@@ -83,84 +156,96 @@ void GpuVideoDecodeAccelerator::PictureReady(
 }
 
 void GpuVideoDecodeAccelerator::NotifyEndOfStream() {
-  Send(new AcceleratedVideoDecoderHostMsg_EndOfStream(route_id_));
+  Send(new AcceleratedVideoDecoderHostMsg_EndOfStream(host_route_id_));
 }
 
 void GpuVideoDecodeAccelerator::NotifyError(
     media::VideoDecodeAccelerator::Error error) {
   if (!Send(new AcceleratedVideoDecoderHostMsg_ErrorNotification(
-          route_id_, error))) {
+          host_route_id_, error))) {
     LOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_ErrorNotification) "
                << "failed";
   }
 }
 
 void GpuVideoDecodeAccelerator::OnGetConfigs(
+    const std::pair<int32, int32>& /* tokens */,
     const std::vector<uint32>& requested, std::vector<uint32>* matched) {
+  // TODO(fischman,vrk): this is borked; can't have a VDA before calling
+  // Initialize, but can't call Initialize until we have some configs!
   if (!video_decode_accelerator_.get())
     return;
   video_decode_accelerator_->GetConfigs(requested, matched);
 }
 
 void GpuVideoDecodeAccelerator::OnInitialize(
+    const std::pair<int32, int32>& /* tokens */,
     const std::vector<uint32>& configs) {
-  if (!video_decode_accelerator_.get())
-    return;
-
+  DCHECK(!video_decode_accelerator_.get());
+  GpuVideoService::GetInstance()->InitializeVideoDecoder(decoder_route_id_);

[scherkus (not reviewing), 2011/06/27 22:54:47]

this call ends up setting video_decode_accelerator_ ?!

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

Yes; see the "wonky" comment below.

+  DCHECK(video_decode_accelerator_.get());
   video_decode_accelerator_->Initialize(configs);
 }
 
-void GpuVideoDecodeAccelerator::OnDecode(int32 id,
-                                         base::SharedMemoryHandle handle,
-                                         int32 size) {
-  if (!video_decode_accelerator_.get())
-    return;
+void GpuVideoDecodeAccelerator::OnDecode(
+    const std::pair<int32, int32>&, /* tokens */
+    base::SharedMemoryHandle handle, int32 id, int32 size) {
+  DCHECK(video_decode_accelerator_.get());

[scherkus (not reviewing), 2011/06/27 22:54:47]

nit: some people are in the camp that DCHECK'ing a pointer immediately before
using it is considered pointless/adds noise to code

I like the security blanket that the DCHECK provides, but I think jam/brettw
will disagree w/ me haha

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

I use it for at least 50% documentation/comment value.

   video_decode_accelerator_->Decode(media::BitstreamBuffer(id, handle, size));
 }
 
 void GpuVideoDecodeAccelerator::AssignGLESBuffers(
     const std::vector<media::GLESBuffer>& buffers) {
-  if (!video_decode_accelerator_.get())
-    return;
+  // TODO(fischman,vrk): it's wonky that we handle the AssignTextures message by
+  // handing its contents to GpuVideoService which then turns around and calls
+  // this (public) method.  Instead we should make GpuVideoService vend the
+  // translation method we need and use it directly.
+  DCHECK(video_decode_accelerator_.get());
   video_decode_accelerator_->AssignGLESBuffers(buffers);
 }
 
-void GpuVideoDecodeAccelerator::OnAssignSysmemBuffers(
+void GpuVideoDecodeAccelerator::OnAssignTextures(
+    const std::pair<int32, int32>& /* tokens */,
     const std::vector<int32>& buffer_ids,
-    const std::vector<base::SharedMemoryHandle>& data,
+    const std::vector<uint32>& texture_ids,
     const std::vector<gfx::Size>& sizes) {
+  GpuVideoService* service = GpuVideoService::GetInstance();
+  service->AssignTexturesToDecoder(
+      decoder_route_id_, buffer_ids, texture_ids, sizes);
+}
+
+void GpuVideoDecodeAccelerator::OnAssignSysmemBuffers(
+    const std::pair<int32, int32>& /* tokens */,
+    const std::vector<int32> buffer_ids,
+    const std::vector<base::SharedMemoryHandle> data,
+    const std::vector<gfx::Size> sizes) {
   // TODO(vrk): Implement.
   NOTIMPLEMENTED();
 }
 
-void GpuVideoDecodeAccelerator::OnReusePictureBuffer(int32 picture_buffer_id) {
-  if (!video_decode_accelerator_.get())
-    return;
-
+void GpuVideoDecodeAccelerator::OnReusePictureBuffer(
+    const std::pair<int32, int32>& /* tokens */,
+    int32 picture_buffer_id) {
+  DCHECK(video_decode_accelerator_.get());
   video_decode_accelerator_->ReusePictureBuffer(picture_buffer_id);
 }
 
-void GpuVideoDecodeAccelerator::OnFlush() {
-  if (!video_decode_accelerator_.get())
-    return;
-
-  if (!video_decode_accelerator_->Flush()) {
-    NotifyError(
-        media::VideoDecodeAccelerator::VIDEODECODERERROR_UNEXPECTED_FLUSH);
-  }
+void GpuVideoDecodeAccelerator::OnFlush(
+    const std::pair<int32, int32>& /* tokens */) {
+  DCHECK(video_decode_accelerator_.get());
+  video_decode_accelerator_->Flush();
 }
 
-void GpuVideoDecodeAccelerator::OnAbort() {
-  if (!video_decode_accelerator_.get())
-    return;
-
+void GpuVideoDecodeAccelerator::OnAbort(
+    const std::pair<int32, int32>& /* tokens */) {
+  DCHECK(video_decode_accelerator_.get());
   video_decode_accelerator_->Abort();
 }
 
 void GpuVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer(
     int32 bitstream_buffer_id) {
   if (!Send(new AcceleratedVideoDecoderHostMsg_BitstreamBufferProcessed(
-          route_id_, bitstream_buffer_id))) {
+          host_route_id_, bitstream_buffer_id))) {
     DLOG(ERROR)
         << "Send(AcceleratedVideoDecoderHostMsg_BitstreamBufferProcessed) "
         << "failed";
@@ -168,17 +253,17 @@ void GpuVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer(
 }
 
 void GpuVideoDecodeAccelerator::NotifyInitializeDone() {
-  if (!Send(new AcceleratedVideoDecoderHostMsg_InitializeDone(route_id_)))
+  if (!Send(new AcceleratedVideoDecoderHostMsg_InitializeDone(host_route_id_)))
     LOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_InitializeDone) failed";
 }
 
 void GpuVideoDecodeAccelerator::NotifyFlushDone() {
-  if (!Send(new AcceleratedVideoDecoderHostMsg_FlushDone(route_id_)))
+  if (!Send(new AcceleratedVideoDecoderHostMsg_FlushDone(host_route_id_)))
     LOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_FlushDone) failed";
 }
 
 void GpuVideoDecodeAccelerator::NotifyAbortDone() {
-  if (!Send(new AcceleratedVideoDecoderHostMsg_AbortDone(route_id_)))
+  if (!Send(new AcceleratedVideoDecoderHostMsg_AbortDone(host_route_id_)))
     LOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_AbortDone) failed";
 }