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

content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc

Index: content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc
diff --git a/content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc b/content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7faae93df14b2153dd09e6e004cc6b2fc1763e33
--- /dev/null
+++ b/content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc
@@ -0,0 +1,251 @@
+// 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/media/vaapi_jpeg_decode_accelerator.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/trace_event/trace_event.h"
+#include "content/common/gpu/gpu_channel.h"
+#include "content/common/gpu/media/vaapi_picture.h"
+#include "media/base/video_frame.h"
+#include "media/filters/jpeg_parser.h"
+
+static void ReportVaapiError() {
+}
+
+namespace content {
+
+VaapiJpegDecodeAccelerator::DecodeRequest::DecodeRequest(
+    const media::BitstreamBuffer& bitstream_buffer,
+    scoped_refptr<media::VideoFrame> video_frame)
+    : bitstream_buffer(bitstream_buffer), video_frame(video_frame) {
+}
+
+VaapiJpegDecodeAccelerator::DecodeRequest::~DecodeRequest() {
+}
+
+void VaapiJpegDecodeAccelerator::NotifyError(int32_t bitstream_buffer_id,
+                                             Error error) {
+  if (task_runner_ != base::ThreadTaskRunnerHandle::Get()) {

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

I don't like this pattern very much. The caller knows which thread it's on, i.e.
whether it needs to post a task or not.

Instead can you have a NotifyError that runs exclusively on the main thread, and
a helper function e.g. NotifyErrorFromDecoderThread that unconditionally posts a
task?

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

Done.

+    DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+    task_runner_->PostTask(FROM_HERE,
+                            base::Bind(&VaapiJpegDecodeAccelerator::NotifyError,
+                                       weak_this_, bitstream_buffer_id, error));
+    return;
+  }
+
+  Cleanup();

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

Is it needed to do this here? It'll be done at Destroy() time.

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

Done.

+
+  LOG(ERROR) << "Notifying of error " << error;

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

Is this needed in release builds? It seems like every broken jpeg on the web
would trigger this.

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

Done.

+  if (client_) {
+    client_->NotifyError(bitstream_buffer_id, error);
+    client_ptr_factory_.reset();
+  }
+}
+
+VaapiJpegDecodeAccelerator::VaapiJpegDecodeAccelerator(
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
+    : initialized_(false),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      io_task_runner_(io_task_runner),
+      decoder_thread_("VaapiJpegDecoderThread"),

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

Can we share a single thread among all decoders?
Because of the global VAAPI lock, you're probably not getting any scaling across
threads anyway.
Instead you can have a global thread with a refcount. The first
VaapiJpegDecodeAccelerator creates the thread and each further
VaapiJpegDecodeAccelerator would reuse the same thread, keeping a count. When
the last one finishes, it would stop and join the thread.

[kcwu, 2015/05/26 14:51:30]

This only happens if both external and internal cameras are in use, which is
unlikely. And I feel this (passing thread from GpuJpegDecodeAccelerator) exposed
implementation detail of each accelerator to GpuJpegDecodeAccelerator.

Since I cannot think of any reasonable use cases (use both cameras at the same
time), I would prefer not to optimize for them now.

+      va_surface_(VA_INVALID_SURFACE),
+      weak_this_factory_(this) {
+  weak_this_ = weak_this_factory_.GetWeakPtr();
+}
+
+VaapiJpegDecodeAccelerator::~VaapiJpegDecodeAccelerator() {
+  DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get());
+}
+
+bool VaapiJpegDecodeAccelerator::Initialize(Client* client) {
+  DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get());
+
+  client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client));
+  client_ = client_ptr_factory_->GetWeakPtr();

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

Why is a WeakPtr needed here? It seems like the client
(GpuJpegDecodeAccelerator) always outlives |this|, since it owns it.

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

Done.

+
+  base::AutoLock auto_lock(lock_);
+  DCHECK(!initialized_);
+
+  vaapi_wrapper_ =
+      VaapiWrapper::Create(VaapiWrapper::kDecode, VAProfileJPEGBaseline,
+                           base::Bind(&ReportVaapiError));
+
+  if (!vaapi_wrapper_.get()) {
+    DLOG(ERROR) << "Failed initializing VAAPI";
+    return false;
+  }
+
+  if (!decoder_thread_.Start()) {
+    LOG(ERROR) << "Failed to start decoding thread.";

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

DLOG

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

Done.

+    return false;
+  }
+  decoder_task_runner_ = decoder_thread_.task_runner();
+
+  initialized_ = true;
+  return true;
+}
+
+bool VaapiJpegDecodeAccelerator::OutputPicture(
+    VASurfaceID va_surface_id,
+    int32_t input_buffer_id,
+    const scoped_refptr<media::VideoFrame>& video_frame) {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+
+  TRACE_EVENT1("jpeg", "VaapiJpegDecodeAccelerator::OutputPicture",
+               "input_buffer_id", input_buffer_id);
+
+  DVLOG(3) << "Outputting VASurface " << va_surface_id
+           << " into video_frame associate to input buffer id "
+           << input_buffer_id;
+
+  VAImage image;
+  VAImageFormat format;
+  const uint32_t kI420Fourcc = VA_FOURCC('I', '4', '2', '0');
+  memset(&image, 0, sizeof(image));
+  memset(&format, 0, sizeof(format));
+  format.fourcc = kI420Fourcc;
+  format.byte_order = VA_LSB_FIRST;
+  format.bits_per_pixel = 12;  // 12 for I420
+
+  void* mem;

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

nit: = nullptr;

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

Done.

+  gfx::Size coded_size = video_frame->coded_size();
+  if (!vaapi_wrapper_->GetVaImage(va_surface_id, &format, coded_size, &image,
+                                  &mem)) {
+    DLOG(ERROR) << "Cannot get VAImage";
+    return false;
+  }
+
+  uint8* frame_mem = video_frame->data(media::VideoFrame::kYPlane);
+  size_t frame_buffer_size =
+      media::VideoFrame::AllocationSize(media::VideoFrame::I420, coded_size);
+  memcpy(frame_mem, mem, frame_buffer_size);
+
+  vaapi_wrapper_->ReturnVaImage(&image);
+
+  if (client_)
+    client_->VideoFrameReady(input_buffer_id);
+
+  return true;
+}
+
+void VaapiJpegDecodeAccelerator::DecodeTask() {
+  DVLOG(3) << __func__;
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  TRACE_EVENT0("jpeg", "DecodeTask");
+  linked_ptr<DecodeRequest> request;
+  {
+    base::AutoLock auto_lock(lock_);
+
+    DCHECK(!decode_requests_.empty());
+    request = decode_requests_.front();
+    decode_requests_.pop();
+  }
+
+  do {
+    DVLOG(4) << "Mapping new input buffer id: "
+             << request->bitstream_buffer.id()
+             << " size: " << (int)request->bitstream_buffer.size();
+
+    scoped_ptr<base::SharedMemory> shm(
+        new base::SharedMemory(request->bitstream_buffer.handle(), true));
+    if (!shm->Map(request->bitstream_buffer.size())) {
+      LOG(ERROR) << "Failed to map input buffer";
+      NotifyError(request->bitstream_buffer.id(), UNREADABLE_INPUT);
+      break;
+    }
+
+    media::JpegParseResult parse_result;
+
+    if (!media::ParseJpegPicture(
+            reinterpret_cast<const uint8_t*>(shm->memory()),
+            request->bitstream_buffer.size(), &parse_result)) {
+      DLOG(ERROR) << "ParseJpegPicture failed";
+      NotifyError(request->bitstream_buffer.id(),
+                  media::JpegDecodeAccelerator::PARSE_JPEG_FAILED);
+      break;
+    }
+
+    // Reuse VASurface if size doesn't change.
+    // This is not only optimization, but also to avoid libva resoruce leak.

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

typo: s/resoruce/resource/

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

Done.

+    // crosbug.com/p/39584
+    gfx::Size coded_size(parse_result.frame_header.coded_width,
+                         parse_result.frame_header.coded_height);
+    if (coded_size != last_coded_size_ || va_surface_ == VA_INVALID_SURFACE) {
+      vaapi_wrapper_->DestroySurfaces();
+      std::vector<VASurfaceID> va_surfaces;
+      if (!vaapi_wrapper_->CreateSurfaces(coded_size, 1, &va_surfaces))
+        break;
+      va_surface_ = va_surfaces[0];
+      last_coded_size_ = coded_size;
+    }
+
+    if (!VaapiJpegDecoder::Decode(vaapi_wrapper_.get(), parse_result,
+                                  va_surface_)) {
+      LOG(ERROR) << "Decode failed";
+      NotifyError(request->bitstream_buffer.id(),
+                  media::JpegDecodeAccelerator::PLATFORM_FAILURE);
+      break;
+    }
+
+    if (!OutputPicture(va_surface_, request->bitstream_buffer.id(),
+                       request->video_frame)) {
+      LOG(ERROR) << "Output failed";
+      NotifyError(request->bitstream_buffer.id(),
+                  media::JpegDecodeAccelerator::PLATFORM_FAILURE);
+      break;
+    }
+  } while (0);
+}
+
+void VaapiJpegDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer,
+    const scoped_refptr<media::VideoFrame>& video_frame) {
+  DVLOG(3) << __func__;
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  TRACE_EVENT1("jpeg", "Decode", "input_id", bitstream_buffer.id());
+
+  if (!initialized_)

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

This is used outside the lock.

Even with a lock, I'm not convinced you don't have races. For example,
initialized_ could be true here, but a task could be pending on the main thread
to Cleanup, which will race against the DecodeTask on the decoder thread, so
what is the point of checking initialized_?

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

I will add a checking in DecodeTask. The check here could save a PostTask.

+    return;
+
+  // Set up a new decode request and queue it for later.
+  linked_ptr<DecodeRequest> input_buffer(
+      new DecodeRequest(bitstream_buffer, video_frame));
+  base::AutoLock auto_lock(lock_);
+  decode_requests_.push(input_buffer);
+
+  decoder_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&VaapiJpegDecodeAccelerator::DecodeTask,
+                            base::Unretained(this)));
+}
+
+void VaapiJpegDecodeAccelerator::Cleanup() {
+  DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get());
+
+  base::AutoLock auto_lock(lock_);
+  if (!initialized_)
+    return;
+
+  DVLOG(1) << "Destroying VaapiJpegDecodeAccelerator";
+
+  client_ptr_factory_.reset();
+  weak_this_factory_.InvalidateWeakPtrs();
+
+  {
+    base::AutoUnlock auto_unlock(lock_);
+    decoder_thread_.Stop();
+  }
+
+  initialized_ = false;
+}
+
+void VaapiJpegDecodeAccelerator::Destroy() {
+  DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get());
+  Cleanup();
+  delete this;
+}
+
+}  // namespace content