MJPEG acceleration for V4L2

content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc

Index: content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc
diff --git a/content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc b/content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d646b66474466c649506e5a7eb5f1d5069afe933
--- /dev/null
+++ b/content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc
@@ -0,0 +1,674 @@
+// 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 <fcntl.h>

[wuchengli, 2015/06/12 11:07:36]

What's this for?

[henryhsu, 2015/06/16 09:37:24]

removed.
I also removed poll.h, sys/eventfd.h, sys/ioctl.h.

+#include <linux/videodev2.h>
+#include <poll.h>
+#include <sys/eventfd.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+
+#include "base/bind.h"

[wuchengli, 2015/06/12 11:07:36]

do we need this?

[henryhsu, 2015/06/16 09:37:21]

We use base::Bind. We can remove bind_helpers.h and callback.h.

+#include "base/bind_helpers.h"
+#include "base/callback.h"

[wuchengli, 2015/06/12 11:07:36]

do we need this?

[henryhsu, 2015/06/16 09:37:23]

Removed.

+#include "base/thread_task_runner_handle.h"
+#include "content/common/gpu/media/v4l2_jpeg_decode_accelerator.h"
+#include "media/base/bind_to_current_loop.h"
+#include "media/base/video_frame.h"
+
+#define IOCTL_OR_ERROR_RETURN_VALUE(type, arg, value, type_name)      \
+  do {                                                                \
+    if (device_->Ioctl(type, arg) != 0) {                             \
+      PLOG(ERROR) << __func__ << "(): ioctl() failed: " << type_name; \
+      return value;                                                   \
+    }                                                                 \
+  } while (0)
+
+#define IOCTL_OR_ERROR_RETURN(type, arg) \
+  IOCTL_OR_ERROR_RETURN_VALUE(type, arg, ((void)0), #type)
+
+#define IOCTL_OR_ERROR_RETURN_FALSE(type, arg) \
+  IOCTL_OR_ERROR_RETURN_VALUE(type, arg, false, #type)
+
+#define IOCTL_OR_LOG_ERROR(type, arg)                             \
+  do {                                                            \
+    if (device_->Ioctl(type, arg) != 0)                           \
+      PLOG(ERROR) << __func__ << "(): ioctl() failed: " << #type; \
+  } while (0)
+
+
+namespace content {
+
+V4L2JpegDecodeAccelerator::InputRecord::InputRecord() : at_device(false) {

[wuchengli, 2015/06/15 07:55:27]

Initialize |address| and |length|.

[henryhsu, 2015/06/16 09:37:22]

Done.

+}
+
+V4L2JpegDecodeAccelerator::InputRecord::~InputRecord() {
+}
+
+V4L2JpegDecodeAccelerator::OutputRecord::OutputRecord()
+    : address(nullptr), length(0), at_device(false) {
+}
+
+V4L2JpegDecodeAccelerator::OutputRecord::~OutputRecord() {
+}
+
+V4L2JpegDecodeAccelerator::JobRecord::JobRecord(
+    media::BitstreamBuffer bitstream_buffer,
+    scoped_refptr<media::VideoFrame> video_frame)
+    : bitstream_buffer(bitstream_buffer), frame(video_frame) {
+}
+
+V4L2JpegDecodeAccelerator::JobRecord::~JobRecord() {
+}
+
+V4L2JpegDecodeAccelerator::V4L2JpegDecodeAccelerator(
+    const scoped_refptr<V4L2Device>& device,
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
+    : reset_buffer_flag_(false),

[kcwu, 2015/06/12 06:59:42]

s/false/0/

[henryhsu, 2015/06/16 09:37:21]

Done.

+      child_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      io_task_runner_(io_task_runner),
+      device_(device),
+      decoder_thread_("V4L2JpegDecodeThread"),
+      device_poll_thread_("V4L2JpegDecodeDevicePollThread"),
+      input_streamon_(false),
+      input_buffer_queued_count_(0),
+      output_streamon_(false),
+      output_buffer_queued_count_(0),
+      device_weak_factory_(this) {

[wuchengli, 2015/06/15 07:55:27]

Initialize output_format_(0), client_(nullptr)

[henryhsu, 2015/06/16 09:37:22]

Done.

+  device_weak_ = device_weak_factory_.GetWeakPtr();

[wuchengli, 2015/06/15 07:55:27]

This can be done in constructor member initializer list. Right?

[henryhsu, 2015/06/16 09:37:23]

Not used anymore.

+}
+
+V4L2JpegDecodeAccelerator::~V4L2JpegDecodeAccelerator() {
+  DCHECK(child_task_runner_->BelongsToCurrentThread());
+
+  // If the device thread is running, destroy using posted task.

[wuchengli, 2015/06/12 11:07:36]

s/device/decoder/

[wuchengli, 2015/06/15 07:55:27]

This comment doesn't provide too much information. I think this can be removed.

[henryhsu, 2015/06/16 09:37:22]

Done.

[henryhsu, 2015/06/16 09:37:23]

Done.

+  if (decoder_thread_.IsRunning()) {
+    decoder_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::DestroyTask,
+                              base::Unretained(this)));
+    // Wait for tasks to finish/early-exit.

[wuchengli, 2015/06/12 11:07:36]

Where is the code related to early-exit?

[wuchengli, 2015/06/15 07:55:28]

This comment doesn't provide too much information. I think this can be removed.

[henryhsu, 2015/06/16 09:37:22]

Done.

[henryhsu, 2015/06/16 09:37:22]

Done.

+    decoder_thread_.Stop();
+  }
+  DCHECK(!decoder_thread_.IsRunning());

[wuchengli, 2015/06/12 11:07:36]

Remove. The code above ensures this.

[henryhsu, 2015/06/16 09:37:22]

Done.

+  DCHECK(!device_poll_thread_.IsRunning());
+  DCHECK(!device_weak_factory_.HasWeakPtrs());
+}
+
+void V4L2JpegDecodeAccelerator::DestroyTask() {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+
+  device_weak_factory_.InvalidateWeakPtrs();

[wuchengli, 2015/06/12 11:07:36]

The invalidation and dereference of weak pointers have to be on the same thread.
Move this to ~V4L2JpegDecodeAccelerator (line 84).

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[Pawel Osciak, 2015/06/16 07:13:21]

Good catch. In V4L2ImageProcessor, the class this class is based on, the factory
is called device_weak_factory, because it's used from the device thread to
produce pointers for VideoFrame destruction callback. Which is later on
dereferenced on device_thread as well.

Here the weak pointer is used for something else, for producing and
dereferencing on child thread for use in NotifyError. So this is a different use
case and needs to be fixed (and the name of the factory changed as well).

[henryhsu, 2015/06/16 09:37:24]

Done.

+  // Stop streaming and the device_poll_thread_.
+  StopDevicePoll(false);
+
+  DestroyInputBuffers();
+  DestroyOutputBuffers();
+}
+
+void V4L2JpegDecodeAccelerator::NotifyError(int32_t bitstream_buffer_id,
+                                            Error error) {
+  DCHECK(child_task_runner_->BelongsToCurrentThread());
+  LOG(ERROR) << "Notifying of error " << error << " for buffer id "
+             << bitstream_buffer_id;
+  DCHECK(client_);
+  client_->NotifyError(bitstream_buffer_id, error);
+}
+
+void V4L2JpegDecodeAccelerator::NotifyErrorFromDecoderThread(
+    int32_t bitstream_buffer_id,
+    Error error) {

[wuchengli, 2015/06/12 11:07:36]

combine with the previous line? Does git cl format do this?

[henryhsu, 2015/06/16 09:37:23]

Yes. git cl format did this.

+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());

[wuchengli, 2015/06/15 07:55:28]

This function can be called from poll thread. Change the function name to
PostNotifyError?

[Pawel Osciak, 2015/06/16 07:13:21]

I don't see this method called from anywhere in this CL... NotifyError is called
from all threads though...

[henryhsu, 2015/06/16 09:37:22]

Done.

[henryhsu, 2015/06/16 09:37:22]

Fixed

+  child_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::NotifyError,
+                            device_weak_, bitstream_buffer_id, error));

[kcwu, 2015/06/12 06:59:42]

s/device_weak_/device_weak_factory_.GetWeakPtr()/

[henryhsu, 2015/06/16 09:37:23]

Done.

+}
+
+bool V4L2JpegDecodeAccelerator::Initialize(Client* client) {
+  DCHECK(child_task_runner_->BelongsToCurrentThread());
+
+  client_ = client;

[wuchengli, 2015/06/12 11:07:36]

Set client_ after other initialization succeeds (after line 145).

[henryhsu, 2015/06/16 09:37:23]

Done.

+
+  // Capabilities check.
+  struct v4l2_capability caps;
+  const __u32 kCapsRequired = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M;
+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QUERYCAP, &caps);
+  if ((caps.capabilities & kCapsRequired) != kCapsRequired) {
+    LOG(ERROR) << "Initialize(): ioctl() failed: VIDIOC_QUERYCAP"

[wuchengli, 2015/06/12 11:07:36]

Remove "ioctl() failed: ". Ioctl actually succeeds. It's capabilities check
failed.

[henryhsu, 2015/06/16 09:37:22]

Done.

+                  ", caps check failed: 0x" << std::hex << caps.capabilities;
+    return false;
+  }
+
+  if (!decoder_thread_.Start()) {
+    LOG(ERROR) << "Initialize(): encoder thread failed to start";

[wuchengli, 2015/06/15 05:58:37]

s/encoder/decoder/

[henryhsu, 2015/06/16 09:37:22]

Done.

+    return false;
+  }
+  decoder_task_runner_ = decoder_thread_.task_runner();
+
+  decoder_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::StartDevicePoll,
+                            base::Unretained(this)));
+
+  DVLOG(1) << "V4L2JpegDecodeAccelerator initialized.";
+  return true;
+}
+
+void V4L2JpegDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer,
+    const scoped_refptr<media::VideoFrame>& video_frame) {
+  DVLOG(1) << "Decode(): input_id=" << bitstream_buffer.id()
+           << ", size=" << bitstream_buffer.size();
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK_EQ(video_frame->format(), media::VideoFrame::I420);
+
+  scoped_ptr<JobRecord> job_record(

[wuchengli, 2015/06/15 07:55:27]

We don't need to use scoped_ptr. Copying BitstreamBuffer is cheap and
JobRecord::frame is a scoped_refptr.

[henryhsu, 2015/06/16 09:37:22]

As discussed. keep scoped_ptr here.

+      new JobRecord(bitstream_buffer, video_frame));
+
+  decoder_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::DecodeTask,
+                            base::Unretained(this), base::Passed(&job_record)));
+}
+
+void V4L2JpegDecodeAccelerator::DecodeTask(scoped_ptr<JobRecord> job_record) {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  input_queue_.push(make_linked_ptr(job_record.release()));
+  if (!CheckBufferAttributes())
+    return;
+  if (!reset_buffer_flag_) {

[wuchengli, 2015/06/15 05:58:37]

This is always set to false in ResetBuffers. Remove.

[henryhsu, 2015/06/16 09:37:23]

Done.

+    Enqueue();
+    device_poll_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::DevicePollTask,
+                              base::Unretained(this)));
+  }
+}
+
+bool V4L2JpegDecodeAccelerator::CheckBufferAttributes() {

[wuchengli, 2015/06/12 11:07:36]

This is a bad name. This function does more than checking attributes. Need
refactor here.

[wuchengli, 2015/06/15 05:58:37]

CreateBuffersIfNecessary should be a better name.

[henryhsu, 2015/06/16 09:37:23]

Done.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  DVLOG(3) << __func__;

[Pawel Osciak, 2015/06/16 07:13:21]

What happens if a DecodeTask() that requires buffer change is called before the
device finishes processing previous buffers/jobs? Won't we streamoff the queues,
cancelling in-flight tasks before they are finished?

I think you'll need to pend the buffer set change and handle it after all
in-flight jobs are finished.

[henryhsu, 2015/06/16 09:37:21]

We will wait all in-flight jobs are finished. And then streamoff, recreate
buffers, streamon, and enqueue buffer.

+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  DCHECK(!input_queue_.empty());
+  linked_ptr<JobRecord> job_record = input_queue_.front();
+  uint32_t reset_input_buffer = 0, reset_output_buffer = 0;
+
+  // Check input buffer size is enough
+  if (input_buffer_map_.empty() ||
+      job_record->bitstream_buffer.size() > input_buffer_map_.front().length) {
+    reset_input_buffer = kResetInputBuffer;
+  }
+
+  // Check image resolution and format are the same as previous.
+  if (job_record->frame->format() != output_format_ ||

[wuchengli, 2015/06/12 11:07:36]

This can be removed because DCHECK of line 162 means the format shouldn't
change.

[wuchengli, 2015/06/15 07:55:27]

|output_format_| can also be removed.

[henryhsu, 2015/06/16 09:37:23]

Done.

[henryhsu, 2015/06/16 09:37:24]

Done.

+      job_record->frame->coded_size() != image_coded_size_) {
+    size_t frame_size = media::VideoFrame::AllocationSize(
+        job_record->frame->format(), job_record->frame->coded_size());
+    if (output_buffer_map_.empty() ||
+        frame_size > output_buffer_map_.front().length) {
+      reset_output_buffer = kResetOutputBuffer;
+    }
+  }
+
+  if (reset_input_buffer || reset_output_buffer) {
+    if (input_streamon_ || output_streamon_) {
+      reset_buffer_flag_ = reset_input_buffer | reset_output_buffer;

[wuchengli, 2015/06/15 07:55:28]

As we discussed, suppose there are two resolution changes in |input_queue_|. The
first resolution change is not done yet. The second resolution change may
overwrite the value of |reset_buffer_flag_|. So the check of
|reset_input_buffer| and |reset_output_buffer| should be done just before
Enqueue to avoid this.

[henryhsu, 2015/06/16 09:37:23]

Done.

+      ResetBuffers();
+    } else {
+      image_coded_size_ = job_record->frame->coded_size();

[wuchengli, 2015/06/15 07:55:28]

Set |image_coded_size_| at the end of CreateOutputBuffers to make sure output
buffers and |image_coded_size_| will always match. Same for line 246.

[henryhsu, 2015/06/16 09:37:24]

Done.

+      output_format_ = job_record->frame->format();
+      if (!CreateInputBuffers() || !CreateOutputBuffers()) {
+        LOG(ERROR) << "Create Input/Output buffer failed.";
+        return false;
+      }
+    }
+  }
+  return true;
+}
+
+void V4L2JpegDecodeAccelerator::ResetBuffers() {
+  DVLOG(3) << __func__;
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  if (input_buffer_queued_count_ || output_buffer_queued_count_)
+    return;
+
+  if (!StopDevicePoll(true)) {
+    LOG(ERROR) << "Stop device poll thread failed when renew buffers.";
+  }
+
+  DCHECK(!input_queue_.empty());
+  linked_ptr<JobRecord> job_record = input_queue_.front();
+
+  if (reset_buffer_flag_ & kResetInputBuffer) {
+    DestroyInputBuffers();
+    CreateInputBuffers();
+  }
+
+  if (reset_buffer_flag_ & kResetOutputBuffer) {
+    DestroyOutputBuffers();
+
+    image_coded_size_ = job_record->frame->coded_size();
+    output_format_ = job_record->frame->format();
+    CreateOutputBuffers();
+  }
+
+  reset_buffer_flag_ = 0;
+  Enqueue();
+  StartDevicePoll();
+}
+
+bool V4L2JpegDecodeAccelerator::CreateInputBuffers() {
+  DVLOG(3) << __func__;
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  DCHECK(!input_streamon_);
+
+  DCHECK(!input_queue_.empty());
+  linked_ptr<JobRecord> job_record = input_queue_.front();
+  size_t reserve_size = job_record->bitstream_buffer.size() * 2;

[wuchengli, 2015/06/15 07:55:28]

Add a simple comment to explain why we multiple by 2.

[henryhsu, 2015/06/16 09:37:23]

Done.

+
+  struct v4l2_format format;
+  memset(&format, 0, sizeof(format));
+  format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+  format.fmt.pix.width = job_record->frame->coded_size().width();
+  format.fmt.pix.height = job_record->frame->coded_size().height();
+  format.fmt.pix.pixelformat = V4L2_PIX_FMT_JPEG;
+  format.fmt.pix.sizeimage = reserve_size;
+  format.fmt.pix.field = V4L2_FIELD_ANY;
+  format.fmt.pix.bytesperline = 0;

[Pawel Osciak, 2015/06/16 07:13:21]

Not needed (memset above).

[henryhsu, 2015/06/16 09:37:22]

Done.

+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_S_FMT, &format);
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = kInputBufferCount;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;

[Pawel Osciak, 2015/06/16 07:13:21]

Could we use _MPLANE API?

[henryhsu, 2015/06/16 09:37:22]

s5p-jpeg doesn't support MPLANE.

[wuchengli, 2015/06/22 22:40:53]

Pawel. Do we need MPLANE? The input is JPEG and doesn't need MPLANE. We use one
shared memory for one output frame. So using MPLANE means memcpy? I think single
plane is ok.

+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_REQBUFS, &reqbufs);
+
+  DCHECK(input_buffer_map_.empty());
+  input_buffer_map_.resize(reqbufs.count);
+
+  for (size_t i = 0; i < input_buffer_map_.size(); ++i) {
+    free_input_buffers_.push_back(i);
+
+    struct v4l2_buffer buffer;
+    memset(&buffer, 0, sizeof(buffer));
+    buffer.index = i;
+    buffer.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+    buffer.memory = V4L2_MEMORY_MMAP;
+    IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QUERYBUF, &buffer);
+    void* address = device_->Mmap(NULL, buffer.length, PROT_READ | PROT_WRITE,
+                                  MAP_SHARED, buffer.m.offset);
+    if (address == MAP_FAILED) {
+      PLOG(ERROR) << "CreateInputBuffers(): mmap() failed";
+      return false;
+    }
+    input_buffer_map_[i].address = address;
+    input_buffer_map_[i].length = buffer.length;
+  }
+  return true;
+}
+
+bool V4L2JpegDecodeAccelerator::CreateOutputBuffers() {
+  DVLOG(3) << __func__;
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  DCHECK(!output_streamon_);
+
+  DCHECK(!input_queue_.empty());
+  linked_ptr<JobRecord> job_record = input_queue_.front();
+
+  size_t frame_size = media::VideoFrame::AllocationSize(
+      output_format_, job_record->frame->coded_size());
+  uint32_t output_format_fourcc_ = V4L2_PIX_FMT_YUV420;

[Pawel Osciak, 2015/06/16 07:13:21]

Could we have a check that VideoFrame format is YUV420 as well?

[henryhsu, 2015/06/16 09:37:23]

We have it in Decode function.

+  struct v4l2_format format;
+  memset(&format, 0, sizeof(format));
+  format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+  format.fmt.pix.width = job_record->frame->coded_size().width();
+  format.fmt.pix.height = job_record->frame->coded_size().height();
+  format.fmt.pix.sizeimage = frame_size;
+  format.fmt.pix.pixelformat = output_format_fourcc_;

[wuchengli, 2015/06/15 07:55:27]

Just use V4L2_PIX_FMT_YUV420 here. Remove output_format_fourcc_.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  format.fmt.pix.field = V4L2_FIELD_ANY;
+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_S_FMT, &format);
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = kOutputBufferCount;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_REQBUFS, &reqbufs);
+
+  DCHECK(output_buffer_map_.empty());
+  output_buffer_map_.resize(reqbufs.count);
+
+  for (size_t i = 0; i < output_buffer_map_.size(); ++i) {
+    free_output_buffers_.push_back(i);
+
+    struct v4l2_buffer buffer;
+    memset(&buffer, 0, sizeof(buffer));
+    buffer.index = i;
+    buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+    buffer.memory = V4L2_MEMORY_MMAP;
+    IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QUERYBUF, &buffer);
+    void* address = device_->Mmap(NULL, buffer.length, PROT_READ | PROT_WRITE,
+                                  MAP_SHARED, buffer.m.offset);
+    if (address == MAP_FAILED) {
+      PLOG(ERROR) << "CreateOutputBuffers(): mmap() failed";
+      return false;
+    }
+    output_buffer_map_[i].address = address;
+    output_buffer_map_[i].length = buffer.length;
+  }
+  return true;
+}
+
+void V4L2JpegDecodeAccelerator::DestroyInputBuffers() {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  DCHECK(!input_streamon_);
+
+  for (size_t buf = 0; buf < input_buffer_map_.size(); ++buf) {
+    InputRecord& input_record = input_buffer_map_[buf];
+    device_->Munmap(input_record.address, input_record.length);
+  }
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = 0;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_LOG_ERROR(VIDIOC_REQBUFS, &reqbufs);
+
+  input_buffer_map_.clear();
+  free_input_buffers_.clear();
+}
+
+void V4L2JpegDecodeAccelerator::DestroyOutputBuffers() {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  DCHECK(!output_streamon_);
+
+  for (size_t buf = 0; buf < output_buffer_map_.size(); ++buf) {
+    OutputRecord& output_record = output_buffer_map_[buf];
+    device_->Munmap(output_record.address, output_record.length);
+  }
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = 0;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_LOG_ERROR(VIDIOC_REQBUFS, &reqbufs);
+
+  output_buffer_map_.clear();
+  free_output_buffers_.clear();
+}
+
+void V4L2JpegDecodeAccelerator::DevicePollTask() {
+  DCHECK(device_poll_task_runner_->BelongsToCurrentThread());
+
+  bool event_pending;
+  if (!device_->Poll(true, &event_pending)) {
+    NotifyError(-1, media::JpegDecodeAccelerator::PLATFORM_FAILURE);

[wuchengli, 2015/06/15 07:55:27]

NotifyErrorFromDecoderThread

[henryhsu, 2015/06/16 09:37:23]

Done.

+    return;
+  }
+
+  // All processing should happen on ServiceDeviceTask(), since we shouldn't
+  // touch encoder state from this thread.

[wuchengli, 2015/06/15 05:58:37]

s/encoder/decoder/

[henryhsu, 2015/06/16 09:37:22]

Done.

+  decoder_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::ServiceDeviceTask,
+                            base::Unretained(this)));
+}
+
+void V4L2JpegDecodeAccelerator::ServiceDeviceTask() {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  // ServiceDeviceTask() should only ever be scheduled from DevicePollTask(),
+  // so either:
+  // * device_poll_thread_ is running normally
+  // * device_poll_thread_ scheduled us, but then a DestroyTask() shut it down,
+  //   in which case we should early-out.
+  if (!device_poll_thread_.IsRunning())
+    return;
+
+  Dequeue();
+  Enqueue();
+
+  if (!device_->ClearDevicePollInterrupt()) {
+    return;
+  }
+
+  if (input_buffer_queued_count_ > 0 && output_buffer_queued_count_ > 0) {
+    device_poll_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&V4L2JpegDecodeAccelerator::DevicePollTask,
+                              base::Unretained(this)));
+  }
+
+  DVLOG(2) << __func__ << ": buffer counts: INPUT["
+           << input_queue_.size() << "] => DEVICE["
+           << free_input_buffers_.size() << "+"
+           << input_buffer_queued_count_ << "/"
+           << input_buffer_map_.size() << "->"
+           << free_output_buffers_.size() << "+"
+           << output_buffer_queued_count_ << "/"
+           << output_buffer_map_.size() << "] => CLIENT["
+           << output_buffer_map_.size() - output_buffer_queued_count_ -
+              free_output_buffers_.size() << "]";
+}
+
+void V4L2JpegDecodeAccelerator::Enqueue() {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+
+  const int old_inputs_queued = input_buffer_queued_count_;
+  while (!input_queue_.empty() && !free_input_buffers_.empty()) {

[wuchengli, 2015/06/12 11:07:36]

Can we check the resolution change here? If the size has changed, do not enqueue
it.

[henryhsu, 2015/06/16 09:37:23]

I check resolution change before Enqueue() function. I'd like to keep Enqueue
function simply and do enqueue directly.

+    if (!EnqueueInputRecord())
+      return;
+  }
+  if (old_inputs_queued == 0 && input_buffer_queued_count_ != 0) {

[wuchengli, 2015/06/12 11:07:36]

Why do we need old_inputs_queued == 0 check?

[henryhsu, 2015/06/16 09:37:23]

Removed.

+    // Start VIDIOC_STREAMON if we haven't yet.
+    if (!input_streamon_) {
+      __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+      IOCTL_OR_ERROR_RETURN(VIDIOC_STREAMON, &type);
+      input_streamon_ = true;
+    }
+  }
+
+  const int old_outputs_queued = output_buffer_queued_count_;
+  while (output_buffer_queued_count_ < input_buffer_queued_count_ &&
+         !free_output_buffers_.empty()) {
+    if (!EnqueueOutputRecord())
+      return;
+  }
+  if (old_outputs_queued == 0 && output_buffer_queued_count_ != 0) {
+    // Start VIDIOC_STREAMON if we haven't yet.
+    if (!output_streamon_) {
+      __u32 type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+      IOCTL_OR_ERROR_RETURN(VIDIOC_STREAMON, &type);
+      output_streamon_ = true;
+    }
+  }
+}
+
+void V4L2JpegDecodeAccelerator::Dequeue() {
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+
+  // Dequeue completed input (VIDEO_OUTPUT) buffers,
+  // and recycle to the free list.
+  struct v4l2_buffer dqbuf;
+  while (input_buffer_queued_count_ > 0) {
+    DCHECK(input_streamon_);
+    memset(&dqbuf, 0, sizeof(dqbuf));
+    dqbuf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+    dqbuf.memory = V4L2_MEMORY_MMAP;
+    if (device_->Ioctl(VIDIOC_DQBUF, &dqbuf) != 0) {
+      if (errno == EAGAIN) {
+        // EAGAIN if we're just out of buffers to dequeue.
+        break;
+      }
+      PLOG(ERROR) << "ioctl() failed: VIDIOC_DQBUF";
+      NotifyError(dqbuf.index, media::JpegDecodeAccelerator::PLATFORM_FAILURE);

[wuchengli, 2015/06/15 07:55:27]

NotifyErrorFromDecoderThread

[henryhsu, 2015/06/16 09:37:23]

Done.

+      return;
+    }
+    InputRecord& input_record = input_buffer_map_[dqbuf.index];
+    DCHECK(input_record.at_device);
+    input_record.at_device = false;
+    free_input_buffers_.push_back(dqbuf.index);
+    input_buffer_queued_count_--;
+  }
+
+  // Dequeue completed output (VIDEO_CAPTURE) buffers, recycle to the free list.
+  // Return the finished buffer to the client via the job ready callback.
+  while (output_buffer_queued_count_ > 0) {
+    DCHECK(output_streamon_);
+    memset(&dqbuf, 0, sizeof(dqbuf));
+    dqbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+    dqbuf.memory = V4L2_MEMORY_MMAP;
+    if (device_->Ioctl(VIDIOC_DQBUF, &dqbuf) != 0) {
+      if (errno == EAGAIN) {
+        // EAGAIN if we're just out of buffers to dequeue.
+        break;
+      }
+      PLOG(ERROR) << "ioctl() failed: VIDIOC_DQBUF";
+      NotifyError(dqbuf.index, media::JpegDecodeAccelerator::PLATFORM_FAILURE);

[wuchengli, 2015/06/15 07:55:28]

NotifyErrorFromDecoderThread

[henryhsu, 2015/06/16 09:37:23]

Done.

+      return;
+    }
+    OutputRecord& output_record = output_buffer_map_[dqbuf.index];
+    DCHECK(output_record.at_device);
+    output_record.at_device = false;
+    free_output_buffers_.push_back(dqbuf.index);
+    output_buffer_queued_count_--;
+
+    // Jobs are always processed in FIFO order.
+    DCHECK(!running_jobs_.empty());
+    linked_ptr<JobRecord> job_record = running_jobs_.front();
+    running_jobs_.pop();
+
+    memcpy(job_record->frame->data(media::VideoFrame::kYPlane),

[Pawel Osciak, 2015/06/16 07:13:21]

Please use libyuv:: copy methods to account for strides.
Also, please do this before requeueing output buffer back and reinitializing
things, just to make it better understandable/readable.

[henryhsu, 2015/06/16 09:37:21]

output_record.length is calculated from VideoFrame::AllocationSize. I think it
is fine. I moved ResetBuffers to ServiceDeviceTask. So this is before requeueing
output buffer and reinitialization.

+           output_record.address, output_record.length);
+
+    DVLOG(3) << "Processing finished, returning frame, ts="

[wuchengli, 2015/06/12 11:07:36]

s/Processing/Decoding/

[henryhsu, 2015/06/16 09:37:21]

Done.

+             << job_record->frame->timestamp().InMilliseconds();
+
+    DCHECK(client_);

[wuchengli, 2015/06/12 11:07:36]

This isn't very useful. If this DCHECK doesn't exist, the code just crashes in
the next line. The effect is almost the same. Remove.

[henryhsu, 2015/06/16 09:37:23]

Done.

+    client_->VideoFrameReady(job_record->bitstream_buffer.id());
+    if (reset_buffer_flag_)
+      ResetBuffers();

[wuchengli, 2015/06/12 11:07:36]

Move this out of this function.

[wuchengli, 2015/06/15 05:58:37]

Do we need to do this in Dequeue or ServiceDeviceTask? It seems it's enough to
just do it in DecodeTask.

[henryhsu, 2015/06/16 09:37:21]

Moved to ServiceDeviceTask.

[henryhsu, 2015/06/16 09:37:22]

DecodeTask only handles queue empty case. We still need this and I move this
section to ServiceDeviceTask.

+  }
+}
+
+bool V4L2JpegDecodeAccelerator::EnqueueInputRecord() {
+  DCHECK(!input_queue_.empty());
+  DCHECK(!free_input_buffers_.empty());
+
+  // Enqueue an input (VIDEO_OUTPUT) buffer for an input video frame.
+  linked_ptr<JobRecord> job_record = input_queue_.front();
+  input_queue_.pop();
+  const int index = free_input_buffers_.back();
+  InputRecord& input_record = input_buffer_map_[index];
+  DCHECK(!input_record.at_device);
+
+  scoped_ptr<base::SharedMemory> shm(
+      new base::SharedMemory(job_record->bitstream_buffer.handle(), true));
+  if (!shm->Map(job_record->bitstream_buffer.size())) {

[Pawel Osciak, 2015/06/16 07:13:21]

Could we do this earlier? We could map before getting a free input buffer,
immediately when we get this on Decode, potentially squeezing out a bit more
parallelism.

[henryhsu, 2015/06/16 09:37:22]

Done.

+    LOG(ERROR) << "Decode(): could not map bitstream_buffer";
+    NotifyError(job_record->bitstream_buffer.id(), UNREADABLE_INPUT);
+    return false;
+  }
+  struct v4l2_buffer qbuf;
+  memset(&qbuf, 0, sizeof(qbuf));
+  memcpy(input_record.address, shm->memory(),
+         job_record->bitstream_buffer.size());
+  qbuf.index = index;
+  qbuf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+  qbuf.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QBUF, &qbuf);
+  input_record.at_device = true;
+  running_jobs_.push(job_record);
+  free_input_buffers_.pop_back();
+  input_buffer_queued_count_++;

[wuchengli, 2015/06/15 07:55:27]

You can see we always manipulate |free_input_buffers_| and
|input_buffer_queued_count_| together. We can use a function to get
input_buffer_queued_count_. We can maintain one less variable.
int input_buffer_queued_count() {
  return input_buffer_map_.size() - free_input_buffers_();
}

Same for |output_buffer_queued_count_|.

[henryhsu, 2015/06/16 09:37:23]

We use input_buffer_queued_count to check queue empty in many places. I'd prefer
to use two more variables to save one subtraction for each time.

[wuchengli, 2015/06/22 22:40:53]

The time saving is very small. The point is it's easy to forget to change them
at the same time. It's also harder to see if the code is correct. For example,
in line 355 at the end of DestroyInputBuffers, do we need to set
input_buffer_queued_count_ = 0 after REQBUFS(0)? Are we relying
input_buffer_queued_count_ = 0 before DestroyInputBuffers is called?

[henryhsu, 2015/06/23 10:08:16]

Done.

+
+  DVLOG(3) << __func__ << ": enqueued frame ts="
+           << job_record->frame->timestamp().InMilliseconds() << " to device.";
+  return true;
+}
+
+bool V4L2JpegDecodeAccelerator::EnqueueOutputRecord() {
+  DCHECK(!free_output_buffers_.empty());
+
+  // Enqueue an output (VIDEO_CAPTURE) buffer.
+  const int index = free_output_buffers_.back();
+  OutputRecord& output_record = output_buffer_map_[index];
+  DCHECK(!output_record.at_device);
+  struct v4l2_buffer qbuf;
+  memset(&qbuf, 0, sizeof(qbuf));
+  qbuf.index = index;
+  qbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+  qbuf.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QBUF, &qbuf);
+  output_record.at_device = true;
+  free_output_buffers_.pop_back();
+  output_buffer_queued_count_++;
+  return true;
+}
+
+void V4L2JpegDecodeAccelerator::StartDevicePoll() {
+  DVLOG(3) << __func__ << ": starting device poll";
+  DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+  DCHECK(!device_poll_thread_.IsRunning());
+
+  // Start up the device poll thread and schedule its first DevicePollTask().

[wuchengli, 2015/06/15 07:55:27]

There is no "schedule its first DevicePollTask" here. Remove this comment.

[Pawel Osciak, 2015/06/16 07:13:21]

Why do we not schedule it from here though?

[henryhsu, 2015/06/16 09:37:22]

Done.

[henryhsu, 2015/06/16 09:37:23]

We only schedule DevicePollTask when queue has buffers.

+  if (!device_poll_thread_.Start()) {
+    LOG(ERROR) << "StartDevicePoll(): Device thread failed to start";
+    NotifyError(-1, media::JpegDecodeAccelerator::PLATFORM_FAILURE);
+    return;
+  }
+  device_poll_task_runner_ = device_poll_thread_.task_runner();
+}
+
+bool V4L2JpegDecodeAccelerator::StopDevicePoll(bool keep_input_queue) {
+  DVLOG(3) << __func__ << ": stopping device poll";
+  if (decoder_thread_.IsRunning())
+    DCHECK(decoder_task_runner_->BelongsToCurrentThread());
+
+  // Signal the DevicePollTask() to stop, and stop the device poll thread.
+  if (!device_->SetDevicePollInterrupt())

[wuchengli, 2015/06/12 11:07:36]

VDA notifies error here. VEA and V4L2imageProcessor don't. I checked with Pawel.
SetDevicePollInterrupt is too simple to fail. Let's notify error here.

[henryhsu, 2015/06/16 09:37:22]

Done.

+    return false;
+
+  device_poll_thread_.Stop();
+
+  // Clear the interrupt now, to be sure.
+  if (!device_->ClearDevicePollInterrupt())
+    return false;
+
+  if (input_streamon_) {
+    __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+    IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_STREAMOFF, &type);
+  }
+  input_streamon_ = false;
+
+  if (output_streamon_) {
+    __u32 type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+    IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_STREAMOFF, &type);
+  }
+  output_streamon_ = false;
+
+  // Reset all our accounting info.
+  if (!keep_input_queue) {

[wuchengli, 2015/06/12 11:07:36]

Remove |keep_input_queue| and move the code out of this function.
while (!input_queue_.empty()) input_queue_.pop();

[henryhsu, 2015/06/16 09:37:22]

Done.

+    while (!input_queue_.empty())
+      input_queue_.pop();
+  }
+
+  while (!running_jobs_.empty())
+    running_jobs_.pop();

[wuchengli, 2015/06/12 11:07:36]

When the resolution changes, shouldn't we wait until all |running_jobs_|
finishes?

[henryhsu, 2015/06/16 09:37:21]

Sure. For resolution change case, running_jobs should be empty. But for
DestroyTask case, we just discard all jobs.

+
+  free_input_buffers_.clear();
+  for (size_t i = 0; i < input_buffer_map_.size(); ++i) {
+    InputRecord& input_record = input_buffer_map_[i];
+    input_record.at_device = false;
+    free_input_buffers_.push_back(i);
+  }
+  input_buffer_queued_count_ = 0;
+
+  free_output_buffers_.clear();
+  for (size_t i = 0; i < output_buffer_map_.size(); ++i) {
+    OutputRecord& output_record = output_buffer_map_[i];
+    output_record.at_device = false;
+    free_output_buffers_.push_back(i);
+  }
+  output_buffer_queued_count_ = 0;
+
+  return true;
+}
+
+}  // namespace content