MJPEG acceleration for video capture, browser part

content/browser/renderer_host/gpu_jpeg_decoder.cc

+// 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/browser/renderer_host/gpu_jpeg_decoder.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "base/trace_event/trace_event.h"
+#include "content/browser/gpu/browser_gpu_channel_host_factory.h"
+#include "content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h"
+#include "content/public/browser/browser_thread.h"
+#include "media/base/video_frame.h"
+
+namespace content {
+
+// static
+bool GpuJpegDecoder::Supported() {
+// A lightweight check for caller to avoid IPC latency for known unsupported
+// platform. Initialize() can do the real platform supporting check but it
+// requires an IPC even for platforms that do not support HW decoder.
+// TODO(kcwu): move this information to GpuInfo.
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
+  return true;
+#endif
+  return false;
+}
+
+GpuJpegDecoder::GpuJpegDecoder(const DecodeDoneCB& decode_done_cb,
+                               const ErrorCB& error_cb)
+    : jpeg_thread_("GpuJpegDecoderThread"),

[piman, 2015/05/18 23:18:30]

Why do we need this thread? It seems all you're doing on the thread is mapping
buffers, copying memory and sending IPCs, but even mapping buffers / copying
memory you do synchronously.

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

The main point to have another thread is the capture thread often blocks waiting
for camera (in ioctl) and IO thread should only do fast things.
My earlier CL was using only capture thread and IO thread, and became
complicated.

+      create_event_(false, false),
+      decode_event_(false, false),
+      decode_done_cb_(decode_done_cb),
+      error_cb_(error_cb),
+      next_bitstream_buffer_id_(0),
+      in_buffer_(media::JpegDecodeAccelerator::kInvalidBitstreamBufferId,
+                 base::SharedMemory::NULLHandle(),
+                 0) {
+}
+
+GpuJpegDecoder::~GpuJpegDecoder() {
+  DCHECK(CalledOnValidThread());
+}
+
+bool GpuJpegDecoder::IsDecoding() {
+  DCHECK(CalledOnValidThread());
+  return !decode_done_closure_.is_null();
+}
+
+bool GpuJpegDecoder::Initialize() {
+  DVLOG(3) << __func__;
+  DCHECK(CalledOnValidThread());
+  DCHECK(!jpeg_thread_.IsRunning());
+
+  if (!jpeg_thread_.Start()) {
+    LOG(ERROR) << "Failed to start decoding thread.";
+    return false;
+  }
+  jpeg_task_runner_ = jpeg_thread_.task_runner();
+
+  jpeg_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&GpuJpegDecoder::CreateDecoderOnJpegThread,
+                 base::Unretained(this)));
+  create_event_.Wait();
+
+  if (!decoder_) {
+    jpeg_thread_.Stop();
+    return false;
+  }
+
+  return true;
+}
+
+void GpuJpegDecoder::CreateDecoderOnJpegThread() {
+  DCHECK(jpeg_task_runner_->BelongsToCurrentThread());
+  GpuChannelHost* const host =
+      BrowserGpuChannelHostFactory::instance()->GetGpuChannel();

[piman, 2015/05/18 23:18:30]

This is problematic for 2 reasons:
- BrowserGpuChannelHostFactory::instance() should not be called from a thread
- GetGpuChannel doesn't create a GPU channel (only returns the existing one if
any), and may return a lost channel. You need to use EstablishGpuChannel or
EstablishGpuChannelSync - it has to be called from the UI thread.

[kcwu, 2015/06/02 15:09:55]

Done.

+  decoder_ = host->CreateJpegDecoder();

[piman, 2015/05/18 23:18:30]

You're creating decoder_ on the jpeg_thread_ but destroy it on the embedder
thread (where GpuJpegDecoder::~GpuJpegDecoder is called), but
GpuJpegDecodeAcceleratorHost is marked as not thread safe.

decoder_ is actually destroyed before jpeg_thread_ is destroyed (and joined), so
you definitely have a race, where e.g. the bottom half of
DecodeCapturedDataOnJpegThread could run concurrently with the destruction.

[kcwu, 2015/06/03 14:30:07]

Done.
I found ~VideoCaptureDeviceClient is always called on device thread. So there is
no problem.
I make it explicit now.

+  if (!decoder_->Initialize(this)) {
+    decoder_.reset();
+  }
+  create_event_.Signal();
+}
+
+void GpuJpegDecoder::VideoFrameReady(int32_t bitstream_buffer_id) {
+  DVLOG(3) << __func__;
+  DCHECK(jpeg_task_runner_->BelongsToCurrentThread());
+  TRACE_EVENT0("jpeg", "GpuJpegDecoder::VideoFrameReady");
+
+  if (bitstream_buffer_id ==
+      media::JpegDecodeAccelerator::kInvalidBitstreamBufferId)
+    return;
+
+  if (!IsDecoding()) {
+    LOG(ERROR) << "Got decode response while not decoding";
+    return;
+  }
+
+  if (bitstream_buffer_id != in_buffer_.id()) {
+    LOG(ERROR) << "Unexpected bitstream_buffer_id " << bitstream_buffer_id
+               << ", expected " << in_buffer_.id();
+    return;
+  }
+
+  decode_done_closure_.Run();
+  decode_done_closure_.Reset();
+
+  TRACE_EVENT_ASYNC_END0("jpeg", "GpuJpegDecoder decoding",
+                         bitstream_buffer_id);
+}
+
+void GpuJpegDecoder::NotifyError(int32_t bitstream_buffer_id,
+                                 media::JpegDecodeAccelerator::Error error) {
+  DVLOG(3) << __func__;
+  DCHECK(jpeg_task_runner_->BelongsToCurrentThread());
+  LOG(ERROR) << "Decode error, bitstream_buffer_id=" << bitstream_buffer_id;
+
+  decode_done_closure_.Reset();
+
+  error_cb_.Run(
+      base::StringPrintf("Decode error, bitstream_buffer_id=%d, error=%d",
+                         bitstream_buffer_id, error));
+}
+
+void GpuJpegDecoder::DecodeCapturedData(
+    const uint8* data,
+    size_t in_buffer_size,
+    const media::VideoCaptureFormat& frame_format,
+    const base::TimeTicks& timestamp,
+    scoped_ptr<media::VideoCaptureDevice::Client::Buffer> out_buffer) {
+  DCHECK(CalledOnValidThread());
+
+  jpeg_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &GpuJpegDecoder::DecodeCapturedDataOnJpegThread,
+          base::Unretained(this),
+          data,
+          in_buffer_size,
+          frame_format,
+          timestamp,
+          base::Passed(&out_buffer)));
+
+  // Wait until |data| is copied out.
+  decode_event_.Wait();
+}
+
+
+void GpuJpegDecoder::DecodeCapturedDataOnJpegThread(
+    const uint8* data,
+    size_t in_buffer_size,
+    const media::VideoCaptureFormat& frame_format,
+    const base::TimeTicks& timestamp,
+    scoped_ptr<media::VideoCaptureDevice::Client::Buffer> out_buffer) {
+  DCHECK(jpeg_task_runner_->BelongsToCurrentThread());
+  TRACE_EVENT_ASYNC_BEGIN0("jpeg", "GpuJpegDecoder decoding",
+                           next_bitstream_buffer_id_);
+  TRACE_EVENT0("jpeg", "GpuJpegDecoder::DecodeCapturedDataOnJpegThread");
+
+  // TODO(kcwu): enqueue decode requests in case decoding is not fast enough
+  // (say, if decoding time is longer than 16ms for 60fps 4k image)
+  if (IsDecoding()) {
+    DVLOG(1) << "Drop captured frame. Previous jpeg frame is still decoding";
+    decode_event_.Signal();
+    return;
+  }
+
+  // Enlarge input buffer if necessary.
+  if (!in_shared_memory_.get() ||
+      in_buffer_size > in_shared_memory_->mapped_size()) {

[piman, 2015/05/18 23:18:30]

If you reuse the shared memory across decodes, it would be useful to do that on
the GPU process side to. Mapping the buffer has a cost, which sounds like it
could be avoided.

[kcwu, 2015/06/02 15:09:55]

I feel mapping is acceptable since VDA is doing so. 
This needs to change the interface a lot, so I prefer to revisit this issue in
another CL.

+    // Reserve 2x space to avoid frequently reallocate for initial frames.
+    size_t reserved_size = 2 * in_buffer_size;
+    in_shared_memory_.reset(new base::SharedMemory);
+    if (!in_shared_memory_->CreateAndMapAnonymous(reserved_size)) {
+      error_cb_.Run(base::StringPrintf("CreateAndMapAnonymous failed, size=%ld",
+                                       reserved_size));
+      decode_event_.Signal();
+      return;
+    }
+  }
+  memcpy(in_shared_memory_->memory(), data, in_buffer_size);
+  decode_event_.Signal();
+
+  in_buffer_ = media::BitstreamBuffer(
+      next_bitstream_buffer_id_, in_shared_memory_->handle(), in_buffer_size);
+  // Mask against 30 bits, to avoid (undefined) wraparound on signed integer.
+  next_bitstream_buffer_id_ = (next_bitstream_buffer_id_ + 1) & 0x3FFFFFFF;
+
+  const gfx::Size dimensions = frame_format.frame_size;
+  scoped_refptr<media::VideoFrame> out_frame =
+      media::VideoFrame::WrapExternalPackedMemory(
+          media::VideoFrame::I420,
+          dimensions,
+          gfx::Rect(dimensions),
+          dimensions,
+          reinterpret_cast<uint8*>(out_buffer->data()),
+          out_buffer->size(),
+          base::SharedMemoryHandle(out_buffer->AsPlatformHandle(), true),
+          0,
+          base::TimeDelta(),
+          base::Closure());
+  DCHECK(out_frame.get());
+  out_frame->metadata()->SetDouble(media::VideoFrameMetadata::FRAME_RATE,
+                                   frame_format.frame_rate);
+
+  decode_done_closure_ = base::Bind(decode_done_cb_, base::Passed(&out_buffer),
+                                    out_frame, timestamp);
+  decoder_->Decode(in_buffer_, out_frame);
+}
+
+}  // namespace content