Support configuration changes 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 adc012f2bff9590eeb28b17139de9bfdabc1f4d6..f625fbcbc66728c12d188558dd5c2c6196bbf0f8 100644
--- a/content/common/gpu/media/vt_video_decode_accelerator.cc
+++ b/content/common/gpu/media/vt_video_decode_accelerator.cc
@@ -146,8 +146,12 @@ bool VTVideoDecodeAccelerator::ConfigureDecoder(
                   status);
     return false;
   }
-  CMVideoDimensions coded_dimensions =
-      CMVideoFormatDescriptionGetDimensions(format_);
+
+  // If the session is compatible, there's nothing to do.
+  if (session_ &&
+      VTDecompressionSessionCanAcceptFormatDescription(session_, format_)) {
+      return true;
+  }
 
   // Prepare VideoToolbox configuration dictionaries.
   base::ScopedCFTypeRef<CFMutableDictionaryRef> decoder_config(
@@ -170,6 +174,8 @@ bool VTVideoDecodeAccelerator::ConfigureDecoder(
           &kCFTypeDictionaryKeyCallBacks,
           &kCFTypeDictionaryValueCallBacks));
 
+  CMVideoDimensions coded_dimensions =
+      CMVideoFormatDescriptionGetDimensions(format_);
 #define CFINT(i) CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &i)
   // TODO(sandersd): RGBA option for 4:4:4 video.
   int32_t pixel_format = kCVPixelFormatType_422YpCbCr8;
@@ -184,7 +190,7 @@ bool VTVideoDecodeAccelerator::ConfigureDecoder(
   CFDictionarySetValue(
       image_config, kCVPixelBufferOpenGLCompatibilityKey, kCFBooleanTrue);
 
-  // TODO(sandersd): Check if the session is already compatible.
+  // TODO(sandersd): Does the old session need to be flushed first?
   session_.reset();
   status = VTDecompressionSessionCreate(
       kCFAllocatorDefault,
@@ -198,18 +204,6 @@ bool VTVideoDecodeAccelerator::ConfigureDecoder(
     return false;
   }
 
-  // If the size has changed, trigger a request for new picture buffers.
-  // TODO(sandersd): Move to SendPictures(), and use this just as a hint for an
-  // upcoming size change.
-  gfx::Size new_coded_size(coded_dimensions.width, coded_dimensions.height);
-  if (coded_size_ != new_coded_size) {
-    coded_size_ = new_coded_size;
-    gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
-        &VTVideoDecodeAccelerator::SizeChangedTask,
-        weak_this_factory_.GetWeakPtr(),
-        coded_size_));;
-  }
-
   return true;
 }
 
@@ -256,10 +250,9 @@ void VTVideoDecodeAccelerator::DecodeTask(
   //
   // 1. Locate relevant NALUs and compute the size of the translated data.
   //    Also record any parameter sets for VideoToolbox initialization.
+  bool config_changed = false;
   size_t data_size = 0;
   std::vector<media::H264NALU> nalus;
-  std::vector<const uint8_t*> config_nalu_data_ptrs;
-  std::vector<size_t> config_nalu_data_sizes;
   parser_.SetStream(buf, size);
   media::H264NALU nalu;
   while (true) {
@@ -271,24 +264,52 @@ void VTVideoDecodeAccelerator::DecodeTask(
       NotifyError(PLATFORM_FAILURE);
       return;
     }
-    // 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;
+    // TODO(sandersd): Strict ordering rules.
+    switch (nalu.nal_unit_type) {
+      case media::H264NALU::kSPS:
+        last_sps_.assign(nalu.data, nalu.data + nalu.size);

[Pawel Osciak, 2014/11/02 23:09:52]

You actually shouldn't have to store SPS/PPS. H264Parser keeps all current SPSes
and PPSes cached and you can get them via GetSPS(id) GetPPS(id). The parser will
also already verify for you that i.e. PPS refers to a valid SPS, so you don't
have to check ordering or worry that the last PPS and SPS is the one you want.

[sandersd (OOO until July 31), 2014/11/03 21:14:25]

But GetSPS()/GetPPS() return the parsed structure, not the original bytes, which
are what need to go in the avcC record.

+        last_spsext_.clear();
+        config_changed = true;
+        break;
+      case media::H264NALU::kSPSExt:
+        // TODO(sandersd): Check that the previous NALU was an SPS.
+        last_spsext_.assign(nalu.data, nalu.data + nalu.size);
+        config_changed = true;
+        break;
+      case media::H264NALU::kPPS:
+        last_pps_.assign(nalu.data, nalu.data + nalu.size);
+        config_changed = true;
+        break;
+      default:
+        nalus.push_back(nalu);
+        data_size += kNALUHeaderLength + nalu.size;
+        break;
     }
   }
 
   // 2. Initialize VideoToolbox.
-  // TODO(sandersd): Reinitialize when there are new parameter sets.
-  if (!session_) {
-    // If configuring fails, ConfigureDecoder() already called NotifyError().
-    if (!ConfigureDecoder(config_nalu_data_ptrs, config_nalu_data_sizes))
+  // TODO(sandersd): Check if the new configuration is identical before
+  // reconfiguring.
+  if (config_changed) {
+    if (last_sps_.size() == 0 || last_pps_.size() == 0) {
+      LOG(ERROR) << "Invalid configuration data";
+      NotifyError(INVALID_ARGUMENT);
+      return;
+    }
+    // TODO(sandersd): Check that the SPS and PPS IDs match.
+    std::vector<const uint8_t*> nalu_data_ptrs;
+    std::vector<size_t> nalu_data_sizes;
+    nalu_data_ptrs.push_back(&last_sps_.front());
+    nalu_data_sizes.push_back(last_sps_.size());
+    if (last_spsext_.size() != 0) {
+      nalu_data_ptrs.push_back(&last_spsext_.front());
+      nalu_data_sizes.push_back(last_spsext_.size());
+    }
+    nalu_data_ptrs.push_back(&last_pps_.front());
+    nalu_data_sizes.push_back(last_pps_.size());
+
+    // If ConfigureDecoder() fails, it already called NotifyError().
+    if (!ConfigureDecoder(nalu_data_ptrs, nalu_data_sizes))
       return;
   }
 
@@ -298,7 +319,15 @@ void VTVideoDecodeAccelerator::DecodeTask(
   if (!data_size)
     return;
 
+  // If the session is not configured, fail.
+  if (!session_) {
+    LOG(ERROR) << "Image slice without configuration data";
+    NotifyError(INVALID_ARGUMENT);
+    return;
+  }
+
   // 3. Allocate a memory-backed CMBlockBuffer for the translated data.
+  // TODO(sandersd): Check that the slice's PPS matches the current PPS.

[Pawel Osciak, 2014/11/02 23:09:52]

A slice doesn't have to refer to the latest PPS afair. Otherwise it wouldn't
make sense to allow more than one pps id in the stream.

[sandersd (OOO until July 31), 2014/11/03 21:14:25]

Acknowledged.

   base::ScopedCFTypeRef<CMBlockBufferRef> data;
   OSStatus status = CMBlockBufferCreateWithMemoryBlock(
       kCFAllocatorDefault,
@@ -387,7 +416,7 @@ void VTVideoDecodeAccelerator::Output(
     // TODO(sandersd): Handle dropped frames.
     NOTIFY_STATUS("Decoding", status);
     image_buffer = NULL;
-  } else  if (CFGetTypeID(image_buffer) != CVPixelBufferGetTypeID()) {
+  } else if (CFGetTypeID(image_buffer) != CVPixelBufferGetTypeID()) {
     LOG(ERROR) << "Decoded frame is not a CVPixelBuffer";
     NotifyError(PLATFORM_FAILURE);
     image_buffer = NULL;
@@ -406,21 +435,14 @@ void VTVideoDecodeAccelerator::OutputTask(DecodedFrame frame) {
   ProcessDecodedFrames();
 }
 
-void VTVideoDecodeAccelerator::SizeChangedTask(gfx::Size coded_size) {
-  DCHECK(CalledOnValidThread());
-  texture_size_ = coded_size;
-  // TODO(sandersd): Dismiss existing picture buffers.
-  client_->ProvidePictureBuffers(
-      kNumPictureBuffers, texture_size_, GL_TEXTURE_RECTANGLE_ARB);
-}
-
 void VTVideoDecodeAccelerator::AssignPictureBuffers(
     const std::vector<media::PictureBuffer>& pictures) {
   DCHECK(CalledOnValidThread());
 
   for (size_t i = 0; i < pictures.size(); i++) {
     DCHECK(!texture_ids_.count(pictures[i].id()));
-    available_picture_ids_.push(pictures[i].id());
+    assigned_picture_ids_.insert(pictures[i].id());
+    available_picture_ids_.push_back(pictures[i].id());
     texture_ids_[pictures[i].id()] = pictures[i].texture_id();
   }
 
@@ -436,12 +458,16 @@ void VTVideoDecodeAccelerator::ReusePictureBuffer(int32_t picture_id) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(CFGetRetainCount(picture_bindings_[picture_id]), 1);
   picture_bindings_.erase(picture_id);
-  available_picture_ids_.push(picture_id);
-  ProcessDecodedFrames();
+  // Don't put the picture back in the available list if has been dismissed.
+  if (assigned_picture_ids_.count(picture_id) != 0) {
+    available_picture_ids_.push_back(picture_id);
+    ProcessDecodedFrames();
+  }
 }
 
 void VTVideoDecodeAccelerator::CompleteAction(Action action) {
   DCHECK(CalledOnValidThread());
+
   switch (action) {
     case ACTION_FLUSH:
       client_->NotifyFlushDone();
@@ -526,6 +552,54 @@ void VTVideoDecodeAccelerator::ProcessDecodedFrames() {
   }
 }
 
+int32_t VTVideoDecodeAccelerator::ProcessDroppedFrames(
+    int32_t last_sent_bitstream_id,
+    int32_t up_to_bitstream_id) {
+  DCHECK(CalledOnValidThread());
+  // Drop frames as long as there is a frame, we have not reached the next
+  // action, and the next frame has no image.
+  while (!decoded_frames_.empty() &&
+         last_sent_bitstream_id != up_to_bitstream_id &&
+         decoded_frames_.front().image_buffer.get() == NULL) {

[Pawel Osciak, 2014/11/02 23:09:52]

Couldn't we just do this in SendPictures? You could check for
decoded_frames_.front().image_buffer.get() == NULL
and not get the surface then, just return everything like here?

[sandersd (OOO until July 31), 2014/11/03 21:14:25]

That's true. The purpose of separating them is that it makes the size change
logic much simpler, without waiting for picture buffers that we don't actually
need. (It also avoids unnecessary size changes and GL context switches, but I
don't consider that important.)

+    const DecodedFrame& frame = decoded_frames_.front();
+    DCHECK_EQ(pending_bitstream_ids_.front(), frame.bitstream_id);
+    client_->NotifyEndOfBitstreamBuffer(frame.bitstream_id);
+    last_sent_bitstream_id = frame.bitstream_id;
+    decoded_frames_.pop();
+    pending_bitstream_ids_.pop();
+  }
+  return last_sent_bitstream_id;
+}
+
+// TODO(sandersd): If GpuVideoDecoder didn't specifically check the size of
+// textures, this would be unnecessary, as the size is actually a property of
+// the texture binding, not the texture. We rebind every frame, so the size
+// passed to ProvidePictureBuffers() is meaningless.
+void VTVideoDecodeAccelerator::ProcessSizeChange() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!decoded_frames_.empty());
+
+  // Find the size of the next image.
+  const DecodedFrame& frame = decoded_frames_.front();
+  CVImageBufferRef image_buffer = frame.image_buffer.get();
+  size_t width = CVPixelBufferGetWidth(image_buffer);
+  size_t height = CVPixelBufferGetHeight(image_buffer);
+  gfx::Size image_size(width, height);
+
+  if (picture_size_ != image_size) {
+    // Dismiss all assigned picture buffers.
+    for (int32_t picture_id : assigned_picture_ids_)
+      client_->DismissPictureBuffer(picture_id);
+    assigned_picture_ids_.clear();
+    available_picture_ids_.clear();
+
+    // Request new pictures.
+    client_->ProvidePictureBuffers(
+        kNumPictureBuffers, image_size, GL_TEXTURE_RECTANGLE_ARB);
+    picture_size_ = image_size;
+  }
+}
+
 int32_t VTVideoDecodeAccelerator::SendPictures(int32_t up_to_bitstream_id) {
   DCHECK(CalledOnValidThread());
   DCHECK(!decoded_frames_.empty());
@@ -533,6 +607,12 @@ int32_t VTVideoDecodeAccelerator::SendPictures(int32_t up_to_bitstream_id) {
   // TODO(sandersd): Store the actual last sent bitstream ID?
   int32_t last_sent_bitstream_id = -1;
 
+  last_sent_bitstream_id =
+      ProcessDroppedFrames(last_sent_bitstream_id, up_to_bitstream_id);
+  if (last_sent_bitstream_id == up_to_bitstream_id || decoded_frames_.empty())
+    return last_sent_bitstream_id;
+
+  ProcessSizeChange();

[Pawel Osciak, 2014/11/02 23:09:52]

This also wouldn't have to be duplicated if we did everything in the while loop?

[sandersd (OOO until July 31), 2014/11/03 21:14:25]

It would also incorrectly wait for a picture buffer before starting the size
change. I admit that this may not matter in practice.

   if (available_picture_ids_.empty())

[Pawel Osciak, 2014/11/02 23:09:52]

Do you have this here to save on making context current if this is empty?
Otherwise the while loop takes care of this.

[sandersd (OOO until July 31), 2014/11/03 21:14:25]

That's correct. This function may get called many times before successfully
sending a frame, but it may not be a necessary optimization.

[Pawel Osciak, 2014/11/07 10:09:38]

I'm not a big fan of this kind of optimizations, but I won't press too much. I
know MCC might be long. But ideally we shouldn't be coming here unless we knew
we could output a picture...

[sandersd (OOO until July 31), 2014/11/07 19:01:11]

The next CL splits this method in two; I'll leave it how it is here for now.

     return last_sent_bitstream_id;
 
@@ -543,52 +623,52 @@ int32_t VTVideoDecodeAccelerator::SendPictures(int32_t up_to_bitstream_id) {
   }
 
   glEnable(GL_TEXTURE_RECTANGLE_ARB);
-  while (!available_picture_ids_.empty() &&
-         !decoded_frames_.empty() &&
-         last_sent_bitstream_id != up_to_bitstream_id &&
-         !has_error_) {
-    // We don't pop |frame| until it is consumed, which won't happen if an
-    // error occurs. Conveniently, this also removes some refcounting.
+  while (!available_picture_ids_.empty() && !has_error_) {
+    DCHECK_NE(last_sent_bitstream_id, up_to_bitstream_id);
+    DCHECK(!decoded_frames_.empty());
+
+    // We don't pop |frame| or |picture_id| until they are consumed, which may
+    // not happen if an error occurs. Conveniently, this also removes some
+    // refcounting.
     const DecodedFrame& frame = decoded_frames_.front();
     DCHECK_EQ(pending_bitstream_ids_.front(), frame.bitstream_id);
-
-    // Likewise, |picture_id| won't be popped if |image_buffer| is NULL or an
-    // error occurs.
-    // TODO(sandersd): Don't block waiting for a |picture_id| when
-    // |image_buffer| is NULL.
-    int32_t picture_id = available_picture_ids_.front();
+    int32_t picture_id = available_picture_ids_.back();
 
     CVImageBufferRef image_buffer = frame.image_buffer.get();
-    if (image_buffer) {
-      IOSurfaceRef surface = CVPixelBufferGetIOSurface(image_buffer);
-
-      gfx::ScopedTextureBinder
-          texture_binder(GL_TEXTURE_RECTANGLE_ARB, texture_ids_[picture_id]);
-      CGLError status = CGLTexImageIOSurface2D(
-          cgl_context_,                 // ctx
-          GL_TEXTURE_RECTANGLE_ARB,     // target
-          GL_RGB,                       // internal_format
-          texture_size_.width(),        // width
-          texture_size_.height(),       // height
-          GL_YCBCR_422_APPLE,           // format
-          GL_UNSIGNED_SHORT_8_8_APPLE,  // type
-          surface,                      // io_surface
-          0);                           // plane
-      if (status != kCGLNoError) {
-        NOTIFY_STATUS("CGLTexImageIOSurface2D()", status);
-        break;
-      }
-
-      picture_bindings_[picture_id] = frame.image_buffer;
-      client_->PictureReady(media::Picture(
-          picture_id, frame.bitstream_id, gfx::Rect(texture_size_)));
-      available_picture_ids_.pop();
+    IOSurfaceRef surface = CVPixelBufferGetIOSurface(image_buffer);
+
+    gfx::ScopedTextureBinder
+        texture_binder(GL_TEXTURE_RECTANGLE_ARB, texture_ids_[picture_id]);
+    CGLError status = CGLTexImageIOSurface2D(
+        cgl_context_,                 // ctx
+        GL_TEXTURE_RECTANGLE_ARB,     // target
+        GL_RGB,                       // internal_format
+        picture_size_.width(),        // width
+        picture_size_.height(),       // height
+        GL_YCBCR_422_APPLE,           // format
+        GL_UNSIGNED_SHORT_8_8_APPLE,  // type
+        surface,                      // io_surface
+        0);                           // plane
+    if (status != kCGLNoError) {
+      NOTIFY_STATUS("CGLTexImageIOSurface2D()", status);
+      break;
     }
 
+    picture_bindings_[picture_id] = frame.image_buffer;
+    client_->PictureReady(media::Picture(
+        picture_id, frame.bitstream_id, gfx::Rect(picture_size_)));
+    available_picture_ids_.pop_back();
     client_->NotifyEndOfBitstreamBuffer(frame.bitstream_id);
     last_sent_bitstream_id = frame.bitstream_id;
     decoded_frames_.pop();
     pending_bitstream_ids_.pop();
+
+    last_sent_bitstream_id =
+        ProcessDroppedFrames(last_sent_bitstream_id, up_to_bitstream_id);
+    if (last_sent_bitstream_id == up_to_bitstream_id || decoded_frames_.empty())
+      break;
+
+    ProcessSizeChange();
   }
   glDisable(GL_TEXTURE_RECTANGLE_ARB);