Fix Vaapi VDA flush handling during resolution change

media/gpu/vaapi_video_decode_accelerator.cc

 // 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 "media/gpu/vaapi_video_decode_accelerator.h"
 
 #include <string.h>
 
 #include <memory>
 
@@ skipping 250 matching lines @@
  private:
   scoped_refptr<VaapiDecodeSurface> VP9PictureToVaapiDecodeSurface(
       const scoped_refptr<VP9Picture>& pic);
 
   VaapiWrapper* vaapi_wrapper_;
   VaapiVideoDecodeAccelerator* vaapi_dec_;
 
   DISALLOW_COPY_AND_ASSIGN(VaapiVP9Accelerator);
 };
 
-VaapiVideoDecodeAccelerator::InputBuffer::InputBuffer() : id(0) {}
+VaapiVideoDecodeAccelerator::InputBuffer::InputBuffer() {}
 
 VaapiVideoDecodeAccelerator::InputBuffer::~InputBuffer() {}
 
 void VaapiVideoDecodeAccelerator::NotifyError(Error error) {
   if (!task_runner_->BelongsToCurrentThread()) {
     DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(&VaapiVideoDecodeAccelerator::NotifyError,
                                       weak_this_, error));
     return;
@@ skipping 172 matching lines @@
   VaapiPicture* picture = PictureById(output_buffers_.front());
   DCHECK(picture);
   output_buffers_.pop();
 
   output_cb.Run(picture);
 
   if (finish_flush_pending_ && pending_output_cbs_.empty())
     FinishFlush();
 }
 
-void VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
+bool VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
     const BitstreamBuffer& bitstream_buffer) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   TRACE_EVENT1("Video Decoder", "MapAndQueueNewInputBuffer", "input_id",
                bitstream_buffer.id());
 
   DVLOG(4) << "Mapping new input buffer id: " << bitstream_buffer.id()
            << " size: " << (int)bitstream_buffer.size();
 
   std::unique_ptr<SharedMemoryRegion> shm(
       new SharedMemoryRegion(bitstream_buffer, true));
 
   // Skip empty buffers.

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

Please see my comment below. Let's do the check in Decode().

   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);
   input_buffer->id = bitstream_buffer.id();
 
   ++num_stream_bufs_at_decoder_;
   TRACE_COUNTER1("Video Decoder", "Stream buffers at decoder",
                  num_stream_bufs_at_decoder_);
 
   input_buffers_.push(input_buffer);
   input_ready_.Signal();
+  return true;
 }
 
 bool VaapiVideoDecodeAccelerator::GetInputBuffer_Locked() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
   lock_.AssertAcquired();
 
   if (curr_input_buffer_.get())
     return true;
 
   // Will only wait if it is expected that in current state new buffers will
   // be queued from the client via Decode(). The state can change during wait.
   while (input_buffers_.empty() && (state_ == kDecoding || state_ == kIdle)) {
     input_ready_.Wait();
   }
 
   // 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());
 
       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:
       // We got woken up due to being destroyed/reset, ignore any already
       // queued inputs.
       return false;
   }
 }
 
 void VaapiVideoDecodeAccelerator::ReturnCurrInputBuffer_Locked() {
@@ skipping 12 matching lines @@
                  num_stream_bufs_at_decoder_);
 }
 
 // TODO(posciak): refactor the whole class to remove sleeping in wait for
 // surfaces, and reschedule DecodeTask instead.
 bool VaapiVideoDecodeAccelerator::WaitForSurfaces_Locked() {
   lock_.AssertAcquired();
   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");
   base::AutoLock auto_lock(lock_);
 
   if (state_ != kDecoding)
     return;
 
   // Main decode task.
   DVLOG(4) << "Decode task";
 
   // Try to decode what stream data is (still) in the decoder until we run out
   // of it.
   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
       // directly and we will reacquire the lock before looking at state again.
       // This is the main decode function of the decoder and while keeping
       // the lock for its duration would be fine, it would defeat the purpose
       // of having a separate decoder thread.
       base::AutoUnlock auto_unlock(lock_);
       res = decoder_->Decode();
     }
@@ skipping 125 matching lines @@
 
   if (bitstream_buffer.id() < 0) {
     if (base::SharedMemory::IsHandleValid(bitstream_buffer.handle()))
       base::SharedMemory::CloseHandle(bitstream_buffer.handle());
     LOG(ERROR) << "Invalid bitstream_buffer, id: " << bitstream_buffer.id();
     NotifyError(INVALID_ARGUMENT);
     return;
   }
 
   // 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_) {
     case kIdle:
       state_ = kDecoding;
       decoder_thread_task_runner_->PostTask(
           FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask,
                                 base::Unretained(this)));
       break;
 
@@ skipping 67 matching lines @@
       RETURN_AND_NOTIFY_ON_FAILURE(
           picture->Allocate(output_format_),
           "Failed to allocate memory for a VaapiPicture", PLATFORM_FAILURE, );
       output_buffers_.push(buffers[i].id());
     }
 
     available_va_surfaces_.push_back(va_surface_ids[i]);
     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)));
   }
 }
 
 #if defined(USE_OZONE)
 static void CloseGpuMemoryBufferHandle(
     const gfx::GpuMemoryBufferHandle& handle) {
   for (const auto& fd : handle.native_pixmap_handle.fds) {
@@ skipping 60 matching lines @@
 
   --num_frames_at_client_;
   TRACE_COUNTER1("Video Decoder", "Textures at client", num_frames_at_client_);
 
   output_buffers_.push(picture_buffer_id);
   TryOutputSurface();
 }
 
 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();
   RETURN_AND_NOTIFY_ON_FAILURE(res, "Failed flushing the decoder.",
                                PLATFORM_FAILURE, );
 
   // Put the decoder in idle state, ready to resume.
   decoder_->Reset();
 
   task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&VaapiVideoDecodeAccelerator::FinishFlush, weak_this_));
 }
 
 void VaapiVideoDecodeAccelerator::Flush() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   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();
 }
 
 void VaapiVideoDecodeAccelerator::FinishFlush() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   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.
   }
 
   // Still waiting for textures from client to finish outputting all pending
   // frames. Try again later.
   if (!pending_output_cbs_.empty()) {
     finish_flush_pending_ = true;
     return;
   }
@@ skipping 31 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   DVLOG(1) << "Got reset request";
 
   // This will make any new decode tasks exit early.
   base::AutoLock auto_lock(lock_);
   state_ = kResetting;
   finish_flush_pending_ = false;
 
   // 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();
   }
 
   decoder_thread_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::ResetTask,
                             base::Unretained(this)));
 
   input_ready_.Signal();
   surfaces_available_.Signal();
 }
@@ skipping 867 matching lines @@
   return vaapi_pic->dec_surface();
 }
 
 // static
 VideoDecodeAccelerator::SupportedProfiles
 VaapiVideoDecodeAccelerator::GetSupportedProfiles() {
   return VaapiWrapper::GetSupportedDecodeProfiles();
 }
 
 }  // namespace media