VDA implementation for Exynos, using V4L2

content/common/gpu/media/exynos_video_decode_accelerator.cc

+// Copyright (c) 2012 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 <dlfcn.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/videodev2.h>
+#include <poll.h>
+#include <sys/eventfd.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+
+#include "base/bind.h"
+#include "base/debug/trace_event.h"
+#include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
+#include "base/shared_memory.h"
+#include "content/common/gpu/media/exynos_video_decode_accelerator.h"
+#include "content/common/gpu/media/h264_parser.h"
+#include "third_party/angle/include/GLES2/gl2.h"
+
+namespace content {
+
+#define EXYNOS_MFC_DEVICE "/dev/mfc-dec"
+#define EXYNOS_GSC_DEVICE "/dev/gsc1"
+#define EXYNOS_MALI_DRIVER "libmali.so"
+
+#define NOTIFY_ERROR(x)                                             \

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

What does this macro buy you over simply moving the SetDecoderState() call into
NotifyError()?

[sheu, 2013/01/05 00:51:49]

NotifyError() calls itself if called not-on the child thread, so a
SetDecoderState() in that case would be called twice.

I broke out the NotifyError into a macro also so LOG(ERROR) would pick up the
actual source line where the macro is invoked from.

+  do {                                                              \
+    SetDecoderState(kError);                                        \
+    LOG(ERROR) << "calling NotifyError(): " << x;                   \

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

DLOG ?

[sheu, 2013/01/05 00:51:49]

Something actually broke.  I think it's import enough to LOG for.

[Ami GONE FROM CHROMIUM, 2013/01/11 00:46:33]

We don't LOG for invalid .png's; we shouldn't LOG for invalid .mp4 streams.  

[sheu, 2013/01/11 10:41:42]

Still think it should be a LOG, but I'm not going to argue.  I was tempted to
make it a VLOG but then I figured that's worse for clutter.

+    NotifyError(x);                                                 \
+  } while (0)
+
+#define IOCTL_OR_ERROR_RETURN(fd, type, arg)                        \
+  do {                                                              \
+    if (ioctl(fd, type, arg) != 0) {                                \
+      DPLOG(ERROR) << __func__ << "(): ioctl() failed: " << #type;  \

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

FWIW the last << is unnecessary.

[sheu, 2013/01/05 00:51:49]

There's a couple other bits (e.g. l.280) where I was advised to use it even for
string concat.  Your call.

+      NOTIFY_ERROR(PLATFORM_FAILURE);                               \
+      return;                                                       \
+    }                                                               \
+  } while (0)
+
+#define IOCTL_OR_ERROR_RETURN_FALSE(fd, type, arg)                  \
+  do {                                                              \
+    if (ioctl(fd, type, arg) != 0) {                                \
+      DPLOG(ERROR) << __func__ << "(): ioctl() failed: " << #type;  \
+      NOTIFY_ERROR(PLATFORM_FAILURE);                               \
+      return false;                                                 \
+    }                                                               \
+  } while (0)
+
+typedef void* GLeglImageOES;

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

Any reason this doesn't come from a GLES2 header?

[sheu, 2013/01/05 00:51:49]

It's not in any one of them that we use.

I'd like to go do a GLES2 header cleanup at some point.

+static void* libmali_handle = NULL;
+static EGLBoolean(*mali_egl_image_get_buffer_ext_phandle)(

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

here and below having no space between the return type and the (* looks weird.  

[sheu, 2013/01/05 00:51:49]

I was doing it analogously with function calls.  Done.

+    EGLImageKHR, EGLint*, void*) = NULL;
+static EGLImageKHR(*egl_create_image_khr)(
+    EGLDisplay, EGLContext, EGLenum, EGLClientBuffer, const EGLint*) = NULL;
+static EGLBoolean(*egl_destroy_image_khr)(
+    EGLDisplay, EGLImageKHR) = NULL;
+static EGLSyncKHR(*egl_create_sync_khr)(
+    EGLDisplay, EGLenum, const EGLint*) = NULL;
+static EGLBoolean(*egl_destroy_sync_khr)(
+    EGLDisplay, EGLSyncKHR) = NULL;
+static EGLint(*egl_client_wait_sync_khr)(
+    EGLDisplay, EGLSyncKHR, EGLint, EGLTimeKHR) = NULL;
+static void(*gl_egl_image_target_texture_2d_oes)(
+    GLenum, GLeglImageOES) = NULL;
+
+ExynosVideoDecodeAccelerator::BitstreamBufferRef::BitstreamBufferRef(
+    base::WeakPtr<Client>& client,
+    scoped_refptr<base::MessageLoopProxy>& client_message_loop_proxy,
+    base::SharedMemory* shm, size_t size, int32 input_id)
+    : client(client),
+      client_message_loop_proxy(client_message_loop_proxy),
+      shm(shm),
+      size(size),
+      bytes_used(0),
+      input_id(input_id) {
+}
+
+ExynosVideoDecodeAccelerator::BitstreamBufferRef::~BitstreamBufferRef() {
+  if (input_id != -1 && client_message_loop_proxy != NULL)

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

when can the proxy be NULL?

[sheu, 2013/01/05 00:51:49]

I suppose not.  Done.

+    client_message_loop_proxy->PostTask(FROM_HERE, base::Bind(
+        &Client::NotifyEndOfBitstreamBuffer, client, input_id));
+}
+
+ExynosVideoDecodeAccelerator::PictureBufferArrayRef::PictureBufferArrayRef(
+    EGLDisplay egl_display, EGLImageKHR egl_images[], int egl_image_fds[],
+    int32 client_ids[], int egl_images_count)
+    : egl_display(egl_display),
+      egl_images(egl_images),
+      egl_image_fds(egl_image_fds),
+      client_ids(client_ids),
+      egl_images_count(egl_images_count) {
+}
+
+ExynosVideoDecodeAccelerator::PictureBufferArrayRef::~PictureBufferArrayRef() {
+  DCHECK_EQ(egl_images != NULL, egl_image_fds != NULL);
+  if (egl_images == NULL)
+    return;
+
+  for (int i = 0; i < egl_images_count; ++i) {
+    if (egl_images[i] != EGL_NO_IMAGE_KHR)
+      egl_destroy_image_khr(egl_display, egl_images[i]);
+    if (egl_image_fds[i] != -1)
+      close(egl_image_fds[i]);
+  }
+}
+
+ExynosVideoDecodeAccelerator::EGLSyncKHRRef::EGLSyncKHRRef(
+    EGLDisplay egl_display, EGLSyncKHR egl_sync)
+    : egl_display(egl_display),
+      egl_sync(egl_sync) {
+}
+
+ExynosVideoDecodeAccelerator::EGLSyncKHRRef::~EGLSyncKHRRef() {
+  if (egl_sync != EGL_NO_SYNC_KHR)
+    egl_destroy_sync_khr(egl_display, egl_sync);
+}
+
+ExynosVideoDecodeAccelerator::MfcInputRecord::MfcInputRecord()
+    : at_device(false),
+      address(NULL),
+      length(0),
+      bytes_used(0),
+      input_id(-1) {
+}
+
+ExynosVideoDecodeAccelerator::MfcInputRecord::~MfcInputRecord() {
+}
+
+ExynosVideoDecodeAccelerator::MfcOutputRecord::MfcOutputRecord()
+    : at_device(false),
+      input_id(-1) {

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

could zero-initialize the [2] members, as well.
(using initializer syntax)

[sheu, 2013/01/05 00:51:49]

I don't think you can do that in an initializer list.

+  bytes_used[0] = 0;
+  bytes_used[1] = 0;
+  address[0] = NULL;
+  address[1] = NULL;
+  length[0] = 0;
+  length[1] = 0;
+}
+
+ExynosVideoDecodeAccelerator::MfcOutputRecord::~MfcOutputRecord() {
+}
+
+ExynosVideoDecodeAccelerator::GscInputRecord::GscInputRecord()
+    : at_device(false),
+      mfc_output(-1) {
+}
+
+ExynosVideoDecodeAccelerator::GscInputRecord::~GscInputRecord() {
+}
+
+ExynosVideoDecodeAccelerator::GscOutputRecord::GscOutputRecord()
+    : at_device(false),
+      at_client(false),
+      fd(-1),
+      egl_image(EGL_NO_IMAGE_KHR),
+      egl_sync(EGL_NO_SYNC_KHR),
+      picture_id(-1) {
+}
+
+ExynosVideoDecodeAccelerator::GscOutputRecord::~GscOutputRecord() {
+}
+
+ExynosVideoDecodeAccelerator::ExynosVideoDecodeAccelerator(
+    EGLDisplay egl_display,
+    EGLContext egl_context,
+    Client* client,
+    const base::Callback<bool(void)>& make_context_current)
+    : child_message_loop_proxy_(base::MessageLoopProxy::current()),
+      weak_this_(base::AsWeakPtr(this)),
+      client_ptr_factory_(client),
+      client_(client_ptr_factory_.GetWeakPtr()),
+      decoder_thread_("ExynosDecoderThread"),
+      decoder_state_(kUninitialized),
+      decoder_current_bitstream_buffer_(NULL),
+      decoder_current_input_buffer_(-1),
+      decoder_decode_buffer_tasks_scheduled_(0),
+      decoder_frames_at_client_(0),
+      decoder_notify_flush_requested_(false),
+      mfc_fd_(-1),
+      mfc_input_streamon_(false),
+      mfc_input_buffer_count_(0),
+      mfc_input_buffer_queued_count_(0),
+      mfc_output_streamon_(false),
+      mfc_output_buffer_count_(0),
+      mfc_output_buffer_queued_count_(0),
+      mfc_output_buffer_pixelformat_(0),
+      gsc_fd_(-1),
+      gsc_input_streamon_(false),
+      gsc_input_buffer_count_(0),
+      gsc_input_buffer_queued_count_(0),
+      gsc_output_streamon_(false),
+      gsc_output_buffer_count_(0),
+      gsc_output_buffer_queued_count_(0),
+      frame_buffer_size_(0, 0),

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

equiv to default ctor?

[sheu, 2013/01/05 00:51:49]

Done.

+      device_poll_thread_("ExynosDevicePollThread"),
+      device_poll_interrupt_fd_(-1),
+      make_context_current_(make_context_current),
+      egl_display_(egl_display),
+      egl_context_(egl_context),
+      video_profile_(media::VIDEO_CODEC_PROFILE_UNKNOWN) {
+}
+
+ExynosVideoDecodeAccelerator::~ExynosVideoDecodeAccelerator() {
+  // Nuke the entire site from orbit -- it's the only way to be sure.

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

dcheck that the member threads have already joined?

[sheu, 2013/01/05 00:51:49]

Done.

+  if (device_poll_interrupt_fd_ != -1) {
+    close(device_poll_interrupt_fd_);
+    device_poll_interrupt_fd_ = -1;
+  }
+  if (gsc_fd_ != -1) {
+    DestroyGscInputBuffers();
+    DestroyGscOutputBuffers();
+    close(gsc_fd_);
+    gsc_fd_ = -1;
+  }
+  if (mfc_fd_ != -1) {
+    DestroyMfcInputBuffers();
+    DestroyMfcOutputBuffers();
+    close(mfc_fd_);
+    mfc_fd_ = -1;
+  }
+
+  // These maps have members that should be manually destroyed, e.g. file
+  // descriptors, mmap() segments, etc.
+  DCHECK(mfc_input_buffer_map_.empty());
+  DCHECK(mfc_output_buffer_map_.empty());
+  DCHECK(gsc_input_buffer_map_.empty());
+  DCHECK(gsc_output_buffer_map_.empty());
+}
+
+bool ExynosVideoDecodeAccelerator::Initialize(
+    media::VideoCodecProfile profile) {
+  DVLOG(3) << "Initialize()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK_EQ(decoder_state_, kUninitialized);
+
+  switch (profile) {
+    case media::H264PROFILE_BASELINE:
+      DVLOG(2) << "Initialize(): profile H264PROFILE_BASELINE";
+      break;
+    case media::H264PROFILE_MAIN:
+      DVLOG(2) << "Initialize(): profile H264PROFILE_MAIN";
+      break;
+    case media::H264PROFILE_HIGH:
+      DVLOG(2) << "Initialize(): profile H264PROFILE_HIGH";
+      break;
+    case media::VP8PROFILE_MAIN:
+      DVLOG(2) << "Initialize(): profile VP8PROFILE_MAIN";
+      break;
+    default:
+      DLOG(ERROR) << "Initialize(): unsupported profile=" << profile;
+      return false;
+  };
+  video_profile_ = profile;
+
+  static bool sandbox_initialized = PostSandboxInitialization();
+  if (!sandbox_initialized) {
+    DLOG(ERROR) << "Initialize(): PostSandboxInitialization() failed";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  if (egl_display_ == EGL_NO_DISPLAY) {
+    DLOG(ERROR) << "Initialize(): could not get EGLDisplay";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  if (egl_context_ == EGL_NO_CONTEXT) {
+    DLOG(ERROR) << "Initialize(): could not get EGLContext";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  // Open the video devices.
+  DVLOG(2) << "Initialize(): opening MFC device: " << EXYNOS_MFC_DEVICE;
+  mfc_fd_ = open(EXYNOS_MFC_DEVICE, O_RDWR | O_NONBLOCK | O_CLOEXEC);
+  if (mfc_fd_ == -1) {
+    DPLOG(ERROR) << "Initialize(): could not open MFC device: "
+                 << EXYNOS_MFC_DEVICE;
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+  DVLOG(2) << "Initialize(): opening GSC device: " << EXYNOS_GSC_DEVICE;
+  gsc_fd_ = open(EXYNOS_GSC_DEVICE, O_RDWR | O_NONBLOCK | O_CLOEXEC);
+  if (gsc_fd_ == -1) {
+    DPLOG(ERROR) << "Initialize(): could not open GSC device: "
+                 << EXYNOS_GSC_DEVICE;
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  // Create the interrupt fd.
+  DCHECK_EQ(device_poll_interrupt_fd_, -1);
+  device_poll_interrupt_fd_ = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+  if (device_poll_interrupt_fd_ == -1) {
+    DPLOG(ERROR) << "Initialize(): eventfd() failed";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  // Capabilities check.
+  struct v4l2_capability caps;
+  const __u32 kCapsRequired =
+      V4L2_CAP_VIDEO_CAPTURE_MPLANE |
+      V4L2_CAP_VIDEO_OUTPUT_MPLANE |
+      V4L2_CAP_STREAMING;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_QUERYCAP, &caps);
+  if ((caps.capabilities & kCapsRequired) != kCapsRequired) {
+    DLOG(ERROR) << "Initialize(): ioctl() failed: VIDIOC_QUERYCAP"
+        ", caps check failed: 0x" << std::hex << caps.capabilities;
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_QUERYCAP, &caps);
+  if ((caps.capabilities & kCapsRequired) != kCapsRequired) {
+    DLOG(ERROR) << "Initialize(): ioctl() failed: VIDIOC_QUERYCAP"
+        ", caps check failed: 0x" << std::hex << caps.capabilities;
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  // Some random ioctls that Exynos requires.
+  struct v4l2_control control;
+  memset(&control, 0, sizeof(control));
+  control.id = V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY; // also VP8
+  control.value = 8; // Magic number from Samsung folks.
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_S_CTRL, &control);
+
+  if (!make_context_current_.Run()) {
+    DLOG(ERROR) << "Initialize(): could not make context current";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  if (!CreateMfcInputBuffers())
+    return false;
+
+  // MFC output format has to be setup before streaming starts.
+  struct v4l2_format format;
+  memset(&format, 0, sizeof(format));
+  format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  format.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_NV12MT_16X16;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_S_FMT, &format);
+
+  // Initialize format-specific bits.
+  if (video_profile_ >= media::H264PROFILE_MIN &&
+      video_profile_ <= media::H264PROFILE_MAX) {
+    decoder_h264_parser_.reset(new content::H264Parser());
+  }
+
+  if (!decoder_thread_.Start()) {
+    DLOG(ERROR) << "Initialize(): decoder thread failed to start";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+
+  SetDecoderState(kInitialized);
+
+  child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+      &Client::NotifyInitializeDone, client_));
+  return true;
+}
+
+void ExynosVideoDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer) {
+  DVLOG(1) << "Decode(): input_id=" << bitstream_buffer.id()
+           << ", size=" << bitstream_buffer.size();
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+
+  scoped_ptr<BitstreamBufferRef> bitstream_record(new BitstreamBufferRef(
+      client_, child_message_loop_proxy_,
+      new base::SharedMemory(bitstream_buffer.handle(), true),
+      bitstream_buffer.size(), bitstream_buffer.id()));
+  if (!bitstream_record->shm->Map(bitstream_buffer.size())) {
+    DLOG(ERROR) << "Decode(): could not map bitstream_buffer";
+    NOTIFY_ERROR(UNREADABLE_INPUT);
+    return;
+  }
+  DVLOG(3) << "Decode(): mapped to addr=" << bitstream_record->shm->memory();
+
+  // DecodeTask() will take care of running a DecodeBufferTask().
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::DecodeTask, base::Unretained(this),
+      base::Passed(&bitstream_record)));
+}
+
+void ExynosVideoDecodeAccelerator::AssignPictureBuffers(
+    const std::vector<media::PictureBuffer>& buffers) {
+  DVLOG(3) << "AssignPictureBuffers(): buffer_count=" << buffers.size();
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+
+  if (!make_context_current_.Run()) {
+    DLOG(ERROR) << "AssignPictureBuffers(): could not make context current";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return;
+  }
+
+  DCHECK_EQ(gsc_output_buffer_count_, static_cast<int>(buffers.size()));

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

This should be stronger than a DCHECK - you don't want an untrusted renderer
messing up the GPU process in the wild.
if != then NOTIFY_ERROR (and DLOG(FATAL) if you like).

[sheu, 2013/01/05 00:51:49]

Good point.  Tightening up ReusePictureBufferTask() as well while I'm at it.

+  scoped_ptr<PictureBufferArrayRef> pic_buffers_ref(
+      new PictureBufferArrayRef(
+          egl_display_, new EGLImageKHR[buffers.size()],
+          new int[buffers.size()], new int32[buffers.size()], buffers.size()));
+  for (int i = 0; i < pic_buffers_ref->egl_images_count; ++i) {
+    pic_buffers_ref->egl_images[i] = EGL_NO_IMAGE_KHR;
+    pic_buffers_ref->egl_image_fds[i] = -1;
+    pic_buffers_ref->client_ids[i] = -1;
+  }
+
+  const static EGLint kImageAttrs[] = {
+    EGL_IMAGE_PRESERVED_KHR, 0,
+    EGL_NONE,
+  };
+  Display* x_display = base::MessagePumpForUI::GetDefaultXDisplay();
+  glActiveTexture(GL_TEXTURE0);
+  for (int i = 0; i < pic_buffers_ref->egl_images_count; ++i) {
+    // Create the X pixmap and then create an EGLImageKHR from it, so we can
+    // get dma_buf backing.
+    Pixmap pixmap = XCreatePixmap(x_display, RootWindow(x_display, 0),

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

I still think you can use Gles2TextureToEglImageTranslator.
Previously you said you didn't want to b/c the translator held on to pixmaps too
long, possibly b/c of tegra considerations, but the pixmap-using path of the
translator was only added for exynos' benefit (tegra doesn't need pixmaps
allocated explicitly).

[sheu, 2013/01/05 00:51:49]

It adds a std::map<> and some other stuff we don't need, even as we're not using
it.  I'd kinda prefer to kill that dead too, and (maybe) nuke OMX VDA as well.

If this becomes a common pattern I guess I could clean up the translator later
and add the use back in.

+        buffers[i].size().width(), buffers[i].size().height(), 32);
+    if (!pixmap) {
+      DLOG(ERROR) << "AssignPictureBuffers(): could not create X pixmap";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    glBindTexture(GL_TEXTURE_2D, buffers[i].texture_id());
+    EGLImageKHR egl_image;
+    egl_image = egl_create_image_khr(
+        egl_display_, EGL_NO_CONTEXT, EGL_NATIVE_PIXMAP_KHR,
+        (EGLClientBuffer)pixmap, kImageAttrs);
+    // We can free the X pixmap immediately -- according to the
+    // EGL_KHR_image_base spec, the backing storage does not go away until the
+    // last referencing EGLImage is destroyed.
+    XFreePixmap(x_display, pixmap);
+    if (egl_image == EGL_NO_IMAGE_KHR) {
+      DLOG(ERROR) << "AssignPictureBuffers(): could not create EGLImageKHR";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    pic_buffers_ref->egl_images[i] = egl_image;
+    int fd;
+    if (!mali_egl_image_get_buffer_ext_phandle(
+        pic_buffers_ref->egl_images[i], NULL, &fd)) {
+      DLOG(ERROR) << "AssignPictureBuffers(): "
+                  << "could not get EGLImageKHR dmabuf fd";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    pic_buffers_ref->egl_image_fds[i] = fd;
+    gl_egl_image_target_texture_2d_oes(GL_TEXTURE_2D, egl_image);
+    pic_buffers_ref->client_ids[i] = buffers[i].id();
+  }
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::AssignPictureBuffersTask,
+      base::Unretained(this), base::Passed(&pic_buffers_ref)));
+}
+
+void ExynosVideoDecodeAccelerator::ReusePictureBuffer(int32 picture_buffer_id) {
+  DVLOG(3) << "ReusePictureBuffer(): picture_buffer_id=" << picture_buffer_id;
+  // Must be run on child thread, as we'll insert a sync in the EGL context.
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+
+  if (!make_context_current_.Run()) {
+    DLOG(ERROR) << "ReusePictureBuffer(): could not make context current";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return;
+  }
+
+  EGLSyncKHR egl_sync;
+  egl_sync = egl_create_sync_khr(egl_display_, EGL_SYNC_FENCE_KHR, NULL);
+  if (egl_sync == EGL_NO_SYNC_KHR) {
+    DLOG(ERROR) << "ReusePictureBuffer(): eglCreateSyncKHR() failed";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return;
+  }
+
+  scoped_ptr<EGLSyncKHRRef> egl_sync_ref(new EGLSyncKHRRef(
+      egl_display_, egl_sync));
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::ReusePictureBufferTask,
+      base::Unretained(this), picture_buffer_id, base::Passed(&egl_sync_ref)));
+}
+
+void ExynosVideoDecodeAccelerator::Flush() {
+  DVLOG(3) << "Flush()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::FlushTask, base::Unretained(this)));
+}
+
+void ExynosVideoDecodeAccelerator::Reset() {
+  DVLOG(3) << "Reset()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::ResetTask, base::Unretained(this)));
+}
+
+void ExynosVideoDecodeAccelerator::Destroy() {
+  DVLOG(3) << "Destroy()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+
+  // We're destroying; cancel all callbacks.
+  client_ptr_factory_.InvalidateWeakPtrs();
+
+  // If the decoder thread is running, destroy using posted task.
+  if (decoder_thread_.message_loop() != NULL) {
+    decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+        &ExynosVideoDecodeAccelerator::DestroyTask, base::Unretained(this)));
+    // DestroyTask() will cause the decoder_thread_ to flush all tasks.
+    decoder_thread_.Stop();
+  } else {
+    // Otherwise, call the destroy task directly.

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

How can this happen?

[sheu, 2013/01/05 00:51:49]

If Initialize() fails.

+    DestroyTask();
+  }
+
+  // Set to kError state just in case.
+  SetDecoderState(kError);
+
+  delete this;
+}
+

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

PS#18 reviewed down to here.

+// static
+void ExynosVideoDecodeAccelerator::PreSandboxInitialization() {
+  DVLOG(3) << "PreSandboxInitialization()";
+  dlerror();

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Is this standard operating procedure in some codebases?
Seems icky to me to be silently dropping previous dlerrors...

(ditto l.546 below)

[sheu, 2013/01/05 00:51:49]

If previous dlerror() calls aren't being checked, this isn't going to help them
anyways.  I'm doing it here to clear dlerror() to a known state, since dlsym()
potentially returns NULL even on success (if the symbol requested is NULL), and
in that case whether the following call to dlerror() returns NULL or not does
have meaning.

Check the manpage, I guess.

+  libmali_handle = dlopen(EXYNOS_MALI_DRIVER, RTLD_LAZY | RTLD_LOCAL);
+  if (libmali_handle == NULL) {
+    DPLOG(ERROR) << "failed to dlopen() " << EXYNOS_MALI_DRIVER
+                 << ": " << dlerror();
+  }
+}
+
+// static
+bool ExynosVideoDecodeAccelerator::PostSandboxInitialization() {
+  DVLOG(3) << "PostSandboxInitialization()";
+  if (libmali_handle == NULL) {
+    DLOG(ERROR) << "PostSandboxInitialization(): no " << EXYNOS_MALI_DRIVER
+                << " driver handle";
+    return false;
+  }
+
+  dlerror();
+  mali_egl_image_get_buffer_ext_phandle =
+      reinterpret_cast<EGLBoolean(*)(EGLImageKHR, EGLint*, void*)>(
+          dlsym(libmali_handle, "mali_egl_image_get_buffer_ext_phandle"));
+  if (mali_egl_image_get_buffer_ext_phandle == NULL) {
+    DPLOG(ERROR) << "PostSandboxInitialization(): failed to dlsym()"
+                 << " mali_egl_image_get_buffer_ext_phandle: " << dlerror();
+    return false;
+  }
+
+  egl_create_image_khr =
+      reinterpret_cast<EGLImageKHR(*)(EGLDisplay, EGLContext, EGLenum,
+                                      EGLClientBuffer, const EGLint*)>(
+          dlsym(libmali_handle, "eglCreateImageKHR"));
+  if (egl_create_image_khr == NULL) {
+    DPLOG(ERROR) << "PostSandBoxInitialization(): failed to dlsym()"
+                 << " eglCreateImageKHR: " << dlerror();
+    return false;
+  }
+
+  egl_destroy_image_khr =
+      reinterpret_cast<EGLBoolean(*)(EGLDisplay, EGLImageKHR)>(
+          dlsym(libmali_handle, "eglDestroyImageKHR"));
+  if (egl_destroy_image_khr == NULL) {
+    DPLOG(ERROR) << "PostSandBoxInitialization(): failed to dlsym()"
+                 << " eglDestroyImageKHR: " << dlerror();
+    return false;
+  }
+
+  egl_create_sync_khr =
+      reinterpret_cast<EGLSyncKHR(*)(EGLDisplay, EGLenum, const EGLint*)>(
+          dlsym(libmali_handle, "eglCreateSyncKHR"));
+  if (egl_create_sync_khr == NULL) {
+    DPLOG(ERROR) << "PostSandboxInitialization(): failed to dlsym()"
+                 << " eglCreateSyncKHR: " << dlerror();
+    return false;
+  }
+
+  egl_destroy_sync_khr =
+      reinterpret_cast<EGLBoolean(*)(EGLDisplay, EGLSyncKHR)>(
+          dlsym(libmali_handle, "eglDestroySyncKHR"));
+  if (egl_destroy_sync_khr == NULL) {
+    DPLOG(ERROR) << "PostSandboxInitialization(): failed to dlsym()"
+                 << " eglDestroySyncKHR: " << dlerror();
+    return false;
+  }
+
+  egl_client_wait_sync_khr =
+      reinterpret_cast<EGLint(*)(EGLDisplay, EGLSyncKHR, EGLint, EGLTimeKHR)>(
+          dlsym(libmali_handle, "eglClientWaitSyncKHR"));
+  if (egl_client_wait_sync_khr == NULL) {
+    DPLOG(ERROR) << "PostSandboxInitialization(): failed to dlsym()"
+                 << " eglClientWaitSyncKHR: " << dlerror();
+    return false;
+  }
+
+  gl_egl_image_target_texture_2d_oes =
+      reinterpret_cast<void(*)(GLenum, GLeglImageOES)>(
+          dlsym(libmali_handle, "glEGLImageTargetTexture2DOES"));
+  if (gl_egl_image_target_texture_2d_oes == NULL) {
+    DPLOG(ERROR) << "PostSandboxInitialization(): failed to dlsym()"
+                 << "glEGLImageTargetTexture2DOES: " << dlerror();
+    return false;
+  }

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

l.547-609 is pretty boilerplateiriffic, making it harder than necessary to see
what's going on.

If you typedef the signatures at the global declarations then you get much
shorter reinterpret_cast<> expressions, at little or no cost of vertical space
at the declarations (which get to use the typedefs).

Devoting 5 lines to each error-handling clause seems crazy given the only value
is the name of the failing symbol, but this will only be emitted in Debug (and
only be interesting when dropping a new libmali.so).  Replace with a single
if (!mali_egl_image_get_buffer_ext_phandle || !egl_create_image_khr || ...)
?

If you really want to keep the name of the missing symbol and the dlerror(),
maybe use a macro to reduce copy/pasta?

[sheu, 2013/01/05 00:51:49]

Typedef-ed and macro-ed.

+
+  return true;
+}
+
+void ExynosVideoDecodeAccelerator::DecodeTask(
+    scoped_ptr<BitstreamBufferRef> bitstream_record) {
+  DVLOG(3) << "DecodeTask(): input_id=" << bitstream_record->input_id;
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  TRACE_EVENT1("Video Decoder", "EVDA::DecodeTask", "input_id",
+               bitstream_record->input_id);
+
+  if (decoder_state_ == kResetting) {
+    DVLOG(2) << "DecodeTask(): early out: kResetting state";

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

I think this is wrong.
A client should be able to call Reset() followed immediately by Decode(), before
NotifyResetDone is triggered, to mean "drop decoder state and then decode this
other buffer".
Search OmxVideoDecodeAccelerator::Decode() for RESETTING to see what is done
there.

I'm surprised vda_test doesn't test for this; want to add that?

[sheu, 2013/01/05 00:51:49]

Alrighty then.  Good catch!  Updated, and associated commentary added.

vda_test tests for this somewhat indirectly in the MidStreamReset tests by
counting frames, but it's a bit timing-dependent (if the Reset() and associated
tasks complete quickly, before the next Decode() comes in) whether vda_test will
catch this.

+    return;
+  } else if (decoder_state_ == kError) {
+    DVLOG(2) << "DecodeTask(): early out: kError state";
+    return;
+  }
+
+  decoder_input_queue_.push_front(

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

I think this is correct, but surprising.
Why push_front and pop_back instead of push_back and pop_front?

[sheu, 2013/01/05 00:51:49]

Not sure what the convention was.  Done.

+      linked_ptr<BitstreamBufferRef>(bitstream_record.release()));
+  decoder_decode_buffer_tasks_scheduled_++;
+  DecodeBufferTask();
+}
+
+void ExynosVideoDecodeAccelerator::DecodeBufferTask() {
+  DVLOG(3) << "DecodeBufferTask()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  TRACE_EVENT0("Video Decoder", "EVDA::DecodeBufferTask");
+
+  decoder_decode_buffer_tasks_scheduled_--;
+
+  if (decoder_state_ == kResetting) {
+    DVLOG(2) << "DecodeBufferTask(): early out: kResetting state";
+    return;
+  } else if (decoder_state_ == kError) {
+    DVLOG(2) << "DecodeBufferTask(): early out: kError state";
+    return;
+  }
+
+  if (decoder_current_bitstream_buffer_ == NULL) {
+    if (decoder_input_queue_.empty()) {
+      // We're waiting for a new buffer -- exit without scheduling a new task.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Can this really happen?

[sheu, 2013/01/05 00:51:49]

I don't think it can happen, but it's not an error if it does.

+      return;
+    }
+    // Setup to use the next buffer.
+    decoder_current_bitstream_buffer_.reset(
+        decoder_input_queue_.back().release());
+    decoder_input_queue_.pop_back();
+    DVLOG(3) << "DecodeBufferTask(): reading input_id="
+             << decoder_current_bitstream_buffer_->input_id
+             << ", addr=" << decoder_current_bitstream_buffer_->shm->memory()
+             << ", size=" << decoder_current_bitstream_buffer_->size;
+  }
+  bool schedule_task = false;
+  const size_t size = decoder_current_bitstream_buffer_->size;
+  size_t decoded_size;
+  if (size == 0) {
+    const int32 input_id = decoder_current_bitstream_buffer_->input_id;
+    decoded_size = 0;
+    if (input_id != -1) {
+      // This is a buffer queued from the client that has zero size.  Skip.
+      schedule_task = true;
+    } else {
+      // This is a buffer of zero size, queued to flush the pipe.  Flush.
+      DCHECK_EQ(decoder_current_bitstream_buffer_->shm.get(),
+                static_cast<base::SharedMemory*>(NULL));
+      // Flush the current input, enqueue an empty buffer, then flush that down.
+      if (!FlushInputFrame() || !AppendToInputFrame(NULL, 0)

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

These methods are confusing because unlike the rest of the bool-returning
private methods in this class, returning false from them is *not* a cause for
NotifyError.  Please doco return values.

[sheu, 2013/01/05 00:51:49]

Done.

+          || !FlushInputFrame()) {

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

|| goes on previous line

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

What does short-circuiting mean here?
IOW, if one of the !'s evals true and the other one or two is not eval'd.

[sheu, 2013/01/05 00:51:49]

Done.

[sheu, 2013/01/05 00:51:49]

If we can't enqueue a completely empty frame and guarantee that it's flushed
down (so nobody else tries to append to it), then try again later.

+        schedule_task = false;
+      } else {
+        DVLOG(2) << "DecodeBufferTask(): enqueued flush buffer";
+        schedule_task = true;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

FWIW this if/else is equiv to:

schedule_task = FlushInputFrame() && AppendToInputFrame(NULL, 0) &&
FlushInputFrame();

[sheu, 2013/01/05 00:51:49]

A little easier to document and comment this way.  :-)

+      }
+    }
+  } else {
+    // This is a buffer queued from the client, with actual `contents.  Decode.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

s/`//

[sheu, 2013/01/05 00:51:49]

Done.

+    const void* const data =
+        reinterpret_cast<const uint8*>(
+            decoder_current_bitstream_buffer_->shm->memory()) +
+        decoder_current_bitstream_buffer_->bytes_used;
+    const size_t data_size =
+        decoder_current_bitstream_buffer_->size -
+        decoder_current_bitstream_buffer_->bytes_used;
+    if (!FindFrameFragment(data, data_size, &decoded_size))
+      return;
+    switch (decoder_state_) {
+      case kInitialized:
+      case kAfterReset:
+        schedule_task = DecodeBufferInitial(data, decoded_size, &decoded_size);
+        break;
+      case kDecoding:
+        schedule_task = DecodeBufferContinue(data, decoded_size, &decoded_size);
+        break;
+      default:
+        NOTIFY_ERROR(ILLEGAL_STATE);
+        return;
+    }
+  }
+  if (decoder_state_ == kError) {
+    // Failed during decode.
+    return;
+  }
+
+  if (schedule_task) {
+    decoder_current_bitstream_buffer_->bytes_used += decoded_size;
+    if (decoder_current_bitstream_buffer_->bytes_used ==
+        decoder_current_bitstream_buffer_->size) {
+      // Our current bitstream buffer is done; return it.
+      int32 input_id = decoder_current_bitstream_buffer_->input_id;
+      DVLOG(3) << "DecodeBufferTask(): finished input_id=" << input_id;
+      // BitstreamBufferRef destructor calls NotifyEndOfBitstreamBuffer().
+      decoder_current_bitstream_buffer_.reset();
+    }
+    ScheduleDecodeBufferTaskIfNeeded();
+  }
+}
+
+bool ExynosVideoDecodeAccelerator::FindFrameFragment(
+    const void* data,
+    size_t size,
+    size_t* endpos) {
+  if (video_profile_ >= media::H264PROFILE_MIN &&
+      video_profile_ <= media::H264PROFILE_MAX) {
+    // For H264, we need to feed HW one frame at a time.  This is going to take
+    // some parsing of our input stream.
+    decoder_h264_parser_->SetStream(reinterpret_cast<const uint8*>(data), size);

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

You can save the cast here and twice more in this method by passing data as a
const uint8* in the first place, no?

[sheu, 2013/01/05 00:51:49]

Done.

+    content::H264NALU nalu;
+    content::H264Parser::Result result;
+
+    // Find the first NAL.
+    result = decoder_h264_parser_->AdvanceToNextNALU(&nalu);
+    if (result == content::H264Parser::kInvalidStream ||
+        result == content::H264Parser::kUnsupportedStream)
+      return false;
+    *endpos = (nalu.data + nalu.size) - reinterpret_cast<const uint8*>(data);
+    if (result == content::H264Parser::kEOStream)
+      return true;
+
+    // Keep on peeking the next NALs while they don't indicate a frame
+    // boundary.
+    for (;;) {
+      result = decoder_h264_parser_->AdvanceToNextNALU(&nalu);
+      if (result == content::H264Parser::kInvalidStream ||
+          result == content::H264Parser::kUnsupportedStream)
+        return false;
+      if (result == content::H264Parser::kEOStream)
+        return true;
+      switch (nalu.nal_unit_type) {
+        case content::H264NALU::kNonIDRSlice:
+        case content::H264NALU::kIDRSlice:
+          // For these two, if the "first_mb_in_slice" field is zero, start a
+          // new frame and return.  This field is Exp-Golomb coded starting on
+          // the eighth data bit of the NAL; a zero value is encoded with a
+          // leading '1' bit in the byte, which we can detect as the byte being
+          // (unsigned) greater than or equal to 0x80.
+          if (nalu.data[1] >= 0x80)
+            return true;
+          break;
+        case content::H264NALU::kSPS:
+        case content::H264NALU::kPPS:
+        case content::H264NALU::kEOSeq:
+        case content::H264NALU::kEOStream:
+          // These unconditionally signal a frame boundary.
+          return true;
+        default:
+          // For all others, keep going.
+          break;
+      }
+      *endpos = (nalu.data + nalu.size) - reinterpret_cast<const uint8*>(data);
+    }
+    NOTREACHED();
+    return false;
+  } else {

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

You could save a +2 indent for l.736-785 by re-organizing this method as:
if (VP8) {
  ...
}
if (!H264) {
  DLOG(FATAL) << "lolwat: " << video_profile_)
  return false;
}
...h264 case, de-indented...

[sheu, 2013/01/05 00:51:49]

I like symmetry, which leads me to excessive use of "else" cases sometimes.  For
situations like this (essentially a "switch"), I'd rather that all cases get
indented the same.

+    DCHECK_GE(video_profile_, media::VP8PROFILE_MIN);
+    DCHECK_LE(video_profile_, media::VP8PROFILE_MAX);
+    // For VP8, we can just dump the entire buffer.  No fragmentation needed.
+    *endpos = size;
+    return true;
+  }
+}
+
+void ExynosVideoDecodeAccelerator::ScheduleDecodeBufferTaskIfNeeded() {

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

DCHECK on decoder_thread_?

[sheu, 2013/01/05 00:51:49]

Done.

+  // If we're behind on tasks, schedule another one.
+  int buffers_to_decode = decoder_input_queue_.size();
+  if (decoder_current_bitstream_buffer_ != NULL)
+    buffers_to_decode++;
+  if (decoder_decode_buffer_tasks_scheduled_ < buffers_to_decode) {
+    decoder_decode_buffer_tasks_scheduled_++;
+    decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+        &ExynosVideoDecodeAccelerator::DecodeBufferTask,
+        base::Unretained(this)));
+  }
+}
+
+bool ExynosVideoDecodeAccelerator::DecodeBufferInitial(
+    const void* data, size_t size, size_t* endpos) {
+  DVLOG(3) << "DecodeBufferInitial(): data=" << data << ", size=" << size;
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kDecoding);
+  DCHECK(!device_poll_thread_.IsRunning());
+  // Initial decode.  We haven't been able to get output stream format info yet.
+  // Get it, and start decoding.
+
+  // Copy in and send to HW.
+  if (!AppendToInputFrame(data, size) || !FlushInputFrame())
+    return false;
+
+  // Recycle buffers.
+  DequeueMfc();
+
+  // Check and see if we have format info yet.
+  struct v4l2_format format;
+  format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  if (ioctl(mfc_fd_, VIDIOC_G_FMT, &format) != 0) {
+    if (errno == EINVAL) {
+      // We will get EINVAL if we haven't seen sufficient stream to decode the
+      // format.  Return true and schedule the next buffer.
+      *endpos = size;
+      return true;
+    } else {
+      DPLOG(ERROR) << "DecodeBufferInitial(): ioctl() failed: VIDIOC_G_FMT";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return false;
+    }
+  }
+
+  // Run this initialization only on first startup.
+  if (decoder_state_ == kInitialized) {
+    DVLOG(3) << "DecodeBufferInitial(): running one-time initialization";
+    // Success! Setup our parameters.
+    CHECK_EQ(format.fmt.pix_mp.num_planes, 2);
+    // We don't handle midstream resizes right now.
+    if (!frame_buffer_size_.IsEmpty() && frame_buffer_size_ !=

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

How can we be mid-stream if decoder_state_==kInitialized?

[sheu, 2013/01/05 00:51:49]

This used to be on the outside of this conditional.  Done.

+        gfx::Size(format.fmt.pix_mp.width, frame_buffer_size_.height())) {
+      // We don't handle midstream resizes right now.
+      NOTIMPLEMENTED();
+      NOTIFY_ERROR(UNREADABLE_INPUT);
+      return false;
+    }
+    frame_buffer_size_.SetSize(
+        format.fmt.pix_mp.width, format.fmt.pix_mp.height);
+    mfc_output_buffer_size_[0] = format.fmt.pix_mp.plane_fmt[0].sizeimage;
+    mfc_output_buffer_size_[1] = format.fmt.pix_mp.plane_fmt[1].sizeimage;
+    mfc_output_buffer_pixelformat_ = format.fmt.pix_mp.pixelformat;
+
+    // Create our other buffers.
+    if (!CreateMfcOutputBuffers() || !CreateGscInputBuffers() ||
+        !CreateGscOutputBuffers())
+      return false;
+
+    // MFC expects to process the initial buffer once during stream init to
+    // configure stream parameters, but will not consume the steam data on that
+    // iteration.  Subsequent iterations (including after reset) do not require
+    // the stream init step.
+    *endpos = 0;
+  } else {
+    *endpos = size;
+  }
+
+  // StartDevicePoll will raise the error if there is one.
+  if (!StartDevicePoll())
+    return false;
+
+  decoder_state_ = kDecoding;
+  ScheduleDecodeBufferTaskIfNeeded();
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::DecodeBufferContinue(
+    const void* data, size_t size, size_t* endpos) {
+  DVLOG(3) << "DecodeBufferContinue(): data=" << data << ", size=" << size;
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_EQ(decoder_state_, kDecoding);
+
+  // We've already setup our output stream parameters, so just keep on truckin'.
+  *endpos = size;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

lol to always setting this output param to an input param (meaning the caller
could do the same) and double-lol to the input & output params being the same :)
Drop?

[sheu, 2013/01/05 00:51:49]

Symmetry with DecodeBufferInitial()?  That's kinda my thing.

+  // Both of these calls will set kError state if they fail.
+  return (AppendToInputFrame(data, size) && FlushInputFrame());
+}
+
+bool ExynosVideoDecodeAccelerator::AppendToInputFrame(
+    const void* data, size_t size) {
+  DVLOG(3) << "AppendToInputFrame()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kResetting);
+  DCHECK_NE(decoder_state_, kError);
+  // This routine can handle data == NULL and size == 0, which occurs when
+  // we queue an empty buffer for the purposes of flushing the pipe.
+
+  // Flush if we're too big

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

comment belongs at l.910?

[sheu, 2013/01/05 00:51:49]

It's the purpose of the whole block here, really.

+  if (decoder_current_input_buffer_ != -1) {
+    MfcInputRecord& input_record =
+        mfc_input_buffer_map_[decoder_current_input_buffer_];
+    if (input_record.bytes_used + size > input_record.length) {
+      if (!FlushInputFrame())
+        return false;
+      decoder_current_input_buffer_ = -1;
+    }
+  }
+
+  // Try to get an available input buffer
+  if (decoder_current_input_buffer_ == -1) {
+    if (mfc_free_input_buffers_.empty()) {
+      // See if we can get more free buffers from HW
+      DequeueMfc();
+      if (mfc_free_input_buffers_.empty()) {
+        // Nope!
+        DVLOG(2) << "AppendToInputFrame(): stalled for input buffers";
+        return false;
+      }
+    }
+    decoder_current_input_buffer_ = mfc_free_input_buffers_.back();
+    mfc_free_input_buffers_.pop_back();
+    MfcInputRecord& input_record =
+        mfc_input_buffer_map_[decoder_current_input_buffer_];
+    DCHECK_EQ(input_record.bytes_used, 0);
+    DCHECK_EQ(input_record.input_id, -1);
+    DCHECK(decoder_current_bitstream_buffer_ != NULL);
+    input_record.input_id = decoder_current_bitstream_buffer_->input_id;
+  }
+
+  if (size == 0) {
+    // If we asked for an empty buffer, return now.  We return only after
+    // getting the next input buffer, since we might actually want an empty
+    // input buffer for flushing purposes.
+    return true;
+  }
+  DCHECK(data != NULL);

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

WDYT of 
DCHECK_EQ(data == NULL, size == 0);
going at l.936 above?

[sheu, 2013/01/05 00:51:49]

Done.

+
+  // Copy in to the buffer.
+  MfcInputRecord& input_record =
+      mfc_input_buffer_map_[decoder_current_input_buffer_];
+  if (size > input_record.length - input_record.bytes_used) {
+    LOG(ERROR) << "AppendToInputFrame(): over-size frame, erroring";
+    NOTIFY_ERROR(UNREADABLE_INPUT);
+    return false;
+  }
+  memcpy((char*)input_record.address + input_record.bytes_used, data, size);
+  input_record.bytes_used += size;
+
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::FlushInputFrame() {
+  DVLOG(3) << "FlushInputFrame()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kResetting);
+  DCHECK_NE(decoder_state_, kError);
+
+  if (decoder_current_input_buffer_ == -1)
+    return true;
+
+  MfcInputRecord& input_record =
+      mfc_input_buffer_map_[decoder_current_input_buffer_];
+  // An empty buffer with input_id == -1 is a "flush buffer" that we don't want
+  // to skip.
+  if (input_record.bytes_used == 0 && input_record.input_id != -1) {
+    input_record.input_id = -1;
+    mfc_free_input_buffers_.push_back(decoder_current_input_buffer_);
+    decoder_current_input_buffer_ = -1;
+    return true;
+  }
+
+  // Queue it to MFC.
+  mfc_input_ready_queue_.push_back(decoder_current_input_buffer_);
+  decoder_current_input_buffer_ = -1;
+  DVLOG(3) << "FlushInputFrame(): submitting input_id="
+           << input_record.input_id;
+  // Kick the MFC once since there's new available input for it.
+  EnqueueMfc();
+
+  return (decoder_state_ != kError);
+}
+
+void ExynosVideoDecodeAccelerator::AssignPictureBuffersTask(
+    scoped_ptr<PictureBufferArrayRef> pic_buffers) {
+  DVLOG(3) << "AssignPictureBuffersTask()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  TRACE_EVENT0("Video Decoder", "EVDA::AssignPictureBuffersTask");
+
+  // We run AssignPictureBuffersTask even if we're in kResetting.
+  if (decoder_state_ == kError) {
+    DVLOG(2) << "AssignPictureBuffersTask(): early out: kError state";
+    return;
+  }
+
+  DCHECK_EQ(pic_buffers->egl_images_count,
+      static_cast<int>(gsc_output_buffer_map_.size()));

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

indent to opening paren (don't mix +4 and opening paren styles).

[sheu, 2013/01/05 00:51:49]

Done.

+  for (size_t i = 0; i < gsc_output_buffer_map_.size(); ++i) {
+    // We should be blank right now.
+    GscOutputRecord& output_record = gsc_output_buffer_map_[i];
+    DCHECK_EQ(output_record.fd, -1);
+    DCHECK_EQ(output_record.egl_image, EGL_NO_IMAGE_KHR);
+    DCHECK_EQ(output_record.egl_sync, EGL_NO_SYNC_KHR);
+    DCHECK_EQ(output_record.picture_id, -1);
+    output_record.fd = pic_buffers->egl_image_fds[i];
+    output_record.egl_image = pic_buffers->egl_images[i];
+    output_record.picture_id = pic_buffers->client_ids[i];
+
+    // Take ownership of the EGLImage and fd.
+    pic_buffers->egl_images[i] = EGL_NO_IMAGE_KHR;
+    pic_buffers->egl_image_fds[i] = -1;
+    // And add this buffer to the free list.
+    gsc_free_output_buffers_.push_front(i);
+  }
+
+  // We got buffers!  Kick the GSC.
+  EnqueueGsc();
+}
+
+void ExynosVideoDecodeAccelerator::ServiceDeviceTask() {
+  DVLOG(3) << "ServiceDeviceTask()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kInitialized);
+  DCHECK_NE(decoder_state_, kAfterReset);
+  TRACE_EVENT0("Video Decoder", "EVDA::ServiceDeviceTask");
+
+  if (decoder_state_ == kResetting) {
+    DVLOG(2) << "ServiceDeviceTask(): early out: kResetting state";
+    return;
+  } else if (decoder_state_ == kError) {
+    DVLOG(2) << "ServiceDeviceTask(): early out: kError state";
+    return;
+  }
+
+  DequeueMfc();
+  DequeueGsc();
+  EnqueueMfc();
+  EnqueueGsc();
+
+  // Clear the interrupt fd.
+  if (!ClearDevicePollInterrupt())
+    return;
+
+  unsigned int poll_fds = 0;
+  // Add MFC fd, if we should poll on it.
+  // MFC can be polled as soon as either input or output buffers are queued.
+  if (mfc_input_buffer_queued_count_ + mfc_output_buffer_queued_count_ > 0)
+    poll_fds |= kPollMfc;
+  // Add GSC fd, if we should poll on it.
+  // GSC has to wait until both input and output buffers are queued.
+  if (gsc_input_buffer_queued_count_ > 0 && gsc_output_buffer_queued_count_ > 0)
+    poll_fds |= kPollGsc;
+
+  // ServiceDeviceTask() should only ever be scheduled from DevicePollTask(),
+  // so either:
+  // * device_poll_thread_ is running normally
+  // * device_poll_thread_ scheduled us, but then a ResetTask() or DestroyTask()
+  //   shut it down, in which case we're either in kResetting or kError states
+  //   respectively, and we should have early-outed already.
+  DCHECK(device_poll_thread_.message_loop());
+  // Queue the DevicePollTask() now.
+  device_poll_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::DevicePollTask,
+      base::Unretained(this),
+      poll_fds));
+
+  DVLOG(1) << "ServiceDeviceTask(): buffer counts: DEC["
+           << decoder_input_queue_.size() << "->"
+           << mfc_input_ready_queue_.size() << "] => MFC["
+           << mfc_free_input_buffers_.size() << "+"
+           << mfc_input_buffer_queued_count_ << "/"
+           << mfc_input_buffer_count_ << "->"
+           << mfc_free_output_buffers_.size() << "+"
+           << mfc_output_buffer_queued_count_ << "/"
+           << mfc_output_buffer_count_ << "] => "
+           << mfc_output_gsc_input_queue_.size() << " => GSC["
+           << gsc_free_input_buffers_.size() << "+"
+           << gsc_input_buffer_queued_count_ << "/"
+           << gsc_input_buffer_count_ << "->"
+           << gsc_free_output_buffers_.size() << "+"
+           << gsc_output_buffer_queued_count_ << "/"
+           << gsc_output_buffer_count_ << "] => VDA["
+           << decoder_frames_at_client_ << "]";
+
+  ScheduleDecodeBufferTaskIfNeeded();
+}
+
+void ExynosVideoDecodeAccelerator::EnqueueMfc() {
+  DVLOG(3) << "EnqueueMfc()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  TRACE_EVENT0("Video Decoder", "EVDA::EnqueueMfc");
+
+  // Drain the pipe of completed decode buffers.
+  const int old_mfc_inputs_queued_ = mfc_input_buffer_queued_count_;
+  while (!mfc_input_ready_queue_.empty()) {
+    if (!EnqueueMfcInputRecord())
+      return;
+  }
+  if (old_mfc_inputs_queued_ == 0 && mfc_input_buffer_queued_count_ != 0) {
+    // We just started up a previously empty queue.
+    // Queue state changed; signal interrupt.
+    if (!SetDevicePollInterrupt())
+      return;
+    // Start VIDIOC_STREAMON if we haven't yet.
+    if (!mfc_input_streamon_) {
+      __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+      IOCTL_OR_ERROR_RETURN(mfc_fd_, VIDIOC_STREAMON, &type);
+      mfc_input_streamon_ = true;
+    }
+  }
+
+  // Enqueue all the MFC outputs we can.
+  const int old_mfc_outputs_queued_ = mfc_output_buffer_queued_count_;
+  while (!mfc_free_output_buffers_.empty()) {
+    if (!EnqueueMfcOutputRecord())
+      return;
+  }
+  if (old_mfc_outputs_queued_ == 0 && mfc_output_buffer_queued_count_ != 0) {
+    // We just started up a previously empty queue.
+    // Queue state changed; signal interrupt.
+    if (!SetDevicePollInterrupt())
+      return;
+    // Start VIDIOC_STREAMON if we haven't yet.
+    if (!mfc_output_streamon_) {
+      __u32 type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+      IOCTL_OR_ERROR_RETURN(mfc_fd_, VIDIOC_STREAMON, &type);
+      mfc_output_streamon_ = true;
+    }
+  }
+}
+
+void ExynosVideoDecodeAccelerator::DequeueMfc() {
+  DVLOG(3) << "DequeueMfc()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  TRACE_EVENT0("Video Decoder", "EVDA::DequeueMfc");
+
+  // Dequeue completed MFC input (VIDEO_OUTPUT) buffers, and recycle to the free
+  // list.
+  struct v4l2_buffer dqbuf;
+  struct v4l2_plane planes[2];
+  while (mfc_input_buffer_queued_count_ > 0) {
+    DCHECK(mfc_input_streamon_);

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

move to DCHECK pile at top of function?

[sheu, 2013/01/05 00:51:49]

In case we get called before STREAMON occurs.  I'd like the Dequeue{Mfc,Gsc}()
functions to be callable regardless of the states of the queues.

+    memset(&dqbuf, 0, sizeof(dqbuf));
+    dqbuf.type   = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+    dqbuf.memory = V4L2_MEMORY_MMAP;
+    if (ioctl(mfc_fd_, VIDIOC_DQBUF, &dqbuf) != 0) {
+      if (errno == EAGAIN) {
+        // EAGAIN if we're just out of buffers to dequeue.
+        break;
+      }
+      DPLOG(ERROR) << "DequeueMfc(): ioctl() failed: VIDIOC_DQBUF";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    MfcInputRecord& input_record = mfc_input_buffer_map_[dqbuf.index];
+    DCHECK(input_record.at_device);
+    mfc_free_input_buffers_.push_back(dqbuf.index);
+    input_record.at_device = false;
+    input_record.bytes_used = 0;
+    input_record.input_id = -1;
+    mfc_input_buffer_queued_count_--;
+  }
+
+  // Dequeue completed MFC output (VIDEO_CAPTURE) buffers, and queue to the
+  // completed queue.
+  while (mfc_output_buffer_queued_count_ > 0) {
+    DCHECK(mfc_output_streamon_);
+    memset(&dqbuf, 0, sizeof(dqbuf));
+    memset(planes, 0, sizeof(planes));
+    dqbuf.type   = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+    dqbuf.memory = V4L2_MEMORY_MMAP;
+    dqbuf.m.planes = planes;
+    dqbuf.length = 2;
+    if (ioctl(mfc_fd_, VIDIOC_DQBUF, &dqbuf) != 0) {
+      if (errno == EAGAIN) {
+        // EAGAIN if we're just out of buffers to dequeue.
+        break;
+      }
+      DPLOG(ERROR) << "DequeueMfc(): ioctl() failed: VIDIOC_DQBUF";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    const long int input_id = dqbuf.timestamp.tv_sec;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

why "long int" and not "int"?

[sheu, 2013/01/05 00:51:49]

tv_sec is a long int, and I'm caching that value really.  The conversion happens
when this value gets assigned to something else.

+    MfcOutputRecord& output_record = mfc_output_buffer_map_[dqbuf.index];
+    DCHECK(output_record.at_device);
+    if (dqbuf.m.planes[0].bytesused + dqbuf.m.planes[1].bytesused == 0) {
+      // This is an empty output buffer returned as part of a flush.
+      mfc_free_output_buffers_.push_back(dqbuf.index);
+      output_record.at_device = false;
+      output_record.input_id = -1;
+      output_record.bytes_used[0] = 0;
+      output_record.bytes_used[1] = 0;
+    } else {
+      // This is an output buffer with contents to pass down the pipe.
+      mfc_output_gsc_input_queue_.push_front(dqbuf.index);
+      output_record.at_device = false;
+      output_record.input_id = input_id;
+      output_record.bytes_used[0] = dqbuf.m.planes[0].bytesused;
+      output_record.bytes_used[1] = dqbuf.m.planes[1].bytesused;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

AFAICT l.1207-1210 can be moved outside the if/else and the corresponding 4
lines in  the "then" clause can be dropped (greatly clarifying the difference
between the flush and non-flush cases, which is whether the output_record is
added to gsc_input or mfc_free).

+      DVLOG(3) << "DequeueMfc(): dequeued input_id=" << input_id;
+      // We don't count this output buffer dequeued yet, or add it to the free
+      // list, as it has data GSC needs to process.
+
+      // We have new frames in mfc_output_gsc_input_queue_.  Kick the pipe.
+      SetDevicePollInterrupt();
+    }
+    mfc_output_buffer_queued_count_--;
+  }
+
+  NotifyFlushDoneIfNeeded();

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Move to l.1203?

[sheu, 2013/01/05 00:51:49]

I figure I'll do it here once all state is resolved, in case the logic for
flush-done changes.

+}
+
+void ExynosVideoDecodeAccelerator::EnqueueGsc() {
+  DVLOG(3) << "EnqueueGsc()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kInitialized);
+  TRACE_EVENT0("Video Decoder", "EVDA::EnqueueGsc");
+
+  // Drain the pipe of completed MFC output buffers.
+  const int old_gsc_inputs_queued_ = gsc_input_buffer_queued_count_;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

s/old_gsc_inputs_queued_/old_gsc_inputs_queued/
same sin is committed elsewhere; search for 
int old_
and drop trailing underscores from function-local variables.

[sheu, 2013/01/05 00:51:49]

Done (and elsewhere)

+  while (!mfc_output_gsc_input_queue_.empty()) {
+    if (gsc_free_input_buffers_.empty())

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

while (foo()) {
  if (!bar())
    break;
  ...
}

is equivalent to the shorter (and, IMO, clearer):

while (foo() && bar()) {
  ...
}

+      break;
+    if (!EnqueueGscInputRecord())
+      return;
+  }
+  if (old_gsc_inputs_queued_ == 0 && gsc_input_buffer_queued_count_ != 0) {
+    // We just started up a previously empty queue.
+    // Queue state changed; signal interrupt.
+    if (!SetDevicePollInterrupt())
+      return;
+    // Start VIDIOC_STREAMON if we haven't yet.
+    if (!gsc_input_streamon_) {
+      __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+      IOCTL_OR_ERROR_RETURN(gsc_fd_, VIDIOC_STREAMON, &type);
+      gsc_input_streamon_ = true;
+    }
+  }
+
+  // Enqueue a GSC output, only if we need one
+  if (gsc_input_buffer_queued_count_ != 0 &&
+      gsc_output_buffer_queued_count_ == 0 &&
+      !gsc_free_output_buffers_.empty()) {
+    const int old_gsc_outputs_queued_ = gsc_output_buffer_queued_count_;
+    if (!EnqueueGscOutputRecord())
+      return;
+    if (old_gsc_outputs_queued_ == 0 && gsc_output_buffer_queued_count_ != 0) {
+      // We just started up a previously empty queue.
+      // Queue state changed; signal interrupt.
+      if (!SetDevicePollInterrupt())
+        return;
+      // Start VIDIOC_STREAMON if we haven't yet.
+      if (!gsc_output_streamon_) {
+        __u32 type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+        IOCTL_OR_ERROR_RETURN(gsc_fd_, VIDIOC_STREAMON, &type);
+        gsc_output_streamon_ = true;
+      }
+    }
+  }
+  // Bug check: GSC is liable to race conditions if more than one buffer is
+  // simultaneously queued.
+  DCHECK_GE(1, gsc_output_buffer_queued_count_);
+}
+
+void ExynosVideoDecodeAccelerator::DequeueGsc() {
+  DVLOG(3) << "DequeueGsc()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kInitialized);
+  DCHECK_NE(decoder_state_, kAfterReset);
+  TRACE_EVENT0("Video Decoder", "EVDA::DequeueGsc");
+
+  // Dequeue completed GSC input (VIDEO_OUTPUT) buffers, and recycle to the free
+  // list.  Also recycle the corresponding MFC output buffers at this time.
+  struct v4l2_buffer dqbuf;
+  while (gsc_input_buffer_queued_count_ > 0) {
+    DCHECK(gsc_input_streamon_);
+    memset(&dqbuf, 0, sizeof(dqbuf));
+    dqbuf.type   = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+    dqbuf.memory = V4L2_MEMORY_DMABUF;
+    if (ioctl(gsc_fd_, VIDIOC_DQBUF, &dqbuf) != 0) {
+      if (errno == EAGAIN) {
+        // EAGAIN if we're just out of buffers to dequeue.
+        break;
+      }
+      DPLOG(ERROR) << "DequeueGsc(): ioctl() failed: VIDIOC_DQBUF";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    GscInputRecord& input_record = gsc_input_buffer_map_[dqbuf.index];
+    MfcOutputRecord& output_record =
+        mfc_output_buffer_map_[input_record.mfc_output];
+    DCHECK(input_record.at_device);
+    gsc_free_input_buffers_.push_back(dqbuf.index);
+    mfc_free_output_buffers_.push_back(input_record.mfc_output);
+    input_record.at_device = false;
+    input_record.mfc_output = -1;
+    output_record.input_id = -1;
+    gsc_input_buffer_queued_count_--;
+  }
+
+  // Dequeue completed GSC output (VIDEO_CAPTURE) buffers, and send them off to
+  // the client.  Don't recycle to its free list yet -- we can't do that until
+  // ReusePictureBuffer() returns it to us.
+  while (gsc_output_buffer_queued_count_ > 0) {
+    DCHECK(gsc_output_streamon_);
+    memset(&dqbuf, 0, sizeof(dqbuf));
+    dqbuf.type   = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+    dqbuf.memory = V4L2_MEMORY_DMABUF;
+    if (ioctl(gsc_fd_, VIDIOC_DQBUF, &dqbuf) != 0) {
+      if (errno == EAGAIN) {
+        // EAGAIN if we're just out of buffers to dequeue.
+        break;
+      }
+      DPLOG(ERROR) << "DequeueGsc(): ioctl() failed: VIDIOC_DQBUF";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return;
+    }
+    GscOutputRecord& output_record = gsc_output_buffer_map_[dqbuf.index];
+    DCHECK(output_record.at_device);
+    DCHECK(!output_record.at_client);
+    DCHECK_EQ(output_record.egl_sync, EGL_NO_SYNC_KHR);
+    output_record.at_device = false;
+    output_record.at_client = true;
+    gsc_output_buffer_queued_count_--;
+    child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &Client::PictureReady, client_, media::Picture(
+            output_record.picture_id, dqbuf.timestamp.tv_sec)));
+    decoder_frames_at_client_++;
+  }
+
+  NotifyFlushDoneIfNeeded();
+}
+
+void ExynosVideoDecodeAccelerator::NotifyFlushDoneIfNeeded() {
+  if (!decoder_notify_flush_requested_)
+    return;
+
+  // Pipeline is empty when:
+  // * Decoder input holding queue is empty.
+  // * There is no currently filling input buffer.
+  // * MFC input holding queue is empty.
+  // * All MFC input (VIDEO_OUTPUT) buffers are returned.
+  // * MFC -> GSC holding queue is empty.
+  // * All GSC input (VIDEO_OUTPUT) buffers are returned.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Does this list mean that there can be pictures in the GSC output that haven't
been sent to the client yet when you consider the pipeline empty (and thus
notify of FlushDone)?  That would be wrong.
FlushDone should only be sent after all PictureReady()s that are the result of
Decode() calls made before the Flush() call.

[sheu, 2013/01/05 00:51:49]

Right now I rely on the GSC input and output queues advancing in lockstep.  I
agree that this is maybe brittle, so I've added gsc_output_buffer_queued_count_
to this.

+  if (decoder_current_input_buffer_ != -1)
+    return;
+  if ((decoder_input_queue_.size() + mfc_input_ready_queue_.size() +
+      mfc_input_buffer_queued_count_ + mfc_output_gsc_input_queue_.size() +
+      gsc_input_buffer_queued_count_) != 0)
+    return;
+
+  child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+    &Client::NotifyFlushDone, client_));
+  decoder_notify_flush_requested_ = false;
+}
+
+bool ExynosVideoDecodeAccelerator::EnqueueMfcInputRecord() {
+  DVLOG(3) << "EnqueueMfcInputRecord()";
+  DCHECK(!mfc_input_ready_queue_.empty());
+
+  // Enqueue a MFC input (VIDEO_OUTPUT) buffer.
+  const int buffer = mfc_input_ready_queue_.back();
+  MfcInputRecord& input_record = mfc_input_buffer_map_[buffer];
+  DCHECK(!input_record.at_device);
+  struct v4l2_buffer qbuf;
+  struct v4l2_plane qbuf_plane;
+  memset(&qbuf, 0, sizeof(qbuf));
+  memset(&qbuf_plane, 0, sizeof(qbuf_plane));
+  qbuf.index                 = buffer;
+  qbuf.type                  = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  qbuf.timestamp.tv_sec      = input_record.input_id;
+  qbuf.memory                = V4L2_MEMORY_MMAP;
+  qbuf.m.planes              = &qbuf_plane;
+  qbuf.m.planes[0].bytesused = input_record.bytes_used;
+  qbuf.length                = 1;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_QBUF, &qbuf);
+  mfc_input_ready_queue_.pop_back();
+  input_record.at_device = true;
+  mfc_input_buffer_queued_count_++;
+  DVLOG(3) << "EnqueueMfcInputRecord(): enqueued input_id="
+           << input_record.input_id;
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::EnqueueMfcOutputRecord() {
+  DVLOG(3) << "EnqueueMfcOutputRecord()";
+  DCHECK(!mfc_free_output_buffers_.empty());
+
+  // Enqueue a MFC output (VIDEO_CAPTURE) buffer.
+  const int buffer = mfc_free_output_buffers_.back();
+  MfcOutputRecord& output_record = mfc_output_buffer_map_[buffer];
+  DCHECK(!output_record.at_device);
+  DCHECK_EQ(output_record.input_id, -1);
+  struct v4l2_buffer qbuf;
+  struct v4l2_plane qbuf_planes[2];
+  memset(&qbuf, 0, sizeof(qbuf));
+  memset(qbuf_planes, 0, sizeof(qbuf_planes));
+  qbuf.index    = buffer;
+  qbuf.type     = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  qbuf.memory   = V4L2_MEMORY_MMAP;
+  qbuf.m.planes = qbuf_planes;
+  qbuf.length   = 2;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_QBUF, &qbuf);
+  mfc_free_output_buffers_.pop_back();
+  output_record.at_device = true;
+  mfc_output_buffer_queued_count_++;
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::EnqueueGscInputRecord() {
+  DVLOG(3) << "EnqueueGscInputRecord()";
+  DCHECK(!gsc_free_input_buffers_.empty());
+
+  // Enqueue a GSC input (VIDEO_OUTPUT) buffer for a complete MFC output
+  // (VIDEO_CAPTURE) buffer.
+  const int mfc_buffer = mfc_output_gsc_input_queue_.back();
+  const int gsc_buffer = gsc_free_input_buffers_.back();
+  MfcOutputRecord& output_record = mfc_output_buffer_map_[mfc_buffer];
+  DCHECK(!output_record.at_device);
+  GscInputRecord& input_record = gsc_input_buffer_map_[gsc_buffer];
+  DCHECK(!input_record.at_device);
+  DCHECK_EQ(input_record.mfc_output, -1);
+  struct v4l2_buffer qbuf;
+  struct v4l2_plane qbuf_planes[2];
+  memset(&qbuf, 0, sizeof(qbuf));
+  memset(qbuf_planes, 0, sizeof(qbuf_planes));
+  qbuf.index                 = gsc_buffer;
+  qbuf.type                  = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  qbuf.timestamp.tv_sec      = output_record.input_id;
+  qbuf.memory                = V4L2_MEMORY_USERPTR;
+  qbuf.m.planes              = qbuf_planes;
+  qbuf.m.planes[0].bytesused = output_record.bytes_used[0];
+  qbuf.m.planes[0].length    = mfc_output_buffer_size_[0];
+  qbuf.m.planes[0].m.userptr = (unsigned long)output_record.address[0];
+  qbuf.m.planes[1].bytesused = output_record.bytes_used[1];
+  qbuf.m.planes[1].length    = mfc_output_buffer_size_[1];
+  qbuf.m.planes[1].m.userptr = (unsigned long)output_record.address[1];
+  qbuf.length                = 2;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_QBUF, &qbuf);
+  mfc_output_gsc_input_queue_.pop_back();
+  gsc_free_input_buffers_.pop_back();
+  input_record.at_device = true;
+  input_record.mfc_output = mfc_buffer;
+  output_record.bytes_used[0] = 0;
+  output_record.bytes_used[1] = 0;
+  gsc_input_buffer_queued_count_++;
+  DVLOG(3) << "EnqueueGscInputRecord(): enqueued input_id="
+           << output_record.input_id;
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::EnqueueGscOutputRecord() {
+  DVLOG(3) << "EnqueueGscOutputRecord()";
+  DCHECK(!gsc_free_output_buffers_.empty());
+
+  // Enqueue a GSC output (VIDEO_CAPTURE) buffer.
+  const int buffer = gsc_free_output_buffers_.back();
+  GscOutputRecord& output_record = gsc_output_buffer_map_[buffer];
+  DCHECK(!output_record.at_device);
+  DCHECK(!output_record.at_client);
+  if (output_record.egl_sync != EGL_NO_SYNC_KHR) {
+    TRACE_EVENT0(
+        "Video Decoder",
+        "EVDA::EnqueueGscOutputRecord: eglClientWaitSyncKHR");
+    // If we have to wait for completion, wait.  Note that
+    // gsc_free_output_buffers_ is a FIFO queue, so we always wait on the
+    // buffer that has been in th queue the longest.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

s/th/the/

[sheu, 2013/01/05 00:51:49]

Done.

+    egl_client_wait_sync_khr(egl_display_, output_record.egl_sync, 0,
+        EGL_FOREVER_KHR);
+    egl_destroy_sync_khr(egl_display_, output_record.egl_sync);

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Why not pass ownership of the EGLSyncKHRRef to GscOutputRecord (instead of just
handing over the native token) and then give EGLSyncKHRRef a Wait() method that
waits & deletes the token if there is one?

This is motivated partially by wanting to keep the sync logic out of this
method, but also by the if test at l.1474 looking funny; I believe the only way
for it to fail is on the initial AssignPictureBuffers; the vast majortity of the
passes through that test will be in reaction to ReusePictureBuffers.

(the way the code is written now makes it look like when the sync token floats
by the GL engine some magic hand will come down and clear the
output_record.egl_sync field, so it'll be NO_SYNC by the time l.1474 is hit)

[sheu, 2013/01/05 00:51:49]

I think we'd end up (assuming that egl_sync becomes a scoped_ptr<EGLSyncKHRRef>)
either checking egl_sync != NULL up at l1474 anyways, or constructing empty refs
in AssignPictureBuffersTask() (so that Wait() completes immediately), so I don't
see how this would make things less confusing.

+    output_record.egl_sync = EGL_NO_SYNC_KHR;
+  }
+  struct v4l2_buffer qbuf;
+  struct v4l2_plane qbuf_plane;
+  memset(&qbuf, 0, sizeof(qbuf));
+  memset(&qbuf_plane, 0, sizeof(qbuf_plane));
+  qbuf.index            = buffer;
+  qbuf.type             = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  qbuf.memory           = V4L2_MEMORY_DMABUF;
+  qbuf.m.planes         = &qbuf_plane;
+  qbuf.m.planes[0].m.fd = output_record.fd;
+  qbuf.length           = 1;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_QBUF, &qbuf);
+  gsc_free_output_buffers_.pop_back();
+  output_record.at_device = true;
+  gsc_output_buffer_queued_count_++;
+  return true;
+}
+
+void ExynosVideoDecodeAccelerator::ReusePictureBufferTask(
+    int32 picture_buffer_id, scoped_ptr<EGLSyncKHRRef> egl_sync_ref) {
+  DVLOG(3) << "ReusePictureBufferTask(): picture_buffer_id="
+           << picture_buffer_id;
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  TRACE_EVENT0("Video Decoder", "EVDA::ReusePictureBufferTask");
+
+  // We run ReusePictureBufferTask even if we're in kResetting.
+  if (decoder_state_ == kError) {
+    DVLOG(2) << "ReusePictureBufferTask(): early out: kError state";
+    return;
+  }
+
+  size_t index;
+  for (index = 0; index < gsc_output_buffer_map_.size(); ++index)
+    if (gsc_output_buffer_map_[index].picture_id == picture_buffer_id)
+      break;
+
+  if (index >= gsc_output_buffer_map_.size()) {
+    DLOG(ERROR) << "ReusePictureBufferTask(): picture_buffer_id not found";
+    NOTIFY_ERROR(INVALID_ARGUMENT);
+    return;
+  }
+
+  GscOutputRecord& output_record = gsc_output_buffer_map_[index];
+  DCHECK(!output_record.at_device);
+  DCHECK(output_record.at_client);
+  DCHECK_EQ(output_record.egl_sync, EGL_NO_SYNC_KHR);
+  output_record.at_client = false;
+  output_record.egl_sync = egl_sync_ref->egl_sync;
+  gsc_free_output_buffers_.push_front(index);
+  decoder_frames_at_client_--;
+  // Take ownership of the EGLSync.
+  egl_sync_ref->egl_sync = EGL_NO_SYNC_KHR;
+  // We got a buffer back, so kick the GSC.
+  EnqueueGsc();
+}
+
+void ExynosVideoDecodeAccelerator::FlushTask() {
+  DVLOG(3) << "FlushTask()";

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Why no decoder_thread_-is-current DCHECK here?

[sheu, 2013/01/05 00:51:49]

Done.

+  TRACE_EVENT0("Video Decoder", "EVDA::FlushTask");
+
+  // Flush the currently-building frame.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Comment is strange/confusing.

[sheu, 2013/01/05 00:51:49]

Done.

+  if (decoder_state_ == kResetting) {
+    DVLOG(2) << "FlushTask(): early out: kResetting state";

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

See the comment about deferring Decode during Reset.  ISTM we should defer Flush
the same way (though OVDA doesn't seem to, instead DCHECKing).

[sheu, 2013/01/05 00:51:49]

Done.

+    return;
+  } else if (decoder_state_ == kError) {
+    DVLOG(2) << "FlushTask(): early out: kError state";
+    return;
+  }
+
+  // Queue up an empty buffer -- this triggers the flush.
+  decoder_input_queue_.push_front(linked_ptr<BitstreamBufferRef>(
+      new BitstreamBufferRef(client_, child_message_loop_proxy_, NULL, 0, -1)));
+  // We'll flag that we want a flush-finished notification, and just return.
+  decoder_notify_flush_requested_ = true;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

DCHECK it wasn't already true?

[sheu, 2013/01/05 00:51:49]

Done.

+
+  ScheduleDecodeBufferTaskIfNeeded();
+}
+
+void ExynosVideoDecodeAccelerator::ResetTask() {
+  DVLOG(3) << "ResetTask()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  TRACE_EVENT0("Video Decoder", "EVDA::ResetTask");
+
+  if (decoder_state_ == kError) {
+    DVLOG(2) << "DecodeTask(): early out: kError state";
+    return;
+  }
+
+  // We stop streaming, but we _don't_ destroy our buffers.
+  if (!StopDevicePoll())
+    return;
+
+  decoder_current_bitstream_buffer_.reset();
+  decoder_input_queue_.clear();
+
+  decoder_current_input_buffer_ = -1;
+  decoder_decode_buffer_tasks_scheduled_ = 0;
+  decoder_notify_flush_requested_ = false;
+
+  // Mark that we're resetting, then enqueue a ResetDoneTask().  All intervening
+  // jobs will early-out in the kResetting state.
+  decoder_state_ = kResetting;
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::ResetDoneTask, base::Unretained(this)));
+}
+
+void ExynosVideoDecodeAccelerator::ResetDoneTask() {
+  DVLOG(3) << "ResetDoneTask()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  TRACE_EVENT0("Video DecodeR", "EVDA::ResetDoneTask");
+
+  if (decoder_state_ == kError) {
+    DVLOG(2) << "DecodeTask(): early out: kError state";
+    return;
+  }
+
+  // Reset format-specific bits.
+  if (video_profile_ >= media::H264PROFILE_MIN &&
+      video_profile_ <= media::H264PROFILE_MAX) {
+    decoder_h264_parser_.reset(new content::H264Parser());
+  }
+
+  // Jobs drained, we're finished resetting.
+  DCHECK_EQ(decoder_state_, kResetting);
+  decoder_state_ = kAfterReset;
+  child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+      &Client::NotifyResetDone, client_));
+}
+
+void ExynosVideoDecodeAccelerator::DestroyTask() {
+  DVLOG(3) << "DestroyTask()";
+  TRACE_EVENT0("Video Decoder", "EVDA::DestroyTask");
+
+  // DestroyTask() should run regardless of decoder_state_.
+
+  // Stop streaming and the device_poll_thread_.
+  StopDevicePoll();
+
+  decoder_current_bitstream_buffer_.reset();
+  decoder_current_input_buffer_ = -1;
+  decoder_decode_buffer_tasks_scheduled_ = 0;
+  decoder_frames_at_client_ = 0;
+  decoder_notify_flush_requested_ = false;
+  decoder_input_queue_.clear();
+
+  // Set our state to kError.  This will cause all subsequent tasks to
+  // early-exit.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

what subsequent tasks? :)
This is the last thing that will run on decoder_thread_, device_poll_thread_ is
already Stop()'d, and the child thread will |delete this| as soon as we
return...

[sheu, 2013/01/05 00:51:49]

Paranoia.  :-)

+  decoder_state_ = kError;
+}
+
+bool ExynosVideoDecodeAccelerator::StartDevicePoll() {
+  DVLOG(3) << "StartDevicePoll()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  // Early-out if we're already running.

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

How can this happen?

[sheu, 2013/01/05 00:51:49]

I guess not.  DCHECK()ed.

+  if (device_poll_thread_.IsRunning()) {
+    DVLOG(2) << "StartDevicePoll(): early out: "
+             << "device poll thread already running";
+    return true;
+  }
+
+  // Start up the device poll thread and schedule its first DevicePollTask().
+  if (!device_poll_thread_.Start()) {
+    DLOG(ERROR) << "StartDevicePoll(): Device thread failed to start";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+  device_poll_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::DevicePollTask,
+      base::Unretained(this),
+      0));
+
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::StopDevicePoll() {
+  DVLOG(3) << "StopDevicePoll()";
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  // Signal the DevicePollTask() to stop, and stop the device poll thread.
+  if (!SetDevicePollInterrupt())
+    return false;
+  device_poll_thread_.Stop();
+  // Clear the interrupt now, to be sure.
+  if (!ClearDevicePollInterrupt())
+    return false;
+
+  // Stop streaming.
+  if (mfc_input_streamon_) {
+    __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+    IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_STREAMOFF, &type);
+  }
+  mfc_input_streamon_ = false;
+  if (mfc_output_streamon_) {
+    __u32 type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+    IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_STREAMOFF, &type);
+  }
+  mfc_output_streamon_ = false;
+  if (gsc_input_streamon_) {
+    __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+    IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_STREAMOFF, &type);
+  }
+  gsc_input_streamon_ = false;
+  if (gsc_output_streamon_) {
+    __u32 type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+    IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_STREAMOFF, &type);
+  }
+  gsc_output_streamon_ = false;
+
+  // Reset all our accounting info.
+  mfc_input_ready_queue_.clear();
+  mfc_free_input_buffers_.clear();
+  DCHECK_EQ(mfc_input_buffer_count_,
+      static_cast<int>(mfc_input_buffer_map_.size()));
+  for (size_t i = 0; i < mfc_input_buffer_map_.size(); ++i) {
+    mfc_free_input_buffers_.push_back(i);
+    mfc_input_buffer_map_[i].at_device = false;
+    mfc_input_buffer_map_[i].bytes_used = 0;
+    mfc_input_buffer_map_[i].input_id = -1;
+  }
+  mfc_input_buffer_queued_count_ = 0;
+  mfc_free_output_buffers_.clear();
+  DCHECK_EQ(mfc_output_buffer_count_,
+     static_cast<int>(mfc_output_buffer_map_.size()));
+  for (size_t i = 0; i < mfc_output_buffer_map_.size(); ++i) {
+    mfc_free_output_buffers_.push_back(i);
+    mfc_output_buffer_map_[i].at_device = false;
+    mfc_output_buffer_map_[i].input_id = -1;
+  }
+  mfc_output_buffer_queued_count_ = 0;
+  mfc_output_gsc_input_queue_.clear();
+  gsc_free_input_buffers_.clear();
+  DCHECK_EQ(gsc_input_buffer_count_,
+      static_cast<int>(gsc_input_buffer_map_.size()));
+  for (size_t i = 0; i < gsc_input_buffer_map_.size(); ++i) {
+    gsc_free_input_buffers_.push_back(i);
+    gsc_input_buffer_map_[i].at_device = false;
+    gsc_input_buffer_map_[i].mfc_output = -1;
+  }
+  gsc_input_buffer_queued_count_ = 0;
+  gsc_free_output_buffers_.clear();
+  DCHECK_EQ(gsc_output_buffer_count_,
+      static_cast<int>(gsc_output_buffer_map_.size()));
+  for (size_t i = 0; i < gsc_output_buffer_map_.size(); ++i) {
+    // Only mark those free that aren't being held by the VDA.
+    if (!gsc_output_buffer_map_[i].at_client) {
+      gsc_free_output_buffers_.push_back(i);
+      gsc_output_buffer_map_[i].at_device = false;
+    }
+  }
+  gsc_output_buffer_queued_count_ = 0;
+
+  DVLOG(3) << "StopDevicePoll(): device poll stopped";
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::SetDevicePollInterrupt() {
+  DVLOG(3) << "SetDevicePollInterrupt()";

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

I believe it's true that this and CDPI below can only be called on the
device_thread_; DCHECK that fact?

[sheu, 2013/01/05 00:51:49]

Done.

+  ssize_t ret;
+  const uint64 buf = 1;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

Is there some reason to use a uint64 to carry a single bit?
(why not a uint8?)

[sheu, 2013/01/05 00:51:49]

The eventfd interface requires uint64.

+  ret = write(device_poll_interrupt_fd_, &buf, sizeof(buf));
+  if (ret == -1) {
+    DPLOG(ERROR) << "SetDevicePollInterrupt(): write() failed";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::ClearDevicePollInterrupt() {
+  DVLOG(3) << "ClearDevicePollInterrupt()";
+  int ret;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

declare at first use two lines down

[sheu, 2013/01/05 00:51:49]

I kinda like to declare them in use order, and in this case it's ret and then
buf.

+  uint64 buf;
+  ret = read(device_poll_interrupt_fd_, &buf, sizeof(buf));
+  if (ret == -1) {

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

l.1759-1769 can be summarized as
if (ret == -1 && errno != EAGAIN) {
  DPLOG(ERROR) << "ClearDevicePollInterrupt(): read() failed";
  NOTIFY_ERROR(PLATFORM_FAILURE);
  return false;
}
return true;

[sheu, 2013/01/05 00:51:49]

Yep, but I wanted to explicitly document the EAGAIN case.

+    if (errno == EAGAIN) {
+      // No interrupt flag set, and we're reading nonblocking.  Not an error.
+      return true;
+    } else {
+      DPLOG(ERROR) << "ClearDevicePollInterrupt(): read() failed";
+      NOTIFY_ERROR(PLATFORM_FAILURE);
+      return false;
+    }
+  }
+  return true;
+}
+
+void ExynosVideoDecodeAccelerator::DevicePollTask(unsigned int poll_fds) {
+  DVLOG(3) << "DevicePollTask()";
+  DCHECK_EQ(device_poll_thread_.message_loop(), MessageLoop::current());
+  TRACE_EVENT0("Video Decoder", "EVDA::DevicePollTask");
+
+  // This routine just polls and the set of device fds, and schedules a

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

s/and the/the/

[sheu, 2013/01/05 00:51:49]

Done.

+  // ServiceDeviceTask() on decoder_thread_ when processing needs to occur.
+  // Other threads may notify this task to return early by writing to
+  // device_poll_interrupt_fd_.
+  struct pollfd pollfds[3];
+  nfds_t nfds;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

declare at first assignment, or init to 0 here and ++ for interrupt_fd_

[sheu, 2013/01/05 00:51:49]

Done.

+  int ret;
+
+  // Add device_poll_interrupt_fd_;
+  pollfds[0].fd = device_poll_interrupt_fd_;
+  pollfds[0].events = POLLIN | POLLERR;
+  nfds = 1;
+
+  if (poll_fds & kPollMfc) {
+    DVLOG(3) << "DevicePollTask(): adding MFC to poll() set";
+    pollfds[nfds].fd = mfc_fd_;
+    pollfds[nfds].events = POLLIN | POLLOUT | POLLERR;
+    nfds++;
+  }
+  // Add GSC fd, if we should poll on it.
+  // GSC has to wait until both input and output buffers are queued.
+  if (poll_fds & kPollGsc) {
+    DVLOG(3) << "DevicePollTask(): adding GSC to poll() set";
+    pollfds[nfds].fd = gsc_fd_;
+    pollfds[nfds].events = POLLIN | POLLOUT | POLLERR;
+    nfds++;
+  }
+
+  // Poll it!
+  do {
+    ret = poll(pollfds, nfds, -1);
+  } while (ret < 1 && errno == EINTR);
+  if (ret == -1) {
+    DPLOG(ERROR) << "DevicePollTask(): poll() failed";
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return;
+  }
+
+  // All processing should happen on ServiceDeviceTask(), since we shouldn't
+  // touch decoder state from this thread.
+  decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+      &ExynosVideoDecodeAccelerator::ServiceDeviceTask,
+      base::Unretained(this)));
+}
+
+void ExynosVideoDecodeAccelerator::NotifyError(Error error) {
+  DVLOG(2) << "NotifyError()";
+
+  if (!child_message_loop_proxy_->BelongsToCurrentThread()) {
+    child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &ExynosVideoDecodeAccelerator::NotifyError, weak_this_, error));
+    return;
+  }
+
+  if (client_) {
+    client_->NotifyError(error);
+    client_ptr_factory_.InvalidateWeakPtrs();
+  }
+}
+
+void ExynosVideoDecodeAccelerator::SetDecoderState(State state) {
+  DVLOG(3) << "SetDecoderState(): state=%d" << state;
+
+  // We can touch decoder_state_ only if this is the decoder thread or the
+  // decoder thread isn't running.
+  if (decoder_thread_.message_loop() != NULL &&
+      decoder_thread_.message_loop() != MessageLoop::current()) {
+    decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+        &ExynosVideoDecodeAccelerator::SetDecoderState,
+        base::Unretained(this), state));
+  } else {
+    decoder_state_ = state;
+  }
+}
+
+bool ExynosVideoDecodeAccelerator::CreateMfcInputBuffers() {
+  DVLOG(3) << "CreateMfcInputBuffers()";
+  // We always run this as we prepare to initialize.
+  DCHECK_EQ(decoder_state_, kUninitialized);
+  DCHECK(!mfc_input_streamon_);
+  DCHECK_EQ(mfc_input_buffer_count_, 0);
+
+  __u32 pixelformat = 0;
+  if (video_profile_ >= media::H264PROFILE_MIN &&
+      video_profile_ <= media::H264PROFILE_MAX) {
+    pixelformat = V4L2_PIX_FMT_H264;
+  } else if (video_profile_ >= media::VP8PROFILE_MIN &&
+             video_profile_ <= media::VP8PROFILE_MAX) {
+    pixelformat = V4L2_PIX_FMT_VP8;
+  } else {
+    NOTREACHED();
+  }
+
+  struct v4l2_format format;
+  memset(&format, 0, sizeof(format));
+  format.type                              = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  format.fmt.pix_mp.pixelformat            = pixelformat;
+  format.fmt.pix_mp.plane_fmt[0].sizeimage = kMfcInputBufferMaxSize;
+  format.fmt.pix_mp.num_planes             = 1;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_S_FMT, &format);
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count  = kMfcInputBufferCount;
+  reqbufs.type   = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_REQBUFS, &reqbufs);
+  mfc_input_buffer_count_ = reqbufs.count;
+  mfc_input_buffer_map_.resize(mfc_input_buffer_count_);
+  for (int i = 0; i < mfc_input_buffer_count_; ++i) {
+    mfc_free_input_buffers_.push_back(i);
+
+    // Query for the MEMORY_MMAP pointer.
+    struct v4l2_plane planes[1];
+    struct v4l2_buffer buffer;
+    memset(&buffer, 0, sizeof(buffer));
+    memset(planes, 0, sizeof(planes));
+    buffer.index    = i;
+    buffer.type     = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+    buffer.memory   = V4L2_MEMORY_MMAP;
+    buffer.m.planes = planes;
+    buffer.length   = 1;
+    IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_QUERYBUF, &buffer);
+    void* address = mmap(NULL, buffer.m.planes[0].length,
+        PROT_READ | PROT_WRITE, MAP_SHARED, mfc_fd_,
+        buffer.m.planes[0].m.mem_offset);
+    if (address == MAP_FAILED) {
+      DPLOG(ERROR) << "CreateMfcInputBuffers(): mmap() failed";
+      return false;
+    }
+    mfc_input_buffer_map_[i].address = address;
+    mfc_input_buffer_map_[i].length = buffer.m.planes[0].length;
+  }
+
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::CreateMfcOutputBuffers() {
+  DVLOG(3) << "CreateMfcOutputBuffers()";
+  DCHECK_EQ(decoder_state_, kInitialized);
+  DCHECK(!mfc_output_streamon_);
+  DCHECK_EQ(mfc_output_buffer_count_, 0);
+
+  // Number of MFC output buffers we need.
+  struct v4l2_control ctrl;
+  memset(&ctrl, 0, sizeof(ctrl));
+  ctrl.id = V4L2_CID_MIN_BUFFERS_FOR_CAPTURE;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_G_CTRL, &ctrl);
+
+  // Output format setup in Initialize().
+
+  // Allocate the output buffers.
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count  = ctrl.value + kMfcOutputBufferExtraCount;
+  reqbufs.type   = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_REQBUFS, &reqbufs);
+
+  // Fill our free-buffers list, and create DMABUFs from them.
+  mfc_output_buffer_count_ = reqbufs.count;
+  mfc_output_buffer_map_.resize(mfc_output_buffer_count_);
+  for (int i = 0; i < mfc_output_buffer_count_; ++i) {
+    mfc_free_output_buffers_.push_back(i);
+
+    // Query for the MEMORY_MMAP pointer.
+    struct v4l2_plane planes[2];
+    struct v4l2_buffer buffer;
+    memset(&buffer, 0, sizeof(buffer));
+    memset(planes, 0, sizeof(planes));
+    buffer.index    = i;
+    buffer.type     = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+    buffer.memory   = V4L2_MEMORY_MMAP;
+    buffer.m.planes = planes;
+    buffer.length   = 2;
+    IOCTL_OR_ERROR_RETURN_FALSE(mfc_fd_, VIDIOC_QUERYBUF, &buffer);
+
+    // Get their user memory for GSC input.
+    for (int j = 0; j < 2; ++j) {
+      void* address = mmap(NULL, buffer.m.planes[j].length,
+          PROT_READ | PROT_WRITE, MAP_SHARED, mfc_fd_,
+          buffer.m.planes[j].m.mem_offset);
+      if (address == MAP_FAILED) {
+        DPLOG(ERROR) << "CreateMfcInputBuffers(): mmap() failed";
+        return false;
+      }
+      mfc_output_buffer_map_[i].address[j] = address;
+      mfc_output_buffer_map_[i].length[j] = buffer.m.planes[j].length;
+    }
+  }
+
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::CreateGscInputBuffers() {
+  DVLOG(3) << "CreateGscInputBuffers()";
+  DCHECK_EQ(decoder_state_, kInitialized);
+  DCHECK(!gsc_input_streamon_);
+  DCHECK_EQ(gsc_input_buffer_count_, 0);
+
+  struct v4l2_format format;
+  memset(&format, 0, sizeof(format));
+  format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  format.fmt.pix_mp.width  = frame_buffer_size_.width();
+  format.fmt.pix_mp.height = frame_buffer_size_.height();
+  DCHECK_EQ(mfc_output_buffer_pixelformat_, V4L2_PIX_FMT_NV12MT_16X16);

[Ami GONE FROM CHROMIUM, 2013/01/04 01:57:05]

Any reason to DCHECK this here instead of where the LHS is assigned (l.858)?

[sheu, 2013/01/05 00:51:49]

I guess I could do that.

+  format.fmt.pix_mp.pixelformat = mfc_output_buffer_pixelformat_;
+  format.fmt.pix_mp.plane_fmt[0].sizeimage = mfc_output_buffer_size_[0];
+  format.fmt.pix_mp.plane_fmt[1].sizeimage = mfc_output_buffer_size_[1];
+  // NV12MT_16X16 is a tiled format for which bytesperline doesn't make too much
+  // sense.  Convention seems to be to assume 8bpp for these tiled formats.
+  format.fmt.pix_mp.plane_fmt[0].bytesperline = frame_buffer_size_.width();
+  format.fmt.pix_mp.plane_fmt[1].bytesperline = frame_buffer_size_.width();
+  format.fmt.pix_mp.num_planes = 2;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_FMT, &format);
+
+  struct v4l2_control control;
+  memset(&control, 0, sizeof(control));
+  control.id = V4L2_CID_ROTATE;
+  control.value = 0;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_CTRL, &control);
+
+  memset(&control, 0, sizeof(control));
+  control.id = V4L2_CID_HFLIP;
+  control.value = 0;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_CTRL, &control);
+
+  memset(&control, 0, sizeof(control));
+  control.id = V4L2_CID_VFLIP;
+  control.value = 0;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_CTRL, &control);
+
+  memset(&control, 0, sizeof(control));
+  control.id = V4L2_CID_GLOBAL_ALPHA;
+  control.value = 255;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_CTRL, &control);
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = kGscInputBufferCount;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_USERPTR;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_REQBUFS, &reqbufs);
+
+  gsc_input_buffer_count_ = reqbufs.count;
+  gsc_input_buffer_map_.resize(gsc_input_buffer_count_);
+  for (int i = 0; i < gsc_input_buffer_count_; ++i) {
+    gsc_free_input_buffers_.push_back(i);
+    gsc_input_buffer_map_[i].mfc_output = -1;
+  }
+
+  return true;
+}
+
+bool ExynosVideoDecodeAccelerator::CreateGscOutputBuffers() {
+  DVLOG(3) << "CreateGscOutputBuffers()";
+  DCHECK_EQ(decoder_state_, kInitialized);
+  DCHECK(!gsc_output_streamon_);
+  DCHECK_EQ(gsc_output_buffer_count_, 0);
+
+  // GSC outputs into the EGLImages we create from the textures we are
+  // assigned.  Assume RGBA8888 format.
+  struct v4l2_format format;
+  memset(&format, 0, sizeof(format));
+  format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  format.fmt.pix_mp.width  = frame_buffer_size_.width();
+  format.fmt.pix_mp.height = frame_buffer_size_.height();
+  format.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_RGB32;
+  format.fmt.pix_mp.plane_fmt[0].sizeimage =
+      frame_buffer_size_.width() * frame_buffer_size_.height() * 4;
+  format.fmt.pix_mp.plane_fmt[0].bytesperline = frame_buffer_size_.width() * 4;
+  format.fmt.pix_mp.num_planes = 1;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_FMT, &format);
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = kGscOutputBufferCount;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_DMABUF;
+  IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_REQBUFS, &reqbufs);
+
+  // We don't actually fill in the freelist or the map here.  That happens once
+  // we have actual usable buffers, after AssignPictureBuffers();
+  gsc_output_buffer_count_ = reqbufs.count;
+  gsc_output_buffer_map_.resize(gsc_output_buffer_count_);
+
+  DVLOG(3) << "CreateGscOutputBuffers(): ProvidePictureBuffers(): "
+           << "buffer_count=" << gsc_output_buffer_count_
+           << ", width=" << frame_buffer_size_.width()
+           << ", height=" << frame_buffer_size_.height();
+  child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+      &Client::ProvidePictureBuffers, client_, gsc_output_buffer_count_,
+      gfx::Size(frame_buffer_size_.width(), frame_buffer_size_.height()),
+      GL_TEXTURE_2D));
+
+  return true;
+}
+
+void ExynosVideoDecodeAccelerator::DestroyMfcInputBuffers() {
+  DVLOG(3) << "DestroyMfcInputBuffers()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(!mfc_input_streamon_);
+
+  for (size_t i = 0; i < mfc_input_buffer_map_.size(); ++i) {
+    if (mfc_input_buffer_map_[i].address != NULL) {
+      munmap(mfc_input_buffer_map_[i].address,
+          mfc_input_buffer_map_[i].length);
+    }
+  }
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = 0;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  if (ioctl(mfc_fd_, VIDIOC_REQBUFS, &reqbufs) != 0)
+    DPLOG(ERROR) << "DestroyMfcInputBuffers(): ioctl() failed: VIDIOC_REQBUFS";
+
+  mfc_input_buffer_map_.clear();
+  mfc_free_input_buffers_.clear();
+  mfc_input_buffer_count_ = 0;
+}
+
+void ExynosVideoDecodeAccelerator::DestroyMfcOutputBuffers() {
+  DVLOG(3) << "DestroyMfcOutputBuffers()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(!mfc_output_streamon_);
+
+  for (size_t i = 0; i < mfc_output_buffer_map_.size(); ++i) {
+    if (mfc_output_buffer_map_[i].address[0] != NULL)
+      munmap(mfc_output_buffer_map_[i].address[0],
+          mfc_output_buffer_map_[i].length[0]);
+    if (mfc_output_buffer_map_[i].address[1] != NULL)
+      munmap(mfc_output_buffer_map_[i].address[1],
+          mfc_output_buffer_map_[i].length[1]);
+  }
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = 0;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_MMAP;
+  if (ioctl(mfc_fd_, VIDIOC_REQBUFS, &reqbufs) != 0)
+    DPLOG(ERROR) << "DestroyMfcOutputBuffers() ioctl() failed: VIDIOC_REQBUFS";
+
+  mfc_output_buffer_map_.clear();
+  mfc_free_output_buffers_.clear();
+  mfc_output_buffer_count_ = 0;
+}
+
+void ExynosVideoDecodeAccelerator::DestroyGscInputBuffers() {
+  DVLOG(3) << "DestroyGscInputBuffers()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(!gsc_input_streamon_);
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = 0;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_DMABUF;
+  if (ioctl(gsc_fd_, VIDIOC_REQBUFS, &reqbufs) != 0)
+    DPLOG(ERROR) << "DestroyGscInputBuffers(): ioctl() failed: VIDIOC_REQBUFS";
+
+  gsc_input_buffer_map_.clear();
+  gsc_free_input_buffers_.clear();
+  gsc_input_buffer_count_ = 0;
+}
+
+void ExynosVideoDecodeAccelerator::DestroyGscOutputBuffers() {
+  DVLOG(3) << "DestroyGscOutputBuffers()";
+  DCHECK(child_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(!gsc_output_streamon_);
+
+  if (gsc_output_buffer_map_.size() != 0) {
+    if (!make_context_current_.Run())
+      DLOG(ERROR) << "DestroyGscOutputBuffers(): "
+                  << "could not make context current";
+
+    size_t i = 0;

[Ami GONE FROM CHROMIUM, 2013/01/04 21:38:55]

any reason not to replace this with a for loop?

[sheu, 2013/01/05 00:51:49]

Save on the first iteration, checking against gsc_output_buffer_map_.size().  We
already know it's >= 1, since we checked that before running
make_current_context_.

[Ami GONE FROM CHROMIUM, 2013/01/11 00:46:33]

lolwat?  Are you saying you're using 
i=0;
do {
  ...1
  ++i;
} while (...2);
instead of 
for (i=0; ...2; ++i) {
  ...1
}

to save the one comparison of 0<gsc_output_buffer_map_.size() (and its attendant
eval of size()) ?
That's crazy.

[sheu, 2013/01/11 10:41:42]

Yeah, it's crazy.  Don't think it's incorrect though :-)

+    do {
+      GscOutputRecord& output_record = gsc_output_buffer_map_[i];
+      if (output_record.fd != -1)
+        close(output_record.fd);
+      if (output_record.egl_image != EGL_NO_IMAGE_KHR)
+        egl_destroy_image_khr(egl_display_, output_record.egl_image);
+      if (output_record.egl_sync != EGL_NO_SYNC_KHR)
+        egl_destroy_sync_khr(egl_display_, output_record.egl_sync);
+      if (client_)
+        client_->DismissPictureBuffer(output_record.picture_id);
+      ++i;
+    } while (i < gsc_output_buffer_map_.size());
+  }
+
+  struct v4l2_requestbuffers reqbufs;
+  memset(&reqbufs, 0, sizeof(reqbufs));
+  reqbufs.count = 0;
+  reqbufs.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+  reqbufs.memory = V4L2_MEMORY_DMABUF;
+  if (ioctl(gsc_fd_, VIDIOC_REQBUFS, &reqbufs) != 0)
+    DPLOG(ERROR) << "DestroyGscOutputBuffers(): ioctl() failed: VIDIOC_REQBUFS";
+
+  gsc_output_buffer_map_.clear();
+  gsc_free_output_buffers_.clear();
+  gsc_output_buffer_count_ = 0;
+}
+
+}  // namespace content