MJPEG acceleration for video capture using VAAPI

content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.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_decode_accelerator_adapter.h"
+
+#include "base/bind.h"
+#include "base/logging.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 GpuJpegDecodeAcceleratorAdapter::Supported() {
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
+  return true;
+#endif
+  return false;
+}
+
+GpuJpegDecodeAcceleratorAdapter::GpuJpegDecodeAcceleratorAdapter(
+    media::VideoCaptureDevice::Client* device_client)
+    : device_client_(device_client),
+      device_thread_(base::MessageLoopProxy::current()),
+      fail_state_(NOT_FAIL),
+      next_bitstream_buffer_id_(0),
+      in_buffer_(media::JpegDecodeAccelerator::kInvalidBitstreamBufferId,
+                 base::SharedMemory::NULLHandle(),
+                 0) {
+  // Device thread may be blocked waiting next frame, so it should not be used
+  // to receive decoding response (IO thread).
+  DCHECK_NE(device_thread_,
+            BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));

[mcasas, 2015/04/16 23:55:13]

Please rephrase comment, I find it hard to read, and change DCHECK to 
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));

[kcwu, 2015/04/20 17:47:59]

I thought again. Since IO thread is not possible blocking wait for capturing,
the device thread is obviously not IO thread. No need to check (at least not
here).

So I removed this check.

+}
+
+GpuJpegDecodeAcceleratorAdapter::~GpuJpegDecodeAcceleratorAdapter() {
+  DCHECK(device_thread_->BelongsToCurrentThread());
+}
+
+bool GpuJpegDecodeAcceleratorAdapter::IsDecoding() {
+  base::AutoLock lock(lock_);
+  return out_frame_ != nullptr;
+}
+
+bool GpuJpegDecodeAcceleratorAdapter::IsExpectedDecodeResponse(
+    int32 bitstream_buffer_id) {
+  if (bitstream_buffer_id !=
+      media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) {

[mcasas, 2015/04/16 23:55:13]

Prefer early termination; by the way shouldn't we return |false| if 
|bitstream_buffer_id| is kInvalidBitstreamBufferId ? Like:

if (bitstream_buffer_id == 
    media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) 
  return false;
LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding";
if (!IsDecoding())
  return false;
...

[kcwu, 2015/04/20 17:48:00]

kInvalidBitstreamBufferId is possible value for NotifyError. For example, GPU
process crashed and the IPC channel disconnected.

[mcasas, 2015/04/22 01:23:52]

The comment says 
"Check |bitstream_buffer_id| from GPU process is expected."
so I'm not understanding how 
|media::JpegDecodeAccelerator::kInvalidBitstreamBufferId| is
a valid BufferId. Perhaps this method needs revisiting or renaming.
Perhaps NotifyError() is OK with |kInvalidBitstreamBufferId| but
what about VideoFrameReady()?

In any case, still consider unwarping the blocks:

if (bitstream_buffer_id == 
    media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) 
  return true;
LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding";
LOG_IF(ERROR, bitstream_buffer_id != in_buffer_.id() << ...;
return IsDecoding() && bitstream_buffer_id == in_buffer_.id()

[kcwu, 2015/04/22 14:43:33]

Done.
Now I handle kInvalidBitstreamBufferId in NotifyError and return false here.
I still don't use LOG_IF because I don't want to repeat these condition code
twice.

+    if (!IsDecoding()) {
+      LOG(ERROR) << "Got decode response while not decoding";
+      return false;
+    }
+    if (bitstream_buffer_id != in_buffer_.id()) {
+      LOG(ERROR) << "Unexpected bitstream_buffer_id " << bitstream_buffer_id
+                 << ", expected " << in_buffer_.id();
+      return false;
+    }
+  }
+  return true;
+}
+
+void GpuJpegDecodeAcceleratorAdapter::DecodeDone() {
+  base::AutoLock lock(lock_);
+  memset(&captured_data_, 0, sizeof(captured_data_));
+  out_buffer_ = nullptr;
+  out_frame_ = nullptr;
+}
+
+bool GpuJpegDecodeAcceleratorAdapter::Initialize() {
+  DCHECK(device_thread_->BelongsToCurrentThread());
+  DVLOG(3) << __func__;

[mcasas, 2015/04/16 23:55:13]

pro tip: In Linux you can use __PRETTY_FUNCTION__ ISO __func__ or __FUNCTION__.

[kcwu, 2015/04/20 17:47:59]

Acknowledged.

+  GpuChannelHost* const host =
+      BrowserGpuChannelHostFactory::instance()->GetGpuChannel();
+  decoder_ = host->CreateJpegDecoder(
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
+  if (!decoder_->Initialize(this)) {
+    decoder_.reset();
+    fail_state_ = FAILED;
+    return false;
+  }
+  return true;
+}
+
+bool GpuJpegDecodeAcceleratorAdapter::IsFailed() {
+  base::AutoLock lock(lock_);
+  return fail_state_ == FAILED;
+}
+
+void GpuJpegDecodeAcceleratorAdapter::VideoFrameReady(
+    int32_t bitstream_buffer_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DVLOG(3) << __func__;
+
+  // Verify parameter from GPU process.
+  if (!IsExpectedDecodeResponse(bitstream_buffer_id))
+    return;
+
+  device_client_->OnIncomingCapturedVideoFrame(out_buffer_, out_frame_,
+                                               captured_data_.timestamp);

[mcasas, 2015/04/16 23:55:13]

If you discard |captured_data_|, you can use |out_frame_.timestamp()|

[kcwu, 2015/04/20 17:48:00]

out_frame_.timestamp() was zero. Can I merge captured timestamp into out_frame_?
I guess there was a reason that they are separated.

[mcasas, 2015/04/28 00:04:49]

IIRC, VideoFrame is created in VideoCaptureDeviceClient but
the timestamp was not plugged into it, it was sent via IPC
instead, for no particular reason IMHO. (I'm ad libbing a bit).

[kcwu, 2015/05/08 14:42:40]

Acknowledged.

+
+  DecodeDone();
+}
+
+void GpuJpegDecodeAcceleratorAdapter::FallbackToSoftwareDecode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
+         device_thread_->BelongsToCurrentThread());
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO))
+    DCHECK(IsDecoding());
+  else
+    DCHECK(!IsDecoding());
+  DVLOG(1) << "Fallback to software decode";
+
+  CapturedData captured_data;
+  {
+    base::AutoLock lock(lock_);
+    // See comment below.
+    if (BrowserThread::CurrentlyOn(BrowserThread::IO))
+      fail_state_ = FAILING;
+    else
+      fail_state_ = FAILED;
+    captured_data = captured_data_;
+  }
+
+  // If we are on device thread, OnIncomingCapturedData is reentrant. But it's
+  // okay since |fail_state_| is FAILED and it won't go hardware decoding route
+  // this time.
+  // If we are on IO thread and next frame is coming on device thread, the next
+  // frame will call DecodeCapturedData() and drop immediately due to
+  // |fail_state_| is FAILING. This is to avoid next frame decoded before this
+  // frame.
+  device_client_->OnIncomingCapturedData(captured_data.data,
+                                         captured_data.length,
+                                         captured_data.frame_format,
+                                         captured_data.rotation,
+                                         captured_data.timestamp);
+  {
+    base::AutoLock lock(lock_);
+    fail_state_ = FAILED;
+  }
+}
+
+void GpuJpegDecodeAcceleratorAdapter::NotifyError(
+    int32_t bitstream_buffer_id,
+    media::JpegDecodeAccelerator::Error error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DVLOG(3) << __func__;
+
+  // Verify parameter from GPU process.
+  if (!IsExpectedDecodeResponse(bitstream_buffer_id))
+    return;
+
+  // Error not associated to any bitstream buffer. For example, GPU channel
+  // is broken.
+  if (bitstream_buffer_id ==
+      media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) {

[mcasas, 2015/04/16 23:55:13]

This condition is already covered inside IsExpectedDecodeResponse()
I believe.

[kcwu, 2015/04/20 17:48:00]

IsExpectedDecodeResponse returns true for this value and we have to do something
here.

+    {
+      base::AutoLock lock(lock_);
+      fail_state_ = FAILED;
+    }
+    DecodeDone();
+    return;
+  }
+
+  FallbackToSoftwareDecode();
+  DecodeDone();
+}
+
+void GpuJpegDecodeAcceleratorAdapter::DecodeCapturedData(
+    const uint8* data,
+    int length,
+    const media::VideoCaptureFormat& frame_format,
+    int rotation,
+    const base::TimeTicks& timestamp) {
+  DCHECK(device_thread_->BelongsToCurrentThread());
+  DCHECK(decoder_);
+  if (IsDecoding()) {
+    DVLOG(1) << "Drop captured frame. Previous jpeg frame is still decoding";
+    return;
+  }
+  DCHECK(!IsFailed());
+
+  gfx::Size dimensions = frame_format.frame_size;

[mcasas, 2015/04/16 23:55:13]

const?

[kcwu, 2015/04/20 17:48:00]

Done.

+  scoped_refptr<media::VideoCaptureDevice::Client::Buffer> out_buffer =

[mcasas, 2015/04/16 23:55:13]

Suggest to directly allocate onto |out_buffer_| and in l.232

[kcwu, 2015/04/20 17:47:59]

@wuchengli prefer to shorten lock time at the end of method. So allocate on
stack first.

+      device_client_->ReserveOutputBuffer(media::PIXEL_FORMAT_I420, dimensions);
+  if (!out_buffer.get()) {
+    DVLOG(1) << "Drop captured frame. No available buffers"
+             << " (all buffers are still queued to display)";
+    return;
+  }
+
+  CapturedData captured_data;
+  captured_data.data = const_cast<uint8*>(data);
+  captured_data.length = length;
+  captured_data.frame_format = frame_format;
+  captured_data.rotation = rotation;
+  captured_data.timestamp = timestamp;
+
+  size_t in_buffer_size = captured_data.length;

[mcasas, 2015/04/16 23:55:13]

Why an extra alias? Just rename the input param.

[kcwu, 2015/04/20 17:48:00]

I just want to keep the name of input param matching OnIncomingCapturedData. Do
you still prefer rename it?

+
+  scoped_ptr<base::SharedMemory> in_shared_memory;
+  {
+    // Swap out to local variable in order to reduce lock time in IO thread.

[mcasas, 2015/04/16 23:55:13]

s/in/on/

[kcwu, 2015/04/20 17:48:00]

Done.

+    base::AutoLock lock(lock_);
+    std::swap(in_shared_memory, in_shared_memory_);
+  }
+  // Enlarge input buffer if necessary.
+  if (!in_shared_memory.get() ||
+      in_buffer_size > in_shared_memory->mapped_size()) {

[mcasas, 2015/04/16 23:55:14]

I suspect this allocate-as-you-go scheme might end up in a series of
initial allocations until we settle in a large enough size to hold
incoming frames. 
Alternatively, consider pre allocating enough size to hold the 
largest possible JPEG frame, which should be the size of
an uncompressed I420 frame, right?

Then you should be able to recycle it instead of swapping
it in and out (which might degrade performance as we
discovered elsewhere).

[kcwu, 2015/04/20 17:48:00]

However JPEG does not guarantee compressed image is smaller than uncompressed
image. And that wastes memory (virtually?).

[kcwu, 2015/05/08 14:42:40]

Now I allocate 2x input size at first time.

+    in_shared_memory.reset(new base::SharedMemory);
+    if (!in_shared_memory->CreateAnonymous(in_buffer_size)) {
+      DLOG(ERROR) << "CreateAnonymous failed";
+      captured_data_ = captured_data;
+      FallbackToSoftwareDecode();
+      return;
+    }
+
+    if (!in_shared_memory->Map(in_buffer_size)) {

[mcasas, 2015/04/16 23:55:14]

Merge this with the previous error condition in l.213.

[kcwu, 2015/04/20 17:48:00]

Done.

+      DLOG(ERROR) << "Map failed";
+      captured_data_ = captured_data;
+      FallbackToSoftwareDecode();
+      return;
+    }
+  }
+
+  media::BitstreamBuffer in_buffer(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;
+
+  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(),
+          out_buffer->handle(),
+          0,
+          base::TimeDelta(),
+          base::Closure());
+  DCHECK(out_frame.get());
+
+  out_frame->metadata()->SetDouble(media::VideoFrameMetadata::FRAME_RATE,
+                                   captured_data.frame_format.frame_rate);
+
+  memcpy(in_shared_memory->memory(), captured_data.data, in_buffer_size);
+  captured_data.data = reinterpret_cast<uint8*>(in_shared_memory->memory());
+
+  base::AutoLock lock(lock_);
+  captured_data_ = captured_data;
+  in_buffer_ = in_buffer;
+  std::swap(in_shared_memory_, in_shared_memory);
+  out_buffer_ = out_buffer;
+  out_frame_ = out_frame;
+
+  decoder_->Decode(in_buffer_, out_frame_);

[mcasas, 2015/04/16 23:55:13]

IIUC, only |in_buffer_->id()| is really needed. Please consider only
keeping id as a member variable for IsExpectedDecodeResponse().

[kcwu, 2015/04/20 17:48:00]

in_* are for fallback.
out_* are keep because we want to make VideoFrameReady (in IO thread) lighter
weight.

+}
+
+}  // namespace content