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() {}
-
 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;
   }
 
   // Post Cleanup() as a task so we don't recursively acquire lock_.
@@ skipping 169 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(
+void VaapiVideoDecodeAccelerator::QueueInputBuffer(
     const BitstreamBuffer& bitstream_buffer) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  TRACE_EVENT1("Video Decoder", "MapAndQueueNewInputBuffer", "input_id",
+  TRACE_EVENT1("Video Decoder", "QueueInputBuffer", "input_id",
                bitstream_buffer.id());
 
-  DVLOG(4) << "Mapping new input buffer id: " << bitstream_buffer.id()
+  DVLOG(4) << "Queueing new input buffer id: " << bitstream_buffer.id()
            << " size: " << (int)bitstream_buffer.size();
 
-  std::unique_ptr<SharedMemoryRegion> shm(
-      new SharedMemoryRegion(bitstream_buffer, true));
+  base::AutoLock auto_lock(lock_);
+  if (bitstream_buffer.size() == 0) {
+    // Dummy buffer for flush.
+    DCHECK(!base::SharedMemory::IsHandleValid(bitstream_buffer.handle()));
+    input_buffers_.push(make_linked_ptr(new InputBuffer()));
+  } else {
+    std::unique_ptr<SharedMemoryRegion> shm(
+        new SharedMemoryRegion(bitstream_buffer, true));
 
-  // Skip empty buffers.
-  if (bitstream_buffer.size() == 0) {
-    if (client_)
-      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
-    return;
+    RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(), "Failed to map input buffer",
+                                 UNREADABLE_INPUT, );
+
+    linked_ptr<InputBuffer> input_buffer(new InputBuffer());
+    input_buffer->shm = std::move(shm);
+    input_buffer->id = bitstream_buffer.id();
+    input_buffers_.push(input_buffer);
   }
 
-  RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(), "Failed to map input buffer",
-                               UNREADABLE_INPUT, );
-
-  base::AutoLock auto_lock(lock_);
-
-  // 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_;

[Pawel Osciak, 2017/01/24 07:10:33]

I think we should only increment for non-flush buffers, it's used only for trace
counters.

[kcwu, 2017/01/24 07:54:23]

Done.

   TRACE_COUNTER1("Video Decoder", "Stream buffers at decoder",
                  num_stream_bufs_at_decoder_);
 
-  input_buffers_.push(input_buffer);
   input_ready_.Signal();
+
+  switch (state_) {
+    case kIdle:
+      state_ = kDecoding;
+      decoder_thread_task_runner_->PostTask(
+          FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask,
+                                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:
+      LOG(ERROR) << "Decode/Flush request from client in invalid state: "
+                 << state_;
+      NotifyError(PLATFORM_FAILURE);
+      break;
+  }
 }
 
 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_->is_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;
 }
 
 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_->is_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 124 matching lines @@
                bitstream_buffer.id());
 
   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);
+  // Skip empty buffers. VaapiVDA uses empty buffer as dummy buffer for flush
+  // internally.
+  if (bitstream_buffer.size() == 0) {
+    if (base::SharedMemory::IsHandleValid(bitstream_buffer.handle()))
+      base::SharedMemory::CloseHandle(bitstream_buffer.handle());
+    if (client_)
+      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
+    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;
-
-    case kDecoding:
-      // Decoder already running, fallthrough.
-    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;
-  }
+  QueueInputBuffer(bitstream_buffer);
 }
 
 void VaapiVideoDecodeAccelerator::RecycleVASurfaceID(
     VASurfaceID va_surface_id) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
 
   available_va_surfaces_.push_back(va_surface_id);
   surfaces_available_.Signal();
 }
@@ skipping 43 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(curr_input_buffer_.get() && curr_input_buffer_->is_flush());
+
   DVLOG(1) << "Flush task";
 
+  curr_input_buffer_.reset();
+  --num_stream_bufs_at_decoder_;
+  TRACE_COUNTER1("Video Decoder", "Stream buffers at decoder",
+                 num_stream_bufs_at_decoder_);
+
   // 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)));
-
-  input_ready_.Signal();
-  surfaces_available_.Signal();
+  // Queue a dummy buffer, which means flush.
+  QueueInputBuffer(media::BitstreamBuffer());
 }
 
 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;
   }
 
-  state_ = kIdle;
+  // Resume decoding if necessary.
+  if (input_buffers_.empty()) {
+    state_ = kIdle;
+  } else {
+    decoder_thread_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask,
+                              base::Unretained(this)));
+  }
 
   task_runner_->PostTask(FROM_HERE,
                          base::Bind(&Client::NotifyFlushDone, client_));
 
   DVLOG(1) << "Flush finished";
 }
 
 void VaapiVideoDecodeAccelerator::ResetTask() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
   DVLOG(1) << "ResetTask";
@@ skipping 19 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->is_flush())

[Pawel Osciak, 2017/01/24 07:10:33]

Perhaps we could have this in destructor of InputBuffer to be done
automatically?

[kcwu, 2017/01/24 07:54:23]

If doing so, InputBuffer would need to keep reference of either VDA or
(task_runner_ and client_). And need to deal with dtor timing of VDA, client_,
and/or task_runner_.

I'd prefer not in this CL.

[Pawel Osciak, 2017/01/24 09:19:16]

Acknowledged.

+      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