Add ArcGpuVideoDecodeAccelerator.

content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "base/run_loop.h"
+#include "content/common/gpu/media/arc/arc_gpu_video_decode_accelerator.h"
+
+namespace content {
+namespace arc {
+
+ArcGpuVideoDecodeAccelerator::BufferInfo::BufferInfo()
+    : state(NOT_BOUND), offset(0), length(0) {}
+
+ArcGpuVideoDecodeAccelerator::PortInfo::PortInfo()
+    : memory_type(MEMORY_DMABUF) {}
+
+ArcGpuVideoDecodeAccelerator::InputRecord::InputRecord(
+    int32_t bitstream_buffer_id,
+    uint32_t index,
+    int64_t timestamp)
+    : bitstream_buffer_id(bitstream_buffer_id),
+      index(index),
+      timestamp(timestamp) {}
+
+ArcGpuVideoDecodeAccelerator::ArcGpuVideoDecodeAccelerator()
+    : arc_client_(nullptr) {}
+
+status_t ArcGpuVideoDecodeAccelerator::Initialize(

[Pawel Osciak, 2016/01/07 09:22:35]

Please use a ThreadChecker to make sure methods are run on the correct thread.

[Owen Lin, 2016/01/12 09:30:23]

Done.

+    DeviceType device,
+    ArcVideoAccelerator::Client* client) {
+  if (device != DEVICE_DECODER)
+    return EINVAL;

[Pawel Osciak, 2016/01/07 09:22:35]

I'm not seeing any other values apart from EINVAL and 0 returned from status_t
methods. Perhaps we could consider using Chrome style and return bools?

[Owen Lin, 2016/01/12 09:30:22]

Done.

[Owen Lin, 2016/01/12 09:30:23]

Done.

+  DCHECK(client);
+  DCHECK(!arc_client_);
+  arc_client_ = client;
+  return 0;
+}
+
+status_t ArcGpuVideoDecodeAccelerator::SetBufferCount(PortType port,
+                                                      size_t* count) {
+  if (vda_ == nullptr) {
+    DVLOG(1) << "Must call setBufferFormat before setBufferCount";
+    return EINVAL;
+  }
+  if (port != PORT_INPUT && port != PORT_OUTPUT) {
+    DVLOG(1) << "Invalid port: " << port;
+    return EINVAL;
+  }
+  if (port == PORT_OUTPUT) {
+    std::vector<media::PictureBuffer> buffers;
+    for (int32_t id = 0, n = *count; id < n; ++id) {
+      // TODO: Make sure the size is what we want.
+      buffers.push_back(media::PictureBuffer(id, coded_size_, 0));
+    }
+    vda_->AssignPictureBuffers(buffers);
+  }
+  PortInfo* port_info = &port_info_[port];
+  port_info->buffers.clear();
+  port_info->buffers.resize(*count);
+  return 0;
+}
+
+status_t ArcGpuVideoDecodeAccelerator::SetBufferFormat(
+    PortType port,
+    const BufferFormat& format) {
+  switch (port) {
+    case PORT_INPUT: {
+      PortInfo* port_info = &port_info_[port];
+      if (format.memory_type != MEMORY_SHARED_MEMORY) {
+        DVLOG(1) << "Only SharedMemory is supported for input buffers";
+        return EINVAL;
+      }
+      port_info->memory_type = format.memory_type;
+      media::VideoDecodeAccelerator::Config config;
+      switch (format.pixel_format) {
+        case HAL_PIXEL_FORMAT_H264:
+          config.profile = media::H264PROFILE_MAIN;
+          vda_ = nullptr;
+          break;
+        case HAL_PIXEL_FORMAT_VP8:
+          config.profile = media::VP8PROFILE_ANY;
+          vda_ = nullptr;  // TODO(owenlin): Change to real implementation.

[Pawel Osciak, 2016/01/07 09:22:35]

Do we need this and l.80? vda_ can be constructed without knowing the profile.
Perhaps we should consider constructing it earlier.
We should have similar code as in GVDA to select the correct VDA to construct,
and (perhaps eventually) share it.

[Owen Lin, 2016/01/12 09:30:22]

Move it to Initialize. 

+          break;
+        default:
+          DVLOG(1) << "Unsupported input format: " << format.pixel_format;
+          return EINVAL;
+      }
+      config.output_mode =
+          media::VideoDecodeAccelerator::Config::OutputMode::IMPORT;
+      vda_->Initialize(config, this);

[Pawel Osciak, 2016/01/07 09:22:35]

This may fail, we should handle the return value.

[Owen Lin, 2016/01/12 09:30:22]

Done.

+      break;
+    }
+    case PORT_OUTPUT: {
+      PortInfo* port_info = &port_info_[port];
+      if (format.memory_type != MEMORY_DMABUF) {
+        DVLOG(1) << "Only DMA buffer is supported for output buffers";
+        return EINVAL;
+      }
+      port_info->memory_type = format.memory_type;
+      break;
+    }
+    default:
+      DVLOG(1) << "Invalid port: " << port;
+      return EINVAL;
+  }
+  return 0;
+}
+
+ArcGpuVideoDecodeAccelerator::BufferInfo*
+ArcGpuVideoDecodeAccelerator::GetBufferInfo(PortType port, uint32_t index) {
+  if (port != PORT_INPUT || port != PORT_OUTPUT)
+    return nullptr;
+  auto& buffers = port_info_[port].buffers;
+  return index >= buffers.size() ? nullptr : &buffers[index];
+}
+
+status_t ArcGpuVideoDecodeAccelerator::BindSharedBuffer(PortType port,
+                                                        uint32_t index,
+                                                        int ashmem_fd,
+                                                        size_t offset,
+                                                        size_t length) {
+  // Make sure we will close the file descriptor.
+  base::ScopedFD handle(ashmem_fd);
+  BufferInfo* buffer_info = GetBufferInfo(port, index);
+  if (buffer_info == nullptr) {
+    DVLOG(1) << "Invalid buffer port: " << port << " index: " << index;
+    return EINVAL;
+  }
+  PortInfo* port_info = &port_info_[port];

[Pawel Osciak, 2016/01/07 09:22:36]

It's not straightforward that we are depending on GetBufferInfo to validate
port. Could we instead validate memory type in GetBufferInfo and remove
l.131-135?

[Owen Lin, 2016/01/12 09:30:22]

But we call GetBufferInfo a lot. In most cases, there is no need to check the
memory type.
How about a new "ValidatePortAndIndex()" function to do the validation
explicitly?

+  if (port_info->memory_type != MEMORY_SHARED_MEMORY) {
+    DVLOG(1) << "Unmatched memory type: " << port_info->memory_type;
+    return EINVAL;
+  }
+  if (buffer_info->state != NOT_BOUND ||

[Pawel Osciak, 2016/01/07 09:22:35]

I think we can drop NOT_BOUND state. VDA has to verify it has a valid handle
anyway before using the buffer. Similarly, it has to check if ReusePictureBuffer
is called on a picture_id that is already owned by it. This and with my comment
about flush below, I think we could remove buffer states entirely?

[Owen Lin, 2016/01/12 09:30:22]

Done.

+      buffer_info->state != OWNED_BY_CLIENT) {
+    DVLOG(1) << "Cannot bind a buffer while in use";
+    return EINVAL;
+  }
+  buffer_info->state = OWNED_BY_CLIENT;
+  buffer_info->handle = std::move(handle);
+  buffer_info->offset = offset;
+  buffer_info->length = length;
+  return 0;
+}
+
+status_t ArcGpuVideoDecodeAccelerator::BindGraphicBuffer(PortType port,
+                                                         uint32_t index,
+                                                         int dmabuf_fd) {
+  // Make sure we will close the file descriptor.
+  base::ScopedFD handle(dmabuf_fd);
+  BufferInfo* buffer_info = GetBufferInfo(port, index);

[Pawel Osciak, 2016/01/07 09:22:35]

Could we unify the code with the other Bind? It seems that the only difference
is the Import path at the end.

[Owen Lin, 2016/01/12 09:30:22]

After we remove the check for buffer's state the only thing is to check port's
memory type.
I don't think we need another function to do that.

+  if (buffer_info == nullptr) {
+    DVLOG(1) << "Invalid buffer port: " << port << " index: " << index;
+    return EINVAL;
+  }
+  PortInfo* port_info = &port_info_[port];
+  if (port_info->memory_type != MEMORY_DMABUF) {
+    DVLOG(1) << "Unmatched memory type: " << port_info->memory_type;
+    return EINVAL;
+  }
+  if (buffer_info->state != NOT_BOUND ||
+      buffer_info->state != OWNED_BY_CLIENT) {
+    DVLOG(1) << "Cannot bind a buffer while in use";
+    return EINVAL;
+  }
+  buffer_info->state = OWNED_BY_CLIENT;
+  std::vector<base::ScopedFD> dmabuf_fds;
+  dmabuf_fds.push_back(std::move(handle));
+  vda_->ImportBufferForPicture(index, std::move(dmabuf_fds));
+  return 0;
+}
+
+void ArcGpuVideoDecodeAccelerator::UseBuffer(PortType port,
+                                             uint32_t index,
+                                             const BufferMetadata& metadata) {
+  BufferInfo* buffer_info = GetBufferInfo(port, index);
+  if (buffer_info == nullptr) {
+    DVLOG(1) << "Invalid buffer port: " << port << ", index: " << index;
+    arc_client_->OnError(INVALID_ARGUMENT);

[Pawel Osciak, 2016/01/07 09:22:35]

It would be great to remove status returns and use this everywhere instead...

[Owen Lin, 2016/01/12 09:30:22]

I think it's the difference between the design of Android and Chromium. Most of
the OMX functions are synchronized. I am not sure about Codec 2.0, maybe we can
gave Lajos some suggestions. For simplicity, I can only make UseBuffer() async
now.

We can try to make more function asynchronized when we remove AVA.h.

+    return;
+  }
+  if (buffer_info->state != OWNED_BY_CLIENT) {
+    DVLOG(1) << "Illegal buffer state: " << buffer_info->state
+             << ", port: " << port << ", index: " << index;
+    arc_client_->OnError(ILLEGAL_STATE);
+    return;
+  }
+  buffer_info->state = OWNED_BY_US;
+  switch (port) {
+    case PORT_INPUT: {
+      if (metadata.flags & BUFFER_FLAG_EOS) {
+        // Ask VDA to return all output pictures so that we can output an EOS
+        // picture when Flush() is done.
+        // vda_->Flush(true);
+        vda_->Flush();
+      }
+      if (metadata.bytes_used > 0) {
+        int32_t bitstream_buffer_id = bitstream_buffer_serial_++;
+        if (bitstream_buffer_serial_ < 0)
+          bitstream_buffer_serial_ = 0;

[Pawel Osciak, 2016/01/07 09:22:35]

Perhaps something like
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_...
?

[Owen Lin, 2016/01/12 09:30:22]

Done.

+        SetInputRecord(bitstream_buffer_id, index, metadata.timestamp);
+        base::SharedMemoryHandle shared_memory_handle(buffer_info->handle.get(),
+                                                      false);
+        // TODO(owenlin): Make BitstreamBuffer surpport offset and use here.
+        media::BitstreamBuffer bitstream_buffer(
+            bitstream_buffer_id, shared_memory_handle, buffer_info->length);
+        buffer_info->state = OWNED_BY_VDA;

[Pawel Osciak, 2016/01/07 09:22:35]

I'm wondering if we could merge OWNED_BY_US and OWNED_BY_VDA to have one state
less. Instead of marking buffers with OWNED_BY_US, we could just push them onto
a queue:

in the class:

std::queue<Picture> pictures_pending_eos_;


then:

PictureReady() {
  if (picture.picture_buffer_id == -1) {
    pictures_pending_eos_.push(picture);
  } else {
    // ...
    arc_client->OnBufferDone(...);
  }
}

UseBuffer() {
  // ...
  SendEosIfNeededOrReusePicture(picture);
}

SendEosIfNeededOrReusePicture(Picture picture) {
  if (pending_eos_output_buffer_) {
    pictures_pending_eos_.push(picture);
    TryFinishEos();
  } else {
    vda->ReusePictureBuffer(picture...);
  }
}

NotifyFlushDone() {
  pending_eos_output_buffer_ = true;
  TryFinishEos();
}

TryFinishEos() {
  DCHECK(pending_eos_output_buffer_);
  if (pictures_pending_eos_.empty()) {
    // will have to send EOS on next UseBuffer()
    return;
  }

  while (!pictures_pending_eos_.empty()) {
    Picture picture = pictures_pending_eos_.front();
    pictures_pending_eos_.pop();
    if (!eos_sent) {
      // send EOS in picture
    } else {
      vda->ReusePictureBuffer(picture);
    }
  }

  pending_eos_output_buffer_ = false;
}

[Owen Lin, 2016/01/12 09:30:22]

Done.

+        vda_->Decode(bitstream_buffer);
+      } else {

[Pawel Osciak, 2016/01/07 09:22:35]

Perhaps reversing the if() and removing else would make it a bit easier to read:

if (metadata.bytes_used == 0) {
  buffer_info->state = OWNED_BY_CLIENT;
  arc_client_->OnBufferDone(PORT_INPUT, index, BufferMetadata());
  return;
}

// here handle normal case

[Owen Lin, 2016/01/12 09:30:22]

Done.

+        buffer_info->state = OWNED_BY_CLIENT;
+        arc_client_->OnBufferDone(PORT_INPUT, index, BufferMetadata());
+      }
+      break;
+    }
+    case PORT_OUTPUT: {
+      SendEosIfNeededOrReusePicture(index, buffer_info);
+      break;
+    }
+    default:
+      NOTREACHED();
+  }
+}
+
+void ArcGpuVideoDecodeAccelerator::Reset() {
+  base::RunLoop loop;
+  reset_done_callback_ = loop.QuitClosure();
+  vda_->Reset();
+  base::MessageLoop::ScopedNestableTaskAllower allow(
+      base::MessageLoop::current());
+  // Wait for the ResetDone callback.
+  loop.Run();
+}
+
+void ArcGpuVideoDecodeAccelerator::ProvidePictureBuffers(
+    size_t requested_num_of_buffers,
+    const gfx::Size& dimensions,
+    uint32_t texture_target) {
+  coded_size_ = dimensions;
+  // TODO(owenlin): Think if we need to handle the pixel format.

[Pawel Osciak, 2016/01/07 09:22:35]

Please use VDA::GetOutputFormat() here.

[Owen Lin, 2016/01/12 09:30:22]

TODO added. 

+  //                It looks like the flexible pixel format is good enough.
+  VideoFormat video_format;

[Pawel Osciak, 2016/01/07 09:22:35]

How are we handling image_size ?

[Owen Lin, 2016/01/12 09:30:22]

We use it to request buffers from NativeWindow.

+  video_format.min_num_buffers = requested_num_of_buffers;
+  video_format.coded_width = dimensions.width();
+  video_format.coded_height = dimensions.height();
+  arc_client_->OnOutputFormatChanged(video_format);
+}
+
+void ArcGpuVideoDecodeAccelerator::PictureReady(const media::Picture& picture) {
+  BufferInfo* output_buffer =
+      GetBufferInfo(PORT_OUTPUT, picture.picture_buffer_id());
+  CHECK_NE(output_buffer, nullptr);

[Pawel Osciak, 2016/01/07 09:22:35]

I know this shouldn't happen, but I'm wondering if we should crash ourselves
anyway. I'd prefer NotifyError(PLATFORM_FAILURE).

[Owen Lin, 2016/01/12 09:30:22]

Done.

+
+  // Empty buffer, returned in Flushing.
+  if (picture.bitstream_buffer_id() == -1) {
+    output_buffer->state = OWNED_BY_US;
+  } else {
+    BufferMetadata metadata;
+    uint32_t index = 0;
+    GetInputRecord(picture.bitstream_buffer_id(), &index, &metadata.timestamp);

[Pawel Osciak, 2016/01/07 09:22:36]

Could we use picture.picture_buffer_id as a key to get index? Then we wouldn't
fail returning the correct buffer index, even if metadata was not found for some
reason.

[Owen Lin, 2016/01/12 09:30:22]

Sorry, I didn't get you. Here we don't use the |index| at all. What' we need is
the timestamp. And it was originally associated with the bitstream buffer.

+    output_buffer->state = OWNED_BY_CLIENT;
+    arc_client_->OnBufferDone(PORT_OUTPUT, picture.picture_buffer_id(),

[Pawel Osciak, 2016/01/07 09:22:35]

should this be s/picture.picture_buffer_id()/index/ ?

[Owen Lin, 2016/01/12 09:30:22]

No, GetInputRecord() returns the information about the bitstream. It's the
buffer index we used for the bitstream.

+                              metadata);
+  }
+}
+
+void ArcGpuVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer(
+    int32_t bitstream_buffer_id) {
+  int64_t timestamp = 0;
+  uint32_t index = 0;
+  GetInputRecord(bitstream_buffer_id, &index, &timestamp);
+  BufferInfo* buffer_info = GetBufferInfo(PORT_OUTPUT, index);
+  CHECK(buffer_info);

[Pawel Osciak, 2016/01/07 09:22:35]

Perhaps NotifyError(PLATFORM_FAILURE).

[Owen Lin, 2016/01/12 09:30:22]

Done.

+  buffer_info->state = OWNED_BY_CLIENT;
+  arc_client_->OnBufferDone(PORT_INPUT, bitstream_buffer_id, BufferMetadata());

[Pawel Osciak, 2016/01/07 09:22:35]

should this be s/bitstream_buffer_id/index/ ?

[Owen Lin, 2016/01/12 09:30:23]

Yes, it is. Thanks.

+}
+
+void ArcGpuVideoDecodeAccelerator::NotifyFlushDone() {
+  pending_eos_output_buffer_ = true;
+  std::vector<BufferInfo>* buffers = &port_info_[PORT_OUTPUT].buffers;
+  for (size_t i = 0, n = buffers->size(); i < n; ++i) {
+    BufferInfo* info = &buffers->at(i);
+    if (info->state == OWNED_BY_US)
+      SendEosIfNeededOrReusePicture(i, info);
+  }
+}
+
+void ArcGpuVideoDecodeAccelerator::NotifyResetDone() {
+  base::ResetAndReturn(&reset_done_callback_).Run();
+}
+
+static ArcVideoAccelerator::Error ConvertErrorCode(
+    media::VideoDecodeAccelerator::Error error) {
+#define CASE(t)                          \

[Pawel Osciak, 2016/01/07 09:22:35]

To be honest, I'd prefer not having this macro please. It makes code shorter,
but a bit more difficult to understand/read for others.

[Owen Lin, 2016/01/12 09:30:23]

Done.

+  case media::VideoDecodeAccelerator::t: \
+    return ArcVideoAccelerator::t
+  switch (error) {
+    CASE(ILLEGAL_STATE);
+    CASE(INVALID_ARGUMENT);
+    CASE(UNREADABLE_INPUT);
+    CASE(PLATFORM_FAILURE);
+    default:
+      NOTREACHED() << "Unknown error: " << error;
+  }
+#undef CASE
+  return static_cast<ArcVideoAccelerator::Error>(error);
+}
+
+void ArcGpuVideoDecodeAccelerator::NotifyError(
+    media::VideoDecodeAccelerator::Error error) {
+  arc_client_->OnError(ConvertErrorCode(error));
+}
+
+void ArcGpuVideoDecodeAccelerator::SendEosIfNeededOrReusePicture(
+    uint32_t index,
+    BufferInfo* info) {
+  DCHECK_EQ(info->state, OWNED_BY_US);
+  if (pending_eos_output_buffer_) {
+    BufferMetadata metadata;
+    metadata.flags = BUFFER_FLAG_EOS;
+    info->state = OWNED_BY_CLIENT;
+    arc_client_->OnBufferDone(PORT_OUTPUT, index, metadata);
+    pending_eos_output_buffer_ = false;
+  } else {
+    info->state = OWNED_BY_VDA;
+    vda_->ReusePictureBuffer(index);
+  }
+}
+
+void ArcGpuVideoDecodeAccelerator::SetInputRecord(int32_t bitstream_buffer_id,
+                                                  uint32_t index,
+                                                  int64_t timestamp) {
+  input_records_.push_front(InputRecord(bitstream_buffer_id, index, timestamp));
+  // The value is copied from media::GpuVideoDecoder.
+  const size_t kMaxInputBufferDataSize = 128;

[Pawel Osciak, 2016/01/07 09:22:35]

In GVD we can get a theoretically unspecified number of input buffers, so we
can't put a cap on the number of input buffers waiting to be decoded. However
here we cannot get more buffers than we have valid index values. Would it be
possible to instead just have a fixed vector (size of allowed index values for
inputs?) of these?

[Owen Lin, 2016/01/12 09:30:23]

I think you're right, I will use the BufferInfo for this.

+  if (input_records_.size() > kMaxInputBufferDataSize)
+    input_records_.pop_back();
+}
+
+void ArcGpuVideoDecodeAccelerator::GetInputRecord(int32_t bitstream_buffer_id,
+                                                  uint32_t* index,
+                                                  int64_t* timestamp) {
+  for (auto& data : input_records_) {
+    if (data.bitstream_buffer_id == bitstream_buffer_id) {
+      *index = data.index;
+      *timestamp = data.timestamp;
+      return;
+    }
+  }
+  NOTREACHED() << "Missing bitstream_buffer_id: " << bitstream_buffer_id;
+}
+
+}  // arc
+}  // content