MJPEG acceleration for video capture using VAAPI

content/browser/renderer_host/gpu_jpeg_decoder.cc

Index: content/browser/renderer_host/gpu_jpeg_decoder.cc
diff --git a/content/browser/renderer_host/gpu_jpeg_decoder.cc b/content/browser/renderer_host/gpu_jpeg_decoder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2395e364d4e6689abc708a3e740e7bba14fab6f4
--- /dev/null
+++ b/content/browser/renderer_host/gpu_jpeg_decoder.cc
@@ -0,0 +1,265 @@
+// 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 "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.
+  // TODO(kcwu): move this logic to GpuInfo.
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
+  return true;
+#endif
+  return false;
+}
+
+GpuJpegDecoder::GpuJpegDecoder(
+    media::VideoCaptureDevice::Client* device_client)
+    : device_client_(device_client),
+      main_task_runner_(base::MessageLoopProxy::current()),
+      fail_state_(NOT_FAIL),
+      next_bitstream_buffer_id_(0),
+      in_buffer_(media::JpegDecodeAccelerator::kInvalidBitstreamBufferId,
+                 base::SharedMemory::NULLHandle(),
+                 0) {
+}
+
+GpuJpegDecoder::~GpuJpegDecoder() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+}
+
+bool GpuJpegDecoder::IsDecoding() {
+  base::AutoLock lock(lock_);
+  return out_frame_ != nullptr;
+}
+
+bool GpuJpegDecoder::IsExpectedDecodeResponse(
+    int32 bitstream_buffer_id) {
+  if (bitstream_buffer_id ==
+      media::JpegDecodeAccelerator::kInvalidBitstreamBufferId)
+    return false;
+
+  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 GpuJpegDecoder::DecodeDone() {
+  base::AutoLock lock(lock_);
+  memset(&captured_data_, 0, sizeof(captured_data_));

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

Is this needed or is |captured_data_| set before use?
(Assuming FallbackToSoftwareDecode() is removed.)

[kcwu, 2015/04/30 19:25:43]

Done.
Not needed. Removed.

+  out_buffer_ = nullptr;
+  out_frame_ = nullptr;
+}
+
+bool GpuJpegDecoder::Initialize() {
+  DVLOG(3) << __func__;
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  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 GpuJpegDecoder::IsFailed() {
+  base::AutoLock lock(lock_);
+  return fail_state_ == FAILED;
+}
+
+void GpuJpegDecoder::VideoFrameReady(

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

VideoFrameReadyOnIOThread()?

[kcwu, 2015/04/30 19:25:42]

This name is inheritance from media::JpegDecodeAccelerator::Client

+    int32_t bitstream_buffer_id) {
+  DVLOG(3) << __func__;
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (!IsExpectedDecodeResponse(bitstream_buffer_id))
+    return;
+
+  device_client_->OnIncomingCapturedVideoFrame(out_buffer_, out_frame_,

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

DecodeDone() in l.106 is executed in mutual exclusion 
thanks to |lock_|, but here we use |out_buffer_| 
and |out_frame_| out of the |lock_|. Correct?

[kcwu, 2015/04/30 19:25:42]

Done.

+                                               captured_data_.timestamp);
+
+  DecodeDone();
+}
+
+void GpuJpegDecoder::FallbackToSoftwareDecode() {
+  DVLOG(1) << "Fallback to software decode";
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
+         main_task_runner_->BelongsToCurrentThread());
+  DCHECK_EQ(BrowserThread::CurrentlyOn(BrowserThread::IO), IsDecoding());

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

These chained DCHECKs are a symptom of excessive complexity,
I'd say. +1 for removing FallbackToSoftwareDecode(): if
GpuJpegDecoder() has failed, VideoCaptureDeviceClient
should never jump into this class.

[kcwu, 2015/04/30 19:25:42]

Done.

+
+  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 |main_task_runner_| 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 |main_task_runner_|
+  // 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 GpuJpegDecoder::NotifyError(
+    int32_t bitstream_buffer_id,
+    media::JpegDecodeAccelerator::Error error) {
+  DVLOG(3) << __func__;
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Error not associated to any bitstream buffer. For example, GPU channel
+  // is broken.
+  if (bitstream_buffer_id ==
+      media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) {
+    {
+      base::AutoLock lock(lock_);
+      fail_state_ = FAILED;
+    }
+    DecodeDone();
+    return;
+  }
+
+  if (!IsExpectedDecodeResponse(bitstream_buffer_id))
+    return;
+
+  FallbackToSoftwareDecode();
+  DecodeDone();
+}
+
+void GpuJpegDecoder::DecodeCapturedData(
+    const uint8* data,
+    int length,
+    const media::VideoCaptureFormat& frame_format,
+    int rotation,
+    const base::TimeTicks& timestamp) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(decoder_);
+  if (IsDecoding()) {
+    DVLOG(1) << "Drop captured frame. Previous jpeg frame is still decoding";
+    return;
+  }
+  {

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

l.181-185:

if (!IsFailed())
  return;

[kcwu, 2015/04/30 19:25:43]

Done.

+    base::AutoLock lock(lock_);
+    if (fail_state_ != NOT_FAIL)
+      return;
+  }

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

There is quite a lot of sequential locking-unlocking, e.g.
here you call IsDecoding() in l.177 then lock() again in l.182
etc. DecodeDone() and IsFailed() also use |lock_|. Too
many mutual exclusion areas in code is a symptom of
complex signalling between threads: hard to maintain
etc. Maybe you could get the fail/ok state into a local

class ThreadSafeState{
 public:
  enum State {OK, FAIL}
  ThreadSafeState(State state) : state_(state) {}
  
  bool state() const { 
    base::AutoLock lock(lock_);
    return state_;
  }
  void set_state() { 
    base::AutoLock lock(lock_);
    state_ = state;
  }

 private:
  base::AutoLock lock_;
  State state_; 
};

And at least that part of the state tracking would
clutter less the specifics of GpuJpegDecoder -- and
it would use it own |lock_| which IIUC is 
actually independent from the locking to be
done in data specifics.

I'm not super sure about that class anyway, WDYT?
In any case please have another go at the locking-
modifying-unlocking and refactor.

[kcwu, 2015/04/30 19:25:43]

Locking is refactored now.

+
+  // Since IsDecoding() is false here and we only decode a frame at a time,
+  // after this point, the only possible multithread access to this class is
+  // NotifyError. This means |fail_state_| may be set to FAILED when running
+  // following code.
+
+  const gfx::Size dimensions = frame_format.frame_size;
+  scoped_refptr<media::VideoCaptureDevice::Client::Buffer> out_buffer =
+      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;
+
+  scoped_ptr<base::SharedMemory> in_shared_memory;
+  {
+    // Swap out to local variable in order to reduce lock time on IO thread.
+    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()) {
+    in_shared_memory.reset(new base::SharedMemory);
+    if (!in_shared_memory->CreateAndMapAnonymous(in_buffer_size)) {
+      DLOG(ERROR) << "CreateAndMapAnonymous failed, size=" << in_buffer_size;
+      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_);

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

l.253-255:

if(!IsFailed())
  return;

[kcwu, 2015/04/30 19:25:43]

Not sure what do you mean.
Please take a look anyway since the locking is revised.

+  if (fail_state_ != NOT_FAIL)
+    return;
+  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_);
+}
+
+}  // namespace content