MJPEG acceleration for video capture using VAAPI, the GPU and IPC part

content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc

Index: content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc
diff --git a/content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc b/content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1fbfe11845590150660eb9a26f458790ded1881a
--- /dev/null
+++ b/content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc
@@ -0,0 +1,156 @@
+// Copyright 2015 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 "content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "content/common/gpu/client/gpu_channel_host.h"
+#include "content/common/gpu/gpu_messages.h"
+#include "ipc/ipc_message_macros.h"
+#include "ipc/ipc_message_utils.h"
+
+using media::JpegDecodeAccelerator;
+namespace content {
+
+GpuJpegDecodeAcceleratorHost::GpuJpegDecodeAcceleratorHost(
+    GpuChannelHost* channel,
+    int32 route_id)
+    : channel_(channel), client_(NULL), decoder_route_id_(route_id) {
+  DCHECK(channel_);
+}
+
+GpuJpegDecodeAcceleratorHost::~GpuJpegDecodeAcceleratorHost() {
+}
+
+bool GpuJpegDecodeAcceleratorHost::OnMessageReceived(const IPC::Message& msg) {
+  DCHECK(CalledOnValidThread());
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(GpuJpegDecodeAcceleratorHost, msg)
+    IPC_MESSAGE_HANDLER(AcceleratedJpegDecoderHostMsg_VideoFrameReady,
+                        OnVideoFrameReady)
+    IPC_MESSAGE_HANDLER(AcceleratedJpegDecoderHostMsg_NotifyError,
+                        OnNotifyError)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  DCHECK(handled);
+  // See OnNotifyError for why |this| mustn't be used after OnNotifyError might
+  // have been called above.
+  return handled;
+}
+
+void GpuJpegDecodeAcceleratorHost::OnChannelError() {
+  DVLOG(3) << __func__;
+  DCHECK(CalledOnValidThread());
+
+  channel_ = NULL;

[piman, 2015/05/18 22:46:18]

nit: nullptr

[kcwu, 2015/05/22 19:46:50]

Done.

+  OnNotifyError(kInvalidBitstreamBufferId, PLATFORM_FAILURE);
+}
+
+bool GpuJpegDecodeAcceleratorHost::Initialize(
+    media::JpegDecodeAccelerator::Client* client) {
+  DCHECK(CalledOnValidThread());
+
+  bool succeeded = false;
+  // This cannot be on IO thread because the msg is synchronous.
+  Send(new GpuMsg_CreateJpegDecoder(decoder_route_id_, &succeeded));
+
+  if (!succeeded) {
+    DLOG(ERROR) << "Send(GpuMsg_CreateJpegDecoder()) failed";
+    channel_->RemoveRoute(decoder_route_id_);
+    return false;

[piman, 2015/05/18 22:46:18]

So, if I understand correctly, the client is responsible for checking for
failure, and in that case call delete instead of Destroy. It seems error-prone
(and is not documented).

Could we instead make GpuChannelHost::CreateJpegDecoder also call Initialize,
and be responsible for destroying GpuJpegDecodeAcceleratorHost in case of
failure? That way also you wouldn't need this to reach into the channel_ to call
RemoveRoute (which is inconsistent with where AddRoute is called) - that could
be done inside GpuChannelHost::CreateJpegDecoder.

[kcwu, 2015/05/22 19:46:50]

Done.

+  }
+  client_ = client;
+
+  return true;
+}
+
+void GpuJpegDecodeAcceleratorHost::Decode(
+    const media::BitstreamBuffer& bitstream_buffer,
+    const scoped_refptr<media::VideoFrame>& video_frame) {
+  DCHECK(CalledOnValidThread());
+  if (!channel_)
+    return;
+
+  base::SharedMemoryHandle input_handle =
+      channel_->ShareToGpuProcess(bitstream_buffer.handle());
+  if (!base::SharedMemory::IsHandleValid(input_handle)) {
+    LOG(ERROR) << "Failed to duplicate buffer handle of BitstreamBuffer";
+    OnNotifyError(bitstream_buffer.id(), INVALID_ARGUMENT);
+    return;
+  }
+
+  if (!base::SharedMemory::IsHandleValid(video_frame->shared_memory_handle())) {
+    LOG(ERROR)
+        << "Decode(): cannot output to frame not backed by shared memory";
+    OnNotifyError(bitstream_buffer.id(), INVALID_ARGUMENT);

[piman, 2015/05/18 22:46:18]

So, IIUC, on Windows, ShareToGpuProcess actually creates a handle in the GPU
process. If we early return here, we would leak it.

So, could we either:
- check handle validity ahead of time before duplicating any handle or
- simply DCHECK that they're valid? Is there a use case for passing an
invalid/NULL handle here?

[kcwu, 2015/05/22 19:46:50]

Done.

+    return;
+  }
+
+  base::SharedMemoryHandle output_handle =
+      channel_->ShareToGpuProcess(video_frame->shared_memory_handle());
+  if (!base::SharedMemory::IsHandleValid(output_handle)) {
+    LOG(ERROR) << "Decode(): failed to duplicate buffer handle of VideoFrame";
+    OnNotifyError(bitstream_buffer.id(), PLATFORM_FAILURE);
+    return;
+  }
+
+  size_t output_buffer_size = media::VideoFrame::AllocationSize(
+      video_frame->format(), video_frame->coded_size());
+
+  AcceleratedJpegDecoderMsg_Decode_Params decode_params;
+  decode_params.coded_size = video_frame->coded_size();
+  decode_params.input_buffer_id = bitstream_buffer.id();
+  decode_params.input_buffer_handle = input_handle;
+  decode_params.input_buffer_size = bitstream_buffer.size();
+  decode_params.output_video_frame_handle = output_handle;
+  decode_params.output_buffer_size = output_buffer_size;
+  Send(new AcceleratedJpegDecoderMsg_Decode(decoder_route_id_, decode_params));
+}
+
+void GpuJpegDecodeAcceleratorHost::Destroy() {
+  DCHECK(CalledOnValidThread());
+  Send(new AcceleratedJpegDecoderMsg_Destroy(decoder_route_id_));
+
+  if (channel_) {
+    if (decoder_route_id_ != MSG_ROUTING_NONE)
+      channel_->RemoveRoute(decoder_route_id_);
+    channel_ = NULL;
+  }
+
+  delete this;

[piman, 2015/05/18 22:46:18]

I'm not in love with this pattern, because it prevents users from using
scoped_ptr - since they have to call Destroy instead of delete.

Could we instead move all this logic into the destructor?

[kcwu, 2015/05/22 19:46:50]

This class implements media::JpegDecodeAccelerator interface, which modeled
after media::VideoDecodeAccelerator. 
I would prefer that JpegDecodeAccelerator and VideoDecodeAccelerator are
consistent in this case.

[piman, 2015/05/22 22:27:48]

Honestly, I don't think VideoDecodeAccelerator is a good pattern to follow. It's
confusing, it prevents the use of standard helper classes, including STL ones
once we have c++11. It's not consistent with the rest of Chrome. Makes
everything more complicated.

[kcwu, 2015/05/25 18:57:16]

Done.
Destroy methods are merged into destructors.

+}
+
+void GpuJpegDecodeAcceleratorHost::Send(IPC::Message* message) {
+  DVLOG(3) << __func__;
+  DCHECK(CalledOnValidThread());
+
+  if (!channel_)
+    return;
+  if (!channel_->Send(message)) {
+    DLOG(ERROR) << "Send(" << message->type() << ") failed";
+  }
+}
+
+void GpuJpegDecodeAcceleratorHost::OnVideoFrameReady(
+    int32_t bitstream_buffer_id) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(client_);
+  client_->VideoFrameReady(bitstream_buffer_id);
+}
+
+void GpuJpegDecodeAcceleratorHost::OnNotifyError(int32_t bitstream_buffer_id,
+                                                 Error error) {
+  DVLOG(2) << __func__ << ": error=" << error
+           << ", bitstream_buffer_id=" << bitstream_buffer_id;
+  DCHECK(CalledOnValidThread());
+  if (!client_)
+    return;
+
+  client_->NotifyError(bitstream_buffer_id,
+                       static_cast<media::JpegDecodeAccelerator::Error>(error));
+  client_ = nullptr;
+}
+
+}  // namespace content