Implement flushing 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 37d96b7bbeae0a38c85a10a1afc77002a3076800..c83e2ef90c0589b9957a78f1f793a9db2e7fcda1 100644
--- a/content/common/gpu/media/vt_video_decode_accelerator.cc
+++ b/content/common/gpu/media/vt_video_decode_accelerator.cc
@@ -40,7 +40,6 @@ 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 = reinterpret_cast<intptr_t>(source_frame_refcon);
@@ -57,6 +56,16 @@ VTVideoDecodeAccelerator::DecodedFrame::DecodedFrame(
 VTVideoDecodeAccelerator::DecodedFrame::~DecodedFrame() {
 }
 
+VTVideoDecodeAccelerator::PendingAction::PendingAction(
+    Action action,
+    int32_t bitstream_id)
+    : action(action),
+      bitstream_id(bitstream_id) {
+}
+
+VTVideoDecodeAccelerator::PendingAction::~PendingAction() {
+}
+
 VTVideoDecodeAccelerator::VTVideoDecodeAccelerator(CGLContextObj cgl_context)
     : cgl_context_(cgl_context),
       client_(NULL),
@@ -160,7 +169,6 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
       image_config, kCVPixelBufferOpenGLCompatibilityKey, kCFBooleanTrue);
 
   // TODO(sandersd): Check if the session is already compatible.
-  // TODO(sandersd): Flush.
   session_.reset();
   CHECK(!VTDecompressionSessionCreate(
       kCFAllocatorDefault,
@@ -171,6 +179,8 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
       session_.InitializeInto()));
 
   // 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;
@@ -183,8 +193,7 @@ void VTVideoDecodeAccelerator::ConfigureDecoder(
 
 void VTVideoDecodeAccelerator::Decode(const media::BitstreamBuffer& bitstream) {
   DCHECK(CalledOnValidThread());
-  // TODO(sandersd): Test what happens if bitstream buffers are passed to VT out
-  // of order.
+  pending_bitstream_ids_.push(bitstream.id());
   decoder_thread_.message_loop_proxy()->PostTask(FROM_HERE, base::Bind(
       &VTVideoDecodeAccelerator::DecodeTask, base::Unretained(this),
       bitstream));
@@ -236,6 +245,14 @@ void VTVideoDecodeAccelerator::DecodeTask(
   if (!session_)
     ConfigureDecoder(config_nalu_data_ptrs, config_nalu_data_sizes);
 
+  if (!data_size) {

[Pawel Osciak, 2014/09/25 01:27:29]

Could you explain? If there is no data we want to output?

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

Done. (And yes.)

+    gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
+        &VTVideoDecodeAccelerator::OutputTask,
+        weak_this_factory_.GetWeakPtr(),
+        DecodedFrame(bitstream.id(), NULL)));
+    return;
+  }
+
   // 3. Allocate a memory-backed CMBlockBuffer for the translated data.
   base::ScopedCFTypeRef<CMBlockBufferRef> data;
   CHECK(!CMBlockBufferCreateWithMemoryBlock(
@@ -311,7 +328,7 @@ void VTVideoDecodeAccelerator::Output(
 void VTVideoDecodeAccelerator::OutputTask(DecodedFrame frame) {
   DCHECK(CalledOnValidThread());
   decoded_frames_.push(frame);
-  SendPictures();
+  ProcessDecodedFrames();
 }
 
 void VTVideoDecodeAccelerator::SizeChangedTask(gfx::Size coded_size) {
@@ -332,10 +349,11 @@ void VTVideoDecodeAccelerator::AssignPictureBuffers(
     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.
+  // Pictures are not marked as uncleared until after this method returns, and
+  // they will be broken if they are used before that happens. So, schedule
+  // future work after that happens.
   gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &VTVideoDecodeAccelerator::SendPictures,
+      &VTVideoDecodeAccelerator::ProcessDecodedFrames,
       weak_this_factory_.GetWeakPtr()));
 }
 
@@ -344,61 +362,173 @@ void VTVideoDecodeAccelerator::ReusePictureBuffer(int32_t picture_id) {
   DCHECK_EQ(CFGetRetainCount(picture_bindings_[picture_id]), 1);
   picture_bindings_.erase(picture_id);
   available_picture_ids_.push(picture_id);
-  SendPictures();
+  ProcessDecodedFrames();
 }
 
-// TODO(sandersd): Proper error reporting instead of CHECKs.
-void VTVideoDecodeAccelerator::SendPictures() {
+void VTVideoDecodeAccelerator::CompleteAction(Action action) {
   DCHECK(CalledOnValidThread());
-  if (available_picture_ids_.empty() || decoded_frames_.empty())
-    return;
+  switch (action) {
+    case ACTION_FLUSH:
+      client_->NotifyFlushDone();
+      break;
+    case ACTION_RESET:
+      client_->NotifyResetDone();
+      break;
+    case ACTION_DESTROY:
+      delete this;
+      break;
+  }
+}
+
+void VTVideoDecodeAccelerator::CompleteActions(int32_t bitstream_id) {
+  DCHECK(CalledOnValidThread());
+  while (!pending_actions_.empty() &&
+         pending_actions_.front().bitstream_id == bitstream_id) {

[Pawel Osciak, 2014/09/25 01:27:29]

Wouldn't you want to do <= instead of == here?

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

No, CompleteActions() is called every time a pending action is due, it can't
miss any. Its only job is to call *all* the due actions for a specific
bitstream_id. I suspect that there are no cases where there is more than one,
but I couldn't prove it can't happen based on our assumptions.

In general, it's an error to assume bitstream IDs are ordered (in fact they
wrap, but admittedly only every 200 days for 60fps content).

[Pawel Osciak, 2014/09/26 04:25:07]

Ah, lack of doc for the method confused me.
And I like your thinking about the wraparounds.

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

Acknowledged.

+    CompleteAction(pending_actions_.front().action);
+    pending_actions_.pop();
+  }
+}
+
+void VTVideoDecodeAccelerator::ProcessDecodedFrames() {
+  DCHECK(CalledOnValidThread());
+
+  while (true) {

[Pawel Osciak, 2014/09/25 01:27:29]

Maybe just while (!decoded_frames_.empty()) instead?

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

Done.

+    if (decoded_frames_.empty())
+      return;
+
+    if (pending_actions_.empty()) {
+      // No pending actions; send frames normally.
+      if (!available_picture_ids_.empty())
+        SendPictures(pending_bitstream_ids_.back());
+      return;
+    } else if (pending_actions_.front().action == ACTION_FLUSH) {

[Pawel Osciak, 2014/09/25 01:27:28]

I think it could be more readable if you:

if (pending_actions_.empty()) {
 ...
 return;
}

switch (pending_actions_.front().action) {
  case ACTION_FLUSH:
  case ACTION_RESET:
  case ACTION_DESTROY:
  ...
}

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

I like this, thanks!

+      // Flushing; send frames normally, then complete any actions.
+      if (!available_picture_ids_.empty()) {

[Pawel Osciak, 2014/09/25 01:27:28]

You make this check in SendPictures anyway. I'd remove it from here and above.
You return -1 from SendPictures if it was empty anyway.

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

This check served a few purposes:
  - It avoids needlessly setting up GL context (which is the same as adding an
early exit in SendPictures()).
  - It avoids passing -1 to CompleteActions(). I was avoiding this because
bitstream_ids are not actually spec'd to be non-negative.
  - It distinguishes whether to return or loop again, as if the queue is
non-empty SendPictures() is guaranteed to send a frame (or enter an error state,
but that's not implemented yet).

Under the assumption you believe that using negative bitstream_ids as flags is
safe, I've added a negativity CHECK for bitstream_ids, and I'll start using that
optimization where possible.

+        int32_t last_sent_bitstream_id =
+            SendPictures(pending_actions_.front().bitstream_id);
+        CompleteActions(last_sent_bitstream_id);
+        // Loop again, as there may be work to do for a new pending action.
+        continue;
+      }
+      return;

[Pawel Osciak, 2014/09/25 01:27:29]

I'd just remove continue and return and let the existing conditions above take
care of this to make this more readable.

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

Acknowledged.

[Pawel Osciak, 2014/09/26 04:25:07]

But not implemented?

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

I thought the switch version made this clear enough.

My worry here is that the Flush case makes no progress if there are no pictures
available, and I'd need to detect that specific case to abort, resulting in the
same branch either way. Similarly, DESTROY must return (it would be an error to
evaluate the loop conditions). The only flow that can be changed is RESET, but I
decided to favor consistency.

[Pawel Osciak, 2014/09/26 05:20:30]

My point is, continue is the default action. So you don't have to write it if
you instead return as the non-default action of each case. That's how people are
used to looking at switch()es, return is the non-default action, break is the
default.

[sandersd (OOO until July 31), 2014/09/26 06:56:02]

Done.

+    } else if (pending_actions_.front().action == ACTION_RESET) {
+      // Reseting; drop a decoded frame and then complete any actions.

[Pawel Osciak, 2014/09/25 01:27:28]

One frame? Not drop frames until the id for the action?

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

It's the same, but I've switched it around.

+      int32_t bitstream_id = decoded_frames_.front().bitstream_id;

[Pawel Osciak, 2014/09/25 01:27:28]

Why not do things until action's bitstream id? This feels like an unnecessary
dependency on the relationship between the action and pending and decoded
frames...

I think it's better to make it clear and simple, drop all until id in action.

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

Done.

+      decoded_frames_.pop();
+      DCHECK_EQ(pending_bitstream_ids_.front(), bitstream_id);
+      pending_bitstream_ids_.pop();
+      client_->NotifyEndOfBitstreamBuffer(bitstream_id);
+      CompleteActions(bitstream_id);
+      // Loop again, as there may be more frames to drop or work to do for a new
+      // pending action.
+      continue;

[Pawel Osciak, 2014/09/25 01:27:29]

Not needed.

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

Acknowledged.

[Pawel Osciak, 2014/09/26 04:25:07]

But not removed?

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

See above.

+    } else {
+      // Destroying; drop a decoded frame, then destroy if ready.

[Pawel Osciak, 2014/09/25 01:27:28]

Why only one not up to action id? That would be simpler and more readable
without having to make the assumption there was only one.

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

Done.

+      DCHECK_EQ(pending_actions_.front().action, ACTION_DESTROY);
+      int32_t bitstream_id = decoded_frames_.front().bitstream_id;
+      decoded_frames_.pop();
+      if (pending_actions_.front().bitstream_id == bitstream_id) {
+        CompleteAction(ACTION_DESTROY);
+        return;
+      }
+      // Loop again, as there may be more frames to drop.
+      continue;
+    }
+
+    NOTREACHED();

[Pawel Osciak, 2014/09/25 01:27:29]

This would go to default: clause.

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

Not quite, I want to be sure that every path at the end of a block is explicit
(return or continue), and this does that.

[Pawel Osciak, 2014/09/26 04:25:07]

You can do that by removing continues and putting NOTREACHED in default clause.

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

I don't believe that is the same. The compiler can tell me if I am missing a
case in the switch, and the continues are inside if blocks , changing them to
'break' is just more confusing.

[Pawel Osciak, 2014/09/26 05:20:30]

That's exactly the point I'm trying to make. Nobody expects "return" to be
default and "break" (de facto continue in this case) to be the non-default
action, when looking at a switch(){}

[sandersd (OOO until July 31), 2014/09/26 06:56:02]

Done.

+  }
+}
+
+int32_t VTVideoDecodeAccelerator::SendPictures(int32_t up_to_bitstream_id) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!decoded_frames_.empty());
+  DCHECK(!available_picture_ids_.empty());
 
+  int32_t last_sent_bitstream_id = -1;
   gfx::ScopedCGLSetCurrentContext scoped_set_current_context(cgl_context_);
   glEnable(GL_TEXTURE_RECTANGLE_ARB);
 
   while (!available_picture_ids_.empty() && !decoded_frames_.empty()) {
-    int32_t picture_id = available_picture_ids_.front();
-    available_picture_ids_.pop();
     DecodedFrame frame = decoded_frames_.front();
     decoded_frames_.pop();
-    IOSurfaceRef surface = CVPixelBufferGetIOSurface(frame.image_buffer);
-
-    gfx::ScopedTextureBinder
-        texture_binder(GL_TEXTURE_RECTANGLE_ARB, texture_ids_[picture_id]);
-    CHECK(!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
-
-    picture_bindings_[picture_id] = frame.image_buffer;
-    client_->PictureReady(media::Picture(
-        picture_id, frame.bitstream_id, gfx::Rect(texture_size_)));
+    DCHECK_EQ(pending_bitstream_ids_.front(), frame.bitstream_id);
+    pending_bitstream_ids_.pop();
+    int32_t picture_id = available_picture_ids_.front();
+    available_picture_ids_.pop();
+
+    CVImageBufferRef image_buffer = frame.image_buffer.get();
+    if (image_buffer) {

[Pawel Osciak, 2014/09/25 01:27:28]

What happens if there is no image_buffer? Don't we have to do something with the
Picture anyway? Or will we just hang? I think this is a NotifyError()
situation...

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

This happens in if !data_size, or if the decoder decides to drop a frame (which
I assume happens if the reference frames are missing, but I've not tested yet).
We skip sending the picture but still notify the end of the bitstream buffer.

I don't know a correct behavior for this case, but I can reproduce getting a
bitstream buffer with no image data in it (only stream configuration), so we'll
need to handle it.

+      IOSurfaceRef surface = CVPixelBufferGetIOSurface(image_buffer);
+
+      // TODO(sandersd): Find out why this somtimes fails due to no GL context.

[Pawel Osciak, 2014/09/25 01:27:28]

s/somtimes/sometimes/

How do you know it failed? How often does this happen? Was it happening before
this change?
If you know it failed, NotifyError().

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

It fails when the CGLTexImageIOSurface2D() check fails. It happened before, but
it's flaky and I can't reliably reproduce it. I intend to find a CGL expert to
help me with this one.

[Pawel Osciak, 2014/09/26 04:25:07]

Well, I'd really like to express my strong preference for NotifyError over
crashing the process once again here FTR.

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

Understood, replacing all the CHECKs is the next item on the TODO list.

+      gfx::ScopedTextureBinder
+          texture_binder(GL_TEXTURE_RECTANGLE_ARB, texture_ids_[picture_id]);
+      CHECK(!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
+
+      picture_bindings_[picture_id] = frame.image_buffer;
+      client_->PictureReady(media::Picture(
+          picture_id, frame.bitstream_id, gfx::Rect(texture_size_)));
+    }
+
     client_->NotifyEndOfBitstreamBuffer(frame.bitstream_id);
+    last_sent_bitstream_id = frame.bitstream_id;
+    if (frame.bitstream_id == up_to_bitstream_id)

[Pawel Osciak, 2014/09/25 01:27:29]

Make this a part of while() condition?

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

Done.

+      break;
   }
 
   glDisable(GL_TEXTURE_RECTANGLE_ARB);
+  return last_sent_bitstream_id;
+}
+
+void VTVideoDecodeAccelerator::FlushTask() {
+  DCHECK(decoder_thread_.message_loop_proxy()->BelongsToCurrentThread());
+  CHECK(!VTDecompressionSessionFinishDelayedFrames(session_));
+}
+
+void VTVideoDecodeAccelerator::QueueAction(Action action) {
+  DCHECK(CalledOnValidThread());
+  if (pending_bitstream_ids_.empty()) {
+    CompleteAction(action);

[Pawel Osciak, 2014/09/25 01:27:29]

Could you just let ProcessDecodedFrames() to this to have one place that handles
this case?

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

ProcessDecodedFrames() doesn't currently handle any cases where there are no
queued decoded frames. It would need a rename if we moved this logic there, and
I didn't find any good names for it!

If you really prefer that, I'd be happy to do a followup CL for it.

+  } else {
+    pending_actions_.push(PendingAction(action, pending_bitstream_ids_.back()));
+    ProcessDecodedFrames();
+    if (!pending_actions_.empty() && !pending_bitstream_ids_.empty()) {
+      decoder_thread_.message_loop_proxy()->PostTask(FROM_HERE, base::Bind(
+          &VTVideoDecodeAccelerator::FlushTask, base::Unretained(this)));

[Pawel Osciak, 2014/09/25 01:27:28]

This requires an explanation...

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

I think the condition was just wrong, as there will never be pending actions if
there are no pending frames.

+    }
+  }
 }
 
 void VTVideoDecodeAccelerator::Flush() {
   DCHECK(CalledOnValidThread());
-  // TODO(sandersd): Trigger flush, sending frames.
+  QueueAction(ACTION_FLUSH);
 }
 
 void VTVideoDecodeAccelerator::Reset() {
   DCHECK(CalledOnValidThread());
-  // TODO(sandersd): Trigger flush, discarding frames.
+  QueueAction(ACTION_RESET);
 }
 
 void VTVideoDecodeAccelerator::Destroy() {
   DCHECK(CalledOnValidThread());

[Pawel Osciak, 2014/09/25 01:27:29]

If you stopped the decoder thread here, you wouldn't have to ensure it was
defined last in the class.

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

This is indeed possible, as long as the FlushTask is queued first, but it
duplicates the implementation of QueueAction(). I'd like to think about it for a
while as a possible future refactoring.

[Pawel Osciak, 2014/09/26 04:25:07]

I don't understand the part about FlushTask... Queued first before what?

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

Queued to the decoder thread message loop, before shutting down the decoder
thread (so that the flush method gets called on VT, which is only done from the
decoder thread).

[Pawel Osciak, 2014/09/26 05:20:30]

If you decoder_thread_.Stop() from Destroy(), it will wait until all tasks are
done on decoder_thread_. If you require a FlushTask to always be done on
Destroy, just queue it explicitly here and then Stop()?

[sandersd (OOO until July 31), 2014/09/26 06:56:02]

This makes sense, but I'd still rather hold off. In particular, it can't go
after QueueAction(), because QueueAction() may delete |this|, and I'd rather not
just duplicate the contents of QueueAction() here.

The good news is that I found a bug while analyzing this, QueueAction() could
attempt to check for pending actions after ProcessDecodedFrames() deletes
|this|. So that's fixed at least (at the cost of removing a minor optimization).

[Pawel Osciak, 2014/09/26 15:30:49]

The idea was to do away with ACTION_DESTROY. It wouldn't have been a problem
then, although I guess then you'd need something else like a bool "destroying_"
or something...

[sandersd (OOO until July 31), 2014/09/26 17:28:51]

Acknowledged.

-  // TODO(sandersd): Trigger flush, discarding frames, and wait for them.
-  delete this;
+  // Drop any other pending actions.
+  while (!pending_actions_.empty())
+    pending_actions_.pop();
+  // Return all bitstream buffers.
+  while (!pending_bitstream_ids_.empty()) {
+    client_->NotifyEndOfBitstreamBuffer(pending_bitstream_ids_.front());

[Pawel Osciak, 2014/09/25 01:27:29]

You shouldn't have to do this on Destroy().

[sandersd (OOO until July 31), 2014/09/25 19:18:53]

It seems I do, GpuVideoDecoder DCHECKS that there are no buffers still in the
decoder in ~GpuVideoDecoder().

[Pawel Osciak, 2014/09/26 04:25:07]

Acknowledged.

+    pending_bitstream_ids_.pop();
+  }
+  QueueAction(ACTION_DESTROY);

[Pawel Osciak, 2014/09/25 01:27:29]

Do we actually need this? You dropped everything already above anyway.

[sandersd (OOO until July 31), 2014/09/25 19:18:52]

We tell the client we are done with the buffers, but VT may still have to flush
(and while it's doing that, it will be calling back with a reference to |this|).
It may be possible to destroy the VT session and avoid that, but I'll need to
ask the Apple engineers if that is guaranteed to work (fits with the stopping
the decoder thread refactor from above).

[Pawel Osciak, 2014/09/26 04:25:07]

How do you guarantee that during that client_ will not be called anymore?

[sandersd (OOO until July 31), 2014/09/26 04:56:41]

During destroy, ProcessDecodedFrames() specifically does not call SendPictures()
or NotifyEndOfBitstreamBuffer(), but instead drops all the frames without any
bookkeeping.

[Pawel Osciak, 2014/09/26 05:20:30]

Could you tell me how this is achieved please?

[sandersd (OOO until July 31), 2014/09/26 06:56:02]

Because there is a queued ACTION_DESTROY, it always goes through the drop frames
path until all frames are decoded.

 }
 
 bool VTVideoDecodeAccelerator::CanDecodeOnIOThread() {