Implement actually decoding frames in VTVideoDecodeAccelerator.

content/common/gpu/media/vt_video_decode_accelerator.cc

Index: content/common/gpu/media/vt_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/vt_video_decode_accelerator.cc b/content/common/gpu/media/vt_video_decode_accelerator.cc
index 7745e6dfb999aacc259f50033d856d2aded7d28a..386c83fad6dc42ab1084c3f6f19cdc25c4fbefcf 100644
--- a/content/common/gpu/media/vt_video_decode_accelerator.cc
+++ b/content/common/gpu/media/vt_video_decode_accelerator.cc
@@ -17,9 +17,18 @@ using content_common_gpu_media::StubPathMap;
 
 namespace content {
 
-// Size of length headers prepended to NALUs in MPEG-4 framing. (1, 2, or 4.)
+// Size of NALU length headers in AVCC/MPEG-4 format (can be 1, 2, or 4).
 static const int kNALUHeaderLength = 4;
 
+// We only request 5 picture buffers from the client which are used to hold the
+// decoded samples. These buffers are then reused when the client tells us that
+// it is done with the buffer.
+static const int kNumPictureBuffers = 5;
+
+// FOURCC for YCBCR_422_APPLE.
+// TODO(sandersd): RGBA option for 4:4:4 video.
+static const int32_t kYCbCr422 = '2vuy';
+
 // Route decoded frame callbacks back into the VTVideoDecodeAccelerator.
 static void OutputThunk(
     void* decompression_output_refcon,
@@ -29,22 +38,31 @@ static void OutputThunk(
     CVImageBufferRef image_buffer,
     CMTime presentation_time_stamp,
     CMTime presentation_duration) {
+  // TODO(sandersd): Implement flush-before-delete to guarantee validity.
   VTVideoDecodeAccelerator* vda =
       reinterpret_cast<VTVideoDecodeAccelerator*>(decompression_output_refcon);
-  int32_t* bitstream_id_ptr = reinterpret_cast<int32_t*>(source_frame_refcon);
-  int32_t bitstream_id = *bitstream_id_ptr;
-  delete bitstream_id_ptr;
-  CFRetain(image_buffer);
+  intptr_t bitstream_id = reinterpret_cast<intptr_t>(source_frame_refcon);
   vda->Output(bitstream_id, status, info_flags, image_buffer);
 }
 
+VTVideoDecodeAccelerator::DecodedFrame::DecodedFrame(
+    uint32_t bitstream_id,
+    CVImageBufferRef image_buffer)
+    : bitstream_id(bitstream_id),
+      image_buffer(image_buffer) {
+}
+
+VTVideoDecodeAccelerator::DecodedFrame::~DecodedFrame() {
+}
+
 VTVideoDecodeAccelerator::VTVideoDecodeAccelerator(CGLContextObj cgl_context)
     : cgl_context_(cgl_context),
-      client_(NULL),
-      decoder_thread_("VTDecoderThread"),
       format_(NULL),
       session_(NULL),
-      weak_this_factory_(this) {
+      frames_pending_decode_(0),

[scherkus (not reviewing), 2014/07/17 02:08:26]

can you remove this for this CL and add it back in when it's actually used by
Flush()?

it'll make it easier to understand the subsequent review for Flush()

[sandersd (OOO until July 31), 2014/07/17 20:31:46]

Done.

+      gpu_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_this_factory_(this),
+      decoder_thread_("VTDecoderThread") {
   callback_.decompressionOutputCallback = OutputThunk;
   callback_.decompressionOutputRefCon = this;
 }
@@ -56,7 +74,9 @@ bool VTVideoDecodeAccelerator::Initialize(
     media::VideoCodecProfile profile,
     Client* client) {
   DCHECK(CalledOnValidThread());
-  client_ = client;
+
+  weak_client_factory_.reset(
+      new base::WeakPtrFactory<media::VideoDecodeAccelerator::Client>(client));
 
   // Only H.264 is supported.
   if (profile < media::H264PROFILE_MIN || profile > media::H264PROFILE_MAX)
@@ -88,6 +108,9 @@ bool VTVideoDecodeAccelerator::Initialize(
 void VTVideoDecodeAccelerator::ConfigureDecoder(
     const std::vector<const uint8_t*>& nalu_data_ptrs,
     const std::vector<size_t>& nalu_data_sizes) {
+  DCHECK(decoder_thread_.message_loop_proxy()->BelongsToCurrentThread());
+  // Construct a new format description from the parameter sets.
+  // TODO(sandersd): Replace this with custom code to support OS X < 10.9.
   format_.reset();
   CHECK(!CMVideoFormatDescriptionCreateFromH264ParameterSets(
       kCFAllocatorDefault,
@@ -95,13 +118,11 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
       &nalu_data_ptrs.front(),    // &parameter_set_pointers
       &nalu_data_sizes.front(),   // &parameter_set_sizes
       kNALUHeaderLength,          // nal_unit_header_length
-      format_.InitializeInto()
-  ));
-
-  // TODO(sandersd): Check if the size has changed and handle picture requests.
-  CMVideoDimensions coded_size = CMVideoFormatDescriptionGetDimensions(format_);
-  coded_size_.SetSize(coded_size.width, coded_size.height);
+      format_.InitializeInto()));
+  CMVideoDimensions coded_dimensions =
+      CMVideoFormatDescriptionGetDimensions(format_);
 
+  // Prepare VideoToolbox configuration dictionaries.
   base::ScopedCFTypeRef<CFMutableDictionaryRef> decoder_config(
       CFDictionaryCreateMutable(
           kCFAllocatorDefault,
@@ -122,12 +143,10 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
           &kCFTypeDictionaryKeyCallBacks,
           &kCFTypeDictionaryValueCallBacks));
 
-  // TODO(sandersd): ARGB for video that is not 4:2:0.
-  int32_t pixel_format = '2vuy';
 #define CFINT(i) CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &i)
-  base::ScopedCFTypeRef<CFNumberRef> cf_pixel_format(CFINT(pixel_format));
-  base::ScopedCFTypeRef<CFNumberRef> cf_width(CFINT(coded_size.width));
-  base::ScopedCFTypeRef<CFNumberRef> cf_height(CFINT(coded_size.height));
+  base::ScopedCFTypeRef<CFNumberRef> cf_pixel_format(CFINT(kYCbCr422));
+  base::ScopedCFTypeRef<CFNumberRef> cf_width(CFINT(coded_dimensions.width));
+  base::ScopedCFTypeRef<CFNumberRef> cf_height(CFINT(coded_dimensions.height));
 #undef CFINT
   CFDictionarySetValue(
       image_config, kCVPixelBufferPixelFormatTypeKey, cf_pixel_format);
@@ -136,8 +155,8 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
   CFDictionarySetValue(
       image_config, kCVPixelBufferOpenGLCompatibilityKey, kCFBooleanTrue);
 
-  // TODO(sandersd): Skip if the session is compatible.
-  // TODO(sandersd): Flush frames when resetting.
+  // TODO(sandersd): Check if the session is already compatible.
+  // TODO(sandersd): Flush.
   session_.reset();
   CHECK(!VTDecompressionSessionCreate(
       kCFAllocatorDefault,
@@ -145,9 +164,20 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
       decoder_config,       // video_decoder_specification
       image_config,         // destination_image_buffer_attributes
       &callback_,           // output_callback
-      session_.InitializeInto()
-  ));
-  DVLOG(2) << "Created VTDecompressionSession";
+      session_.InitializeInto()));
+
+  // If the size has changed, request new picture buffers.
+  gfx::Size new_coded_size(coded_dimensions.width, coded_dimensions.height);
+  if (coded_size_ != new_coded_size) {
+    coded_size_ = new_coded_size;
+    // TODO(sandersd): Dismiss existing picture buffers.
+    gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
+        &media::VideoDecodeAccelerator::Client::ProvidePictureBuffers,
+        weak_client_factory_->GetWeakPtr(),
+        kNumPictureBuffers,
+        coded_size_,
+        GL_TEXTURE_RECTANGLE_ARB));
+  }
 }
 
 void VTVideoDecodeAccelerator::Decode(const media::BitstreamBuffer& bitstream) {
@@ -167,7 +197,12 @@ void VTVideoDecodeAccelerator::DecodeTask(
   CHECK(memory.Map(size));
   const uint8_t* buf = static_cast<uint8_t*>(memory.memory());
 
-  // Locate relevant NALUs in the buffer.
+  // NALUs are stored with Annex B format in the bitstream buffer (3-byte start
+  // codes), but VideoToolbox expects AVCC/MPEG-4 format (length headers), so we
+  // must to rewrite the data.
+  //
+  // 1. Locate relevant NALUs and compute the size of the translated data.
+  //    Also record any parameter sets for VideoToolbox initialization.
   size_t data_size = 0;
   std::vector<media::H264NALU> nalus;
   std::vector<const uint8_t*> config_nalu_data_ptrs;
@@ -179,21 +214,81 @@ void VTVideoDecodeAccelerator::DecodeTask(
     if (result == media::H264Parser::kEOStream)
       break;
     CHECK_EQ(result, media::H264Parser::kOk);
+    // TODO(sandersd): Check that these are only at the start.
     if (nalu.nal_unit_type == media::H264NALU::kSPS ||
         nalu.nal_unit_type == media::H264NALU::kPPS ||
         nalu.nal_unit_type == media::H264NALU::kSPSExt) {
+      DVLOG(2) << "Parameter set " << nalu.nal_unit_type;
       config_nalu_data_ptrs.push_back(nalu.data);
       config_nalu_data_sizes.push_back(nalu.size);
+    } else {
+      nalus.push_back(nalu);
+      data_size += kNALUHeaderLength + nalu.size;
     }
-    nalus.push_back(nalu);
-    // Each NALU will have a 4-byte length header prepended.
-    data_size += kNALUHeaderLength + nalu.size;
   }
 
-  if (!config_nalu_data_ptrs.empty())
+  // 2. Initialize VideoToolbox.
+  // TODO(sandersd): Reinitialize when there are new parameter sets.
+  if (!session_)
     ConfigureDecoder(config_nalu_data_ptrs, config_nalu_data_sizes);
 
-  // TODO(sandersd): Rewrite slice NALU headers and send for decoding.
+  // 3. Allocate a memory-backed CMBlockBuffer for the translated data.
+  base::ScopedCFTypeRef<CMBlockBufferRef> data;
+  CHECK(!CMBlockBufferCreateWithMemoryBlock(
+      kCFAllocatorDefault,
+      NULL,                 // &memory_block
+      data_size,            // block_length
+      kCFAllocatorDefault,  // block_allocator
+      NULL,                 // &custom_block_source
+      0,                    // offset_to_data
+      data_size,            // data_length
+      0,                    // flags
+      data.InitializeInto()));
+
+  // 4. Copy NALU data, inserting length headers.
+  size_t offset = 0;
+  for (size_t i = 0; i < nalus.size(); i++) {
+    media::H264NALU& nalu = nalus[i];
+    uint8_t header[4] = {0xff & nalu.size >> 24,
+                         0xff & nalu.size >> 16,
+                         0xff & nalu.size >> 8,
+                         0xff & nalu.size};
+    CHECK(!CMBlockBufferReplaceDataBytes(header, data, offset, 4));
+    offset += 4;
+    CHECK(!CMBlockBufferReplaceDataBytes(nalu.data, data, offset, nalu.size));
+    offset += nalu.size;
+  }
+
+  // 5. Package the data for VideoToolbox and request decoding.
+  base::ScopedCFTypeRef<CMSampleBufferRef> frame;
+  CHECK(!CMSampleBufferCreate(
+      kCFAllocatorDefault,
+      data,                 // data_buffer
+      true,                 // data_ready
+      NULL,                 // make_data_ready_callback
+      NULL,                 // make_data_ready_refcon
+      format_,              // format_description
+      1,                    // num_samples
+      0,                    // num_sample_timing_entries
+      NULL,                 // &sample_timing_array
+      0,                    // num_sample_size_entries
+      NULL,                 // &sample_size_array
+      frame.InitializeInto()));
+
+  VTDecodeFrameFlags decode_flags =
+      kVTDecodeFrame_EnableAsynchronousDecompression |
+      kVTDecodeFrame_EnableTemporalProcessing;
+
+  intptr_t bitstream_id = bitstream.id();
+  CHECK(!VTDecompressionSessionDecodeFrame(
+      session_,
+      frame,                                  // sample_buffer
+      decode_flags,                           // decode_flags
+      reinterpret_cast<void*>(bitstream_id),  // source_frame_refcon
+      NULL));                                 // &info_flags_out
+
+  base::AutoLock lock(lock_);
+  frames_pending_decode_++;
 }
 
 // This method may be called on any VideoToolbox thread.
@@ -202,17 +297,101 @@ void VTVideoDecodeAccelerator::Output(
     OSStatus status,
     VTDecodeInfoFlags info_flags,
     CVImageBufferRef image_buffer) {
-  // TODO(sandersd): Store the frame in a queue.
-  CFRelease(image_buffer);
+  // The info_flags are ignored for now as they seem to be corrupted when the
+  // software decoder is selected.

[scherkus (not reviewing), 2014/07/17 02:08:26]

this comment is confusing to me ... is this a TODO? should be reading the
info_flags? if so, what should we be doing it if it wasn't corrupted? when/why
are we selecting the software decoder?

[sandersd (OOO until July 31), 2014/07/17 20:31:46]

The only bit of interest in info_flags is the frame dropped bit, but we don't
expect any frames to be dropped (and I suspect that image_buffer will be of type
CFNull in that case).

The software decoder is selected for us automatically if the hardware can't be
initialized or does not support the parameters of the video.

+  CHECK(!status);
+  CHECK(CFGetTypeID(image_buffer) == CVPixelBufferGetTypeID());

[scherkus (not reviewing), 2014/07/17 02:08:25]

nit: is it possible to CHECK_EQ() these two, or do we get type errors?

[sandersd (OOO until July 31), 2014/07/17 20:31:46]

Done.

+  CHECK(CVPixelBufferGetPixelFormatType(image_buffer) == kYCbCr422);
+  {
+    base::AutoLock lock(lock_);
+    frames_pending_decode_--;
+  }
+  gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &VTVideoDecodeAccelerator::OutputTask,
+      weak_this_factory_.GetWeakPtr(),
+      bitstream_id,
+      base::ScopedCFTypeRef<CVImageBufferRef>(
+          image_buffer, base::scoped_policy::RETAIN)));
+}
+
+void VTVideoDecodeAccelerator::OutputTask(
+    int32_t bitstream_id,
+    base::ScopedCFTypeRef<CVImageBufferRef> image_buffer) {
+  DCHECK(CalledOnValidThread());
+  decoded_frames_.push(DecodedFrame(bitstream_id, image_buffer));
+  SendPictures();
 }
 
 void VTVideoDecodeAccelerator::AssignPictureBuffers(
     const std::vector<media::PictureBuffer>& pictures) {
   DCHECK(CalledOnValidThread());
+
+  for (size_t i = 0; i < pictures.size(); i++) {
+    picture_ids_.push(pictures[i].id());
+    texture_ids_[pictures[i].id()] = pictures[i].texture_id();
+  }
+
+  // Pictures are not marked as uncleared until this method returns. They will
+  // become broken if they are used before that happens.
+  gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &VTVideoDecodeAccelerator::SendPictures,
+      weak_this_factory_.GetWeakPtr()));
 }
 
 void VTVideoDecodeAccelerator::ReusePictureBuffer(int32_t picture_id) {
   DCHECK(CalledOnValidThread());
+
+  picture_bindings_.erase(picture_id);
+  picture_ids_.push(picture_id);
+
+  gpu_task_runner_->PostTask(FROM_HERE, base::Bind(

[scherkus (not reviewing), 2014/07/17 02:08:26]

does this have to be made async? it looks like we should be on the gpu thread
already

if it has to be async a comment or something would be nice, similar to one above
in AssignPictureBuffers() ... unless the PostTask() is also not needed in
AssignPictureBuffers :)

[sandersd (OOO until July 31), 2014/07/17 20:31:46]

Done.

I've tried to avoid any calling back into the host on the same stack, as the
results are not documented, but some of these are indeed trivial.

+      &VTVideoDecodeAccelerator::SendPictures,
+      weak_this_factory_.GetWeakPtr()));
+}
+
+void VTVideoDecodeAccelerator::SendPictures() {
+  DCHECK(CalledOnValidThread());
+  if (!picture_ids_.empty() && !decoded_frames_.empty()) {

[scherkus (not reviewing), 2014/07/17 02:08:25]

nit: prefer early return when possible so we avoid deeply nesting code

if (picture_ids_.empty() || decoded_frames_.empty())
  return;

[sandersd (OOO until July 31), 2014/07/17 20:31:46]

Done.

+    CGLContextObj prev_context = CGLGetCurrentContext();
+    CHECK(!CGLSetCurrentContext(cgl_context_));
+    glEnable(GL_TEXTURE_RECTANGLE_ARB);
+
+    while (!picture_ids_.empty() && !decoded_frames_.empty()) {
+      int32_t picture_id = picture_ids_.front();
+      picture_ids_.pop();
+      DecodedFrame frame = decoded_frames_.front();
+      decoded_frames_.pop();
+      IOSurfaceRef surface = CVPixelBufferGetIOSurface(frame.image_buffer);
+
+      glBindTexture(GL_TEXTURE_RECTANGLE_ARB, texture_ids_[picture_id]);
+      CHECK(!CGLTexImageIOSurface2D(
+          cgl_context_,                 // ctx
+          GL_TEXTURE_RECTANGLE_ARB,     // target
+          GL_RGB,                       // internal_format
+          coded_size_.width(),          // width
+          coded_size_.height(),         // height
+          GL_YCBCR_422_APPLE,           // format
+          GL_UNSIGNED_SHORT_8_8_APPLE,  // type
+          surface,                      // io_surface
+          0));                          // plane
+      glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);
+
+      picture_bindings_[picture_id] = frame.image_buffer;
+
+      gpu_task_runner_->PostTask(FROM_HERE, base::Bind(

[scherkus (not reviewing), 2014/07/17 02:08:25]

do these need to be async?

we're already on gpu_task_runner_ and should be able to call the client directly

the only time weak_client_factory_ gets invalidated is when our destructor is
running, which should mean it's safe to store a raw VDA::Client* pointer and
call it directly

(if we're somehow executing this code after our dtor has run we've got much
bigger problems to solve!)

[sandersd (OOO until July 31), 2014/07/17 20:31:46]

Done.

It looks like both the Chrome and Pepper implementations are trivial enough to
call directly (but I don't like it because I had to audit the code to be sure).

+          &media::VideoDecodeAccelerator::Client::PictureReady,
+          weak_client_factory_->GetWeakPtr(),
+          media::Picture(picture_id, frame.bitstream_id)));
+
+      gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
+          &media::VideoDecodeAccelerator::Client::NotifyEndOfBitstreamBuffer,
+          weak_client_factory_->GetWeakPtr(),
+          frame.bitstream_id));
+    }
+
+    glDisable(GL_TEXTURE_RECTANGLE_ARB);
+    CHECK(!CGLSetCurrentContext(prev_context));
+  }
 }
 
 void VTVideoDecodeAccelerator::Flush() {