Fix Vaapi VDA flush handling during resolution change

media/gpu/vaapi_video_decode_accelerator.cc

Index: media/gpu/vaapi_video_decode_accelerator.cc
diff --git a/media/gpu/vaapi_video_decode_accelerator.cc b/media/gpu/vaapi_video_decode_accelerator.cc
index a119f0fd16b039ddefc831c5be2fef71e14837de..4640c83cc10de48cbabab07c8242167225ff1da4 100644
--- a/media/gpu/vaapi_video_decode_accelerator.cc
+++ b/media/gpu/vaapi_video_decode_accelerator.cc
@@ -268,7 +268,7 @@ class VaapiVideoDecodeAccelerator::VaapiVP9Accelerator
   DISALLOW_COPY_AND_ASSIGN(VaapiVP9Accelerator);
 };
 
-VaapiVideoDecodeAccelerator::InputBuffer::InputBuffer() : id(0) {}
+VaapiVideoDecodeAccelerator::InputBuffer::InputBuffer() {}
 
 VaapiVideoDecodeAccelerator::InputBuffer::~InputBuffer() {}
 
@@ -461,7 +461,7 @@ void VaapiVideoDecodeAccelerator::TryOutputSurface() {
     FinishFlush();
 }
 
-void VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
+bool VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
     const BitstreamBuffer& bitstream_buffer) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   TRACE_EVENT1("Video Decoder", "MapAndQueueNewInputBuffer", "input_id",
@@ -477,14 +477,22 @@ void VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
   if (bitstream_buffer.size() == 0) {
     if (client_)
       client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
-    return;
+    return true;
   }
 
   RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(), "Failed to map input buffer",
-                               UNREADABLE_INPUT, );
+                               UNREADABLE_INPUT, false);
 
   base::AutoLock auto_lock(lock_);
 
+  if (IsFlushPending_Locked()) {
+    LOG(ERROR) << "Shouldn't enqueue more input before FlushDone";
+    if (client_)
+      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
+    NotifyError(PLATFORM_FAILURE);
+    return false;
+  }
+
   // Set up a new input buffer and queue it for later.
   linked_ptr<InputBuffer> input_buffer(new InputBuffer());
   input_buffer->shm = std::move(shm);
@@ -496,6 +504,7 @@ void VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
 
   input_buffers_.push(input_buffer);
   input_ready_.Signal();
+  return true;
 }
 
 bool VaapiVideoDecodeAccelerator::GetInputBuffer_Locked() {
@@ -514,12 +523,6 @@ bool VaapiVideoDecodeAccelerator::GetInputBuffer_Locked() {
   // We could have got woken up in a different state or never got to sleep
   // due to current state; check for that.
   switch (state_) {
-    case kFlushing:
-      // Here we are only interested in finishing up decoding buffers that are
-      // already queued up. Otherwise will stop decoding.
-      if (input_buffers_.empty())
-        return false;
-      // else fallthrough
     case kDecoding:
     case kIdle:
       DCHECK(!input_buffers_.empty());
@@ -527,12 +530,17 @@ bool VaapiVideoDecodeAccelerator::GetInputBuffer_Locked() {
       curr_input_buffer_ = input_buffers_.front();
       input_buffers_.pop();
 
-      DVLOG(4) << "New current bitstream buffer, id: " << curr_input_buffer_->id
-               << " size: " << curr_input_buffer_->shm->size();
+      if (curr_input_buffer_->dummy_flush) {
+        DVLOG(4) << "New flush buffer";
+      } else {
+        DVLOG(4) << "New current bitstream buffer, id: "
+                 << curr_input_buffer_->id
+                 << " size: " << curr_input_buffer_->shm->size();
 
-      decoder_->SetStream(
-          static_cast<uint8_t*>(curr_input_buffer_->shm->memory()),
-          curr_input_buffer_->shm->size());
+        decoder_->SetStream(
+            static_cast<uint8_t*>(curr_input_buffer_->shm->memory()),
+            curr_input_buffer_->shm->size());
+      }
       return true;
 
     default:
@@ -565,16 +573,30 @@ bool VaapiVideoDecodeAccelerator::WaitForSurfaces_Locked() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
 
   while (available_va_surfaces_.empty() &&
-         (state_ == kDecoding || state_ == kFlushing || state_ == kIdle)) {
+         (state_ == kDecoding || state_ == kIdle)) {
     surfaces_available_.Wait();
   }
 
-  if (state_ != kDecoding && state_ != kFlushing && state_ != kIdle)
+  if (state_ != kDecoding && state_ != kIdle)
     return false;
 
   return true;
 }
 
+bool VaapiVideoDecodeAccelerator::IsFlushPending_Locked() {

[Owen Lin, 2017/01/19 07:39:16]

I am thinking maybe we can get rid of this function. The function is mainly used
to no Decode() or Flush() before FlushDone().

But I think we can easily handle those cases by keep them in the input queue and
handle them until FlushDone() (instead of reject them).

For example:

Decode() ---> input A
Flush()  ---> Flush until Output A
Decode() ---> input B (Keep it in the queue)
PictureReady() --->  output A
FlushDone() --> Now we can process input B.

We can do this by making GetInputBuffer_Locked() returns false when still
processing a flush(). 

How do you think?

[kcwu, 2017/01/19 08:26:10]

It's intentional to reject them since this is invalid api usage.
Before FlushDone, only Reset is allowed.

[Owen Lin, 2017/01/20 02:20:06]

Yes, I see your point and it makes totally sense. 

Actually, it is a dilemma to me as well. In the respect of API, I don't think
this restriction is really needed. It makes sense to call flush() and then keep
sending input (if we still accept input after flush). The behavior can be easily
explained without confusion. Besides, I believe this restriction is not
documented yet.

For now, the restriction complicates the code. Without the restriction, the API
is easier to use and is still backward compatible.

+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (finish_flush_pending_)
+    return true;
+
+  if (curr_input_buffer_.get() && curr_input_buffer_->dummy_flush)
+    return true;
+
+  // Checking last element of |input_buffers_| is enough because no more
+  // buffers will be queued after flush.
+  return !input_buffers_.empty() && input_buffers_.back()->dummy_flush;
+}
+
 void VaapiVideoDecodeAccelerator::DecodeTask() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
   TRACE_EVENT0("Video Decoder", "VAVDA::DecodeTask");
@@ -591,6 +613,11 @@ void VaapiVideoDecodeAccelerator::DecodeTask() {
   while (GetInputBuffer_Locked()) {
     DCHECK(curr_input_buffer_.get());
 
+    if (curr_input_buffer_->dummy_flush) {
+      FlushTask();
+      break;
+    }
+
     AcceleratedVideoDecoder::DecodeResult res;
     {
       // We are OK releasing the lock here, as decoder never calls our methods
@@ -736,7 +763,8 @@ void VaapiVideoDecodeAccelerator::Decode(
   }
 
   // We got a new input buffer from the client, map it and queue for later use.
-  MapAndQueueNewInputBuffer(bitstream_buffer);
+  if (!MapAndQueueNewInputBuffer(bitstream_buffer))
+    return;
 
   base::AutoLock auto_lock(lock_);
   switch (state_) {
@@ -824,10 +852,8 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
     surfaces_available_.Signal();
   }
 
-  // The resolution changing may happen while resetting or flushing. In this
-  // case we do not change state and post DecodeTask().
-  if (state_ != kResetting && state_ != kFlushing) {
-    state_ = kDecoding;
+  // Resume DecodeTask if it is still in decoding state.
+  if (state_ == kDecoding) {
     decoder_thread_task_runner_->PostTask(
         FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask,
                               base::Unretained(this)));
@@ -908,8 +934,15 @@ void VaapiVideoDecodeAccelerator::ReusePictureBuffer(
 
 void VaapiVideoDecodeAccelerator::FlushTask() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
+  DCHECK(input_buffers_.empty()) << "Input buffers should be empty because"

[Owen Lin, 2017/01/19 07:39:16]

It looks like a comment instead of what we should print out.

[kcwu, 2017/01/20 07:49:03]

Code removed.

+                                 << " the client cannot queue buffers before"
+                                 << " FlushDone";
+  DCHECK(curr_input_buffer_.get() && curr_input_buffer_->dummy_flush);
+
   DVLOG(1) << "Flush task";
 
+  curr_input_buffer_.reset();
+
   // First flush all the pictures that haven't been outputted, notifying the
   // client to output them.
   bool res = decoder_->Flush();
@@ -929,11 +962,40 @@ void VaapiVideoDecodeAccelerator::Flush() {
   DVLOG(1) << "Got flush request";
 
   base::AutoLock auto_lock(lock_);
-  state_ = kFlushing;
-  // Queue a flush task after all existing decoding tasks to clean up.
-  decoder_thread_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::FlushTask,
-                            base::Unretained(this)));
+  if (IsFlushPending_Locked()) {
+    LOG(ERROR) << "Shouldn't Flush again before FlushDone";
+    NotifyError(PLATFORM_FAILURE);
+    return;
+  }
+
+  switch (state_) {
+    case kIdle:
+      state_ = kDecoding;
+      decoder_thread_task_runner_->PostTask(
+          FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask,

[Owen Lin, 2017/01/19 07:39:16]

Can we extract the common part of this function and Decode()? Maybe call it
QueueInput(bitstream_buffer)

My idea is to reject 0 size bitstream_buffer in Decode, and Flush() calls
QueueInput with a empty bitstream_buffer.

[kcwu, 2017/01/19 08:26:10]

Regarding to Flush() to use empty buffer, I don't want to overload the meaning
of empty buffer. Use an explicit flag is easier to understand.
Otherwise, empty buffer is invalid in some functions and means flush in some
other functions.

[Owen Lin, 2017/01/20 02:20:06]

First, empty buffer is not invalid, we do accept empty buffer, but just return
them to the client immediately(see l.476). And I believe this will be the only
place different, in all other places, we can treat empty buffer as flush().

My point here is to extract the common code of Flush() and Decode() as this part
is really complicated and they are actually doing the same thing.

[kcwu, 2017/01/20 07:49:03]

Done.

+                                base::Unretained(this)));
+      break;
+
+    case kDecoding:
+      // Decoder already running.
+      break;
+
+    case kResetting:
+      // When resetting, allow accumulating bitstream buffers, so that
+      // the client can queue after-seek-buffers while we are finishing with
+      // the before-seek one.
+      break;
+
+    default:
+      RETURN_AND_NOTIFY_ON_FAILURE(
+          false, "Decode request from client in invalid state: " << state_,
+          PLATFORM_FAILURE, );
+      break;
+  }
+
+  linked_ptr<InputBuffer> input_buffer(new InputBuffer());
+  input_buffer->dummy_flush = true;
+  input_buffers_.push(input_buffer);
 
   input_ready_.Signal();
   surfaces_available_.Signal();
@@ -945,7 +1007,7 @@ void VaapiVideoDecodeAccelerator::FinishFlush() {
   finish_flush_pending_ = false;
 
   base::AutoLock auto_lock(lock_);
-  if (state_ != kFlushing) {
+  if (state_ != kDecoding) {
     DCHECK_EQ(state_, kDestroying);
     return;  // We could've gotten destroyed already.
   }
@@ -997,9 +1059,11 @@ void VaapiVideoDecodeAccelerator::Reset() {
 
   // Drop all remaining input buffers, if present.
   while (!input_buffers_.empty()) {
-    task_runner_->PostTask(
-        FROM_HERE, base::Bind(&Client::NotifyEndOfBitstreamBuffer, client_,
-                              input_buffers_.front()->id));
+    const auto& input_buffer = input_buffers_.front();
+    if (!input_buffer->dummy_flush)
+      task_runner_->PostTask(
+          FROM_HERE, base::Bind(&Client::NotifyEndOfBitstreamBuffer, client_,
+                                input_buffer->id));
     input_buffers_.pop();
   }