Implement media::VideoDecodeAccelerator on Mac

content/common/gpu/media/mac_video_decode_accelerator.mm

+// 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 "content/common/gpu/media/mac_video_decode_accelerator.h"
+
+#include "base/bind.h"
+#include "base/file_path.h"
+#import "base/mac/foundation_util.h"
+#import "base/memory/ref_counted_memory.h"
+#import "base/message_loop.h"
+#include "base/location.h"
+#include "base/native_library.h"
+#include "ui/surface/io_surface_support_mac.h"
+#include "ui/gfx/video_decode_acceleration_support_mac.h"
+
+namespace {
+
+enum { kNumPictureBuffers = 4 };
+
+class ScopedContextSetter {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

This seems like kind of overkill considering you use it exactly once...
(esp. given you need did_succeed())

[sail, 2012/05/23 23:02:29]

I agree that this is pretty verbose but I really like the simplicity.

[Ami GONE FROM CHROMIUM, 2012/05/24 21:30:29]

"simplicity"
I don't think that word means what you think it means ;)

+ public:
+  ScopedContextSetter(CGLContextObj context)
+      : old_context_(NULL),
+        did_succeed_(false) {
+    old_context_ = CGLGetCurrentContext();
+    did_succeed_ = CGLSetCurrentContext(context) == kCGLNoError;
+  }
+
+  ~ScopedContextSetter() {
+    if (did_succeed_)
+      CGLSetCurrentContext(old_context_);
+  }
+
+  bool did_succeed() const {
+    return did_succeed_;
+  }
+
+ private:
+  CGLContextObj old_context_;
+  bool did_succeed_;
+};
+
+}  // namespace
+
+static bool BindImageToTexture(CGLContextObj context,
+                               CVImageBufferRef image,
+                               uint32 texture_id) {
+  ScopedContextSetter scoped_context_setter(context);
+  if (!scoped_context_setter.did_succeed())
+    return false;
+
+  IOSurfaceSupport* io_surface_support = IOSurfaceSupport::Initialize();

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Get & check this before Initialize returns true?

[sail, 2012/05/23 23:02:29]

Done.

+  if (!io_surface_support)
+    return false;
+
+  CFTypeRef io_surface =
+      io_surface_support->CVPixelBufferGetIOSurface(image);
+  if (!io_surface)

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

When can this happen?  Do you want to log something about it?  Do you want to
abort the decode because of it?

Pretty much the same question applies to every unhappy codepath below (early
return from void functions, return false from bool functions).

For an example of what I mean, see the RETURN_ON_{,OMX_}FAILURE macros in OVDA:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/o...

[sail, 2012/05/23 23:02:29]

Done.
Added logging.

+    return false;
+
+  glEnable(GL_TEXTURE_RECTANGLE_ARB);
+  glBindTexture(GL_TEXTURE_RECTANGLE_ARB, texture_id);
+  if (io_surface_support->CGLTexImageIOSurface2D(
+          context,
+          GL_TEXTURE_RECTANGLE_ARB,
+          GL_RGB,
+          io_surface_support->IOSurfaceGetWidth(io_surface),
+          io_surface_support->IOSurfaceGetHeight(io_surface),
+          GL_YCBCR_422_APPLE,
+          GL_UNSIGNED_SHORT_8_8_APPLE,
+          io_surface,
+          0) != kCGLNoError) {
+    return false;
+  }
+  glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Why this line & the next?

[sail, 2012/05/23 23:02:29]

Hm.. it looks like we don't normally clear this in the chrome code. Removed.

+  glDisable(GL_TEXTURE_RECTANGLE_ARB);
+  if (glGetError() != GL_NO_ERROR)

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

if (foo != bar)
  return false;
return true;

is more concisely written as:
return foo == bar;

[sail, 2012/05/23 23:02:29]

Done.

+    return false;
+
+  return true;
+}
+
+MacVideoDecodeAccelerator::MacVideoDecodeAccelerator(
+    media::VideoDecodeAccelerator::Client* client)
+    : client_(client),
+      cgl_context_(NULL),
+      nalu_len_field_size_(0),
+      frame_width_(0),
+      frame_height_(0),
+      did_request_pictures_(false) {
+}
+
+void MacVideoDecodeAccelerator::SetGLContext(void* gl_context) {
+  cgl_context_ = static_cast<CGLContextObj>(gl_context);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Why not type the param explicitly and avoid the cast here?

[sail, 2012/05/23 23:02:29]

gfx::GLContext::GetHandle() returns void*. It seems better to do the cast in
this platform specific file then in GpuVideoDecodeAccelerator which is cross
platform.

[Ami GONE FROM CHROMIUM, 2012/05/24 21:30:29]

I disagree b/c the call-site is not cross-platform (it is in a platform-specific
#ifdef).  In fact the call-site is clearly cross-platform since it calls *this*
file, which *is* platform specific :)

[sail, 2012/05/29 03:45:01]

Done.

+}
+
+bool MacVideoDecodeAccelerator::SetConfigInfo(

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

This at least needs a TODO to support mid-stream frame size changes (and test
that failure is graceful, not resulting in ASAN violations & security bugs).

[sail, 2012/05/23 23:02:29]

I'm not sure if this config info is related to mid-stream frame size changes.

+    uint32_t frame_width,
+    uint32_t frame_height,
+    const std::vector<uint8_t>& avc_data) {
+  frame_width_ = frame_width;
+  frame_height_ = frame_height;
+  nalu_len_field_size_ = (avc_data[4] & 0x03) + 1;
+
+  DCHECK(!vda_.get());
+  vda_ = new gfx::VideoDecodeAccelerationSupport();
+  gfx::VideoDecodeAccelerationSupport::Status status = vda_->Create(
+      frame_width_, frame_height_,
+      kCVPixelFormatType_422YpCbCr8, &avc_data.front(), avc_data.size());
+  if (status != gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS)

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

That's a funny name for the enum given it's already scoped inside VDAS.  I'd
drop the VDA_.

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

again shorter/clearer as:
return status == gfx::VideoDecodeAccelerationSupport::VDA_SUCCESS;

[sail, 2012/05/23 23:02:29]

Done.

[sail, 2012/05/23 23:02:29]

Done.

+    return false;
+  return true;
+}
+
+bool MacVideoDecodeAccelerator::Initialize(media::VideoCodecProfile profile) {
+  if (client_)
+    client_->NotifyInitializeDone();

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

IIRC this can't be done in-line b/c of reentrancy in the caller. 
ML::current()->PostTask() it instead.

[sail, 2012/05/23 23:02:29]

Done.

+  return true;
+}
+
+void MacVideoDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer) {
+  base::SharedMemory memory(bitstream_buffer.handle(), true);
+  if (!memory.Map(bitstream_buffer.size())) {
+    CHECK(false);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

This isn't very cool ;)

[sail, 2012/05/23 23:02:29]

Done.

+    return;
+  }
+
+  size_t buffer_size = bitstream_buffer.size();
+  if (buffer_size < nalu_len_field_size_ + 1) {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Why so forgiving?  Shouldn't this be an invalid-stream error?

[sail, 2012/05/23 23:02:29]

Done.

+    if (client_)
+      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
+    return;
+  }
+
+  // The decoder can only handle slice types (1-5).

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Drop the parens?

[sail, 2012/05/23 23:02:29]

Done.

+  const uint8_t* buffer = static_cast<const uint8_t*>(memory.memory());
+  uint8_t nalu_type = buffer[nalu_len_field_size_] & 0x1f;
+  if (nalu_type < 1 || nalu_type > 5) {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

are you sure about these bounds?  H264Parser claims these are the slice types:

    kPSlice = 0,
    kBSlice = 1,
    kISlice = 2,
    kSPSlice = 3,
    kSISlice = 4,

[sail, 2012/05/23 23:02:29]

Yea, the bounds are correct. 0 is unspecified which the Mac decoder rejects.

+    if (client_)
+      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
+    return;
+  }
+
+  // Keep a ref counted copy of the buffer.
+  std::vector<uint8_t> vector(buffer, buffer + buffer_size);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

That's quite the variable name....  vbuffer?

[sail, 2012/05/23 23:02:29]

Done.

+  scoped_refptr<base::RefCountedBytes> bytes(
+      base::RefCountedBytes::TakeVector(&vector));
+
+  // Store the buffer size at the beginning of the buffer as the decoder
+  // expects.
+  size_t frame_buffer_size = buffer_size - nalu_len_field_size_;
+  for (size_t i = 0; i < nalu_len_field_size_; ++i) {
+    size_t shift = nalu_len_field_size_ * 8 - (i + 1) * 8;
+    bytes->data()[i] = (frame_buffer_size >> shift) & 0xff;
+  }

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Do you want to have any kind of error checking?  
I.e. that frame_buffer_size actually fits inside nalu_len_field_size_ bytes ?

[sail, 2012/05/23 23:02:29]

Done.

+
+  vda_->Decode(bytes->front(), bytes->size(),
+               base::Bind(&MacVideoDecodeAccelerator::OnFrameReady, this,
+                          bitstream_buffer.id(), bytes));
+
+  if (!did_request_pictures_) {
+    did_request_pictures_ = true;
+    if (client_)
+      client_->ProvidePictureBuffers(
+        kNumPictureBuffers, gfx::Size(frame_width_, frame_height_));
+  }
+}
+
+void MacVideoDecodeAccelerator::AssignPictureBuffers(
+    const std::vector<media::PictureBuffer>& buffers) {
+  available_pictures_.insert(
+      available_pictures_.end(), buffers.begin(), buffers.end());
+  SendImages();
+}
+
+void MacVideoDecodeAccelerator::ReusePictureBuffer(int32 picture_buffer_id) {
+  std::map<int32, PendingPictureInfo>::iterator it =
+      pending_pictures_.find(picture_buffer_id);
+  DCHECK(it != pending_pictures_.end());

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Do you really want to crash the GPU process if a buggy PPAPI plugin provides an
incorrect id?

[sail, 2012/05/23 23:02:29]

Done.

+  PendingPictureInfo info = it->second;
+  pending_pictures_.erase(it);
+  available_pictures_.push_back(info.picture_buffer);
+  SendImages();
+}
+
+void MacVideoDecodeAccelerator::Flush() {
+  vda_->Flush(true);
+  MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
+      &MacVideoDecodeAccelerator::NotifyFlushDone, this));
+}
+
+void MacVideoDecodeAccelerator::Reset() {
+  vda_->Flush(false);
+  MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
+      &MacVideoDecodeAccelerator::NotifyResetDone, this));
+}
+
+void MacVideoDecodeAccelerator::Destroy() {
+  vda_->Destroy();
+  vda_ = NULL;

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Don't all the other entry points want to check for vda_ being NULL before
assuming it isn't? 

[sail, 2012/05/23 23:02:29]

Done.

+  client_ = NULL;
+}
+
+MacVideoDecodeAccelerator::~MacVideoDecodeAccelerator() {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

move to below ctor
call Destroy()?

[sail, 2012/05/23 23:02:29]

I'd rather leave it here to match the declaration order.
Done.

+}
+
+void MacVideoDecodeAccelerator::OnFrameReady(
+    int32 bitstream_buffer_id,
+    scoped_refptr<base::RefCountedBytes> bytes,
+    CVImageBufferRef image,
+    int status) {
+  if (image) {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

how can this fail?

[sail, 2012/05/23 23:02:29]

The decoder can send a NULL image if it failed to decode it for some reason.

+    DecodedImageInfo info;
+    info.image.reset(image, base::mac::RETAIN);
+    info.bitstream_buffer_id = bitstream_buffer_id;
+    decoded_images_.push_back(info);
+    SendImages();
+  }
+  if (client_)
+    client_->NotifyEndOfBitstreamBuffer(bitstream_buffer_id);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

This is wrong if multiple frames are encoded in a single bitstreambuffer.  Does
VDAS need to be extended to notify when a buffer is fully consumed?

[sail, 2012/05/23 23:02:29]

The Mac API can only handle a single NALU at a time so I wrote this code
assuming that each Decode() call only contains a single NALU.

If we go with the CL that uses the content::H265Parser class then it should be
fairly easy to extend this to handle multiple NALUs. Mind if fix this after that
CL has landed.

+}
+
+void MacVideoDecodeAccelerator::SendImages() {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

This sends at most one image, so the name is misleading.
I think if you replace the if at the top of the method with
while (available_pictures_.size() && decoded_images_.size()) 
and put the rest of the function inside that while loop, you'll be good.

[sail, 2012/05/23 23:02:29]

Done.

+  if (available_pictures_.size() == 0 || decoded_images_.size() == 0)
+    return;
+
+  DecodedImageInfo info = decoded_images_.front();
+  decoded_images_.pop_front();
+  media::PictureBuffer picture_buffer = available_pictures_.front();
+
+  if (!BindImageToTexture(cgl_context_, info.image,
+                          picture_buffer.texture_id())) {
+    LOG(ERROR) << "Error binding image to texture.";
+    return;
+  }
+
+  available_pictures_.pop_front();

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

move this up to right below the .front() call?

[sail, 2012/05/23 23:02:29]

Done.

+  PendingPictureInfo pending_info(picture_buffer);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

why use the intermediate?

[sail, 2012/05/23 23:02:29]

Done.

+  pending_info.image = info.image;

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

why not pass both buffer & image to the PPI ctor?
(and make them const in PPI)

[sail, 2012/05/23 23:02:29]

Done.

+  pending_pictures_.insert(std::pair<int, PendingPictureInfo>(

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

std::make_pair lets you drop the template params.

[sail, 2012/05/23 23:02:29]

The Mac compiler doesn't seem to allow this.

+      picture_buffer.id(), pending_info));
+  media::Picture picture(picture_buffer.id(), info.bitstream_buffer_id);
+  if (client_)

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

If !client_ early-return at the top of the function?

[sail, 2012/05/23 23:02:29]

Done.

+    client_->PictureReady(picture);

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

can construct this inline:
client_->PictureReady(media::Picture(picture_buffer.id(),
info.bitstream_buffer_id));
if you like.

[sail, 2012/05/23 23:02:29]

Done.

+}
+
+void MacVideoDecodeAccelerator::NotifyFlushDone() {
+  if (client_)

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

assert decoded_images_ is empty?

[sail, 2012/05/23 23:02:29]

Hm... it's possible that there are no available images so the decoded images
haven't been sent yet. (DXVAVideoDecodeAccelerator has the same issue).

+    client_->NotifyFlushDone();
+}
+
+void MacVideoDecodeAccelerator::NotifyResetDone() {
+  decoded_images_.clear();
+  if (client_)
+    client_->NotifyResetDone();
+}
+
+MacVideoDecodeAccelerator::PendingPictureInfo::PendingPictureInfo(
+    media::PictureBuffer pic)
+    : picture_buffer(pic.id(), pic.size(), pic.texture_id()) {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

what's wrong w/ the copy ctor?

[sail, 2012/05/23 23:02:29]

Done.

+}
+
+MacVideoDecodeAccelerator::PendingPictureInfo::~PendingPictureInfo() {
+}
+
+MacVideoDecodeAccelerator::DecodedImageInfo::DecodedImageInfo() {
+}
+
+MacVideoDecodeAccelerator::DecodedImageInfo::~DecodedImageInfo() {
+}