Revert "Vaapi: Remove ConditionVariables and use PostTask"

This reverts commit 6683995b.

Reason for revert: crbug.com/823235

Original change's description:
> Vaapi: Remove ConditionVariables and use PostTask
> 
> This CL removes the use of the ConditionVariables inside VaVDA and
> uses instead PostTask()ing. This removes blocking the worker
> thread, which allows for moving other tasks there, e.g. the
> Surface Blit (as done in the follow up WIP CL crrev.com/c/947341)
> 
> For all that goodness, this CL:
> 
> - Refactors the waiting logic that on ToT is spread between
>  GetCurrInputBuffer_Locked() and WaitForSurfaces_Locked() and
>  simplifies it in DecodeTask() itself.
> 
> - Moves the |curr_input_buffer_| filling logic in
>  GetCurrInputBuffer_Locked() into DecodeTask() proper.
> 
> - Reduces the scoped of the |lock_| in DecodeTask() (note that
>  |curr_input_buffer_| and |decode_| are only used on the decoder
>  thread -- added a .h comment).
> 
> Minor stuff: This CL also moves together the declaration of the
> related member vars |input_buffers_|, |curr_input_buffer_| and
> |available_va_surfaces_|
> 
> TEST=simplechrome+crosvideo (including seeks) on soraka, and
> v_d_a_unittest with h264, vp8, vp9 (with dcheck_always_on=true).
> 
> Bug: 717265
> Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: Ib167115dd2a4ebaeaea3ba3b5a205779d659f479
> Reviewed-on: https://chromium-review.googlesource.com/949283
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542955}

TBR=posciak@chromium.org,kcwu@chromium.org,mcasas@chromium.org,dcastagna@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 717265
Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I62361a33067140eb42112ccf7613136137bc30e2
Reviewed-on: https://chromium-review.googlesource.com/967967
Commit-Queue: Pawel Osciak <posciak@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544014}

media/gpu/vaapi/vaapi_video_decode_accelerator.cc

@@ -132,7 +132,9 @@ VaapiVideoDecodeAccelerator::VaapiVideoDecodeAccelerator(
     const MakeGLContextCurrentCallback& make_context_current_cb,
     const BindGLImageCallback& bind_image_cb)
     : state_(kUninitialized),
+      input_ready_(&lock_),
       vaapi_picture_factory_(new VaapiPictureFactory()),
+      surfaces_available_(&lock_),
       task_runner_(base::ThreadTaskRunnerHandle::Get()),
       decoder_thread_("VaapiDecoderThread"),
       num_frames_at_client_(0),
@@ -287,17 +289,20 @@ void VaapiVideoDecodeAccelerator::QueueInputBuffer(
   }
 
   TRACE_COUNTER1("media,gpu", "Vaapi input buffers", input_buffers_.size());
+  input_ready_.Signal();
 
   switch (state_) {
     case kIdle:
       state_ = kDecoding;
-      FALLTHROUGH;
-    case 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
@@ -312,67 +317,128 @@ void VaapiVideoDecodeAccelerator::QueueInputBuffer(
   }
 }
 
-void VaapiVideoDecodeAccelerator::DecodeTask() {
+bool VaapiVideoDecodeAccelerator::GetCurrInputBuffer_Locked() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
+  lock_.AssertAcquired();
 
-  {
-    base::AutoLock auto_lock(lock_);
-    if (state_ != kDecoding)
-      return;
+  if (curr_input_buffer_.get())
+    return true;
 
-    // Extract |curr_input_buffer_| out of |input_buffers_| if we have none, and
-    // set it in |decoder_| if it isn't a Flush Request.
-    if (!curr_input_buffer_ && !input_buffers_.empty()) {
-      curr_input_buffer_ = std::move(input_buffers_.front());
-      input_buffers_.pop();
+  // 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();
 
-      if (curr_input_buffer_->IsFlushRequest()) {
-        VLOGF(4) << "New flush buffer";
-        FlushTask();
-        return;
-      }
+  // We could have got woken up in a different state or never got to sleep
+  // due to current state.
+  if (state_ != kDecoding && state_ != kIdle)
+    return false;
 
-      SharedMemoryRegion* const shm = curr_input_buffer_->shm();
-      VLOGF(4) << "New |curr_input_buffer|, id " << curr_input_buffer_->id()
-               << " size: " << shm->size() << "B";
-      decoder_->SetStream(curr_input_buffer_->id(),
-                          static_cast<uint8_t*>(shm->memory()), shm->size());
-    }
+  DCHECK(!input_buffers_.empty());
+  curr_input_buffer_ = std::move(input_buffers_.front());
+  input_buffers_.pop();
+  TRACE_COUNTER1("media,gpu", "Input buffers", input_buffers_.size());
+
+  if (curr_input_buffer_->IsFlushRequest()) {
+    VLOGF(4) << "New flush buffer";
+    return true;
   }
 
-  AcceleratedVideoDecoder::DecodeResult res;
-  {
-    TRACE_EVENT0("media,gpu", "VAVDA::Decode");
-    res = decoder_->Decode();
-  }
-  switch (res) {
-    case AcceleratedVideoDecoder::kAllocateNewSurfaces:
-      VLOGF(2) << "Decoder requesting a new set of surfaces";
-      task_runner_->PostTask(
-          FROM_HERE,
-          base::BindOnce(&VaapiVideoDecodeAccelerator::InitiateSurfaceSetChange,
-                         weak_this_, decoder_->GetRequiredNumOfPictures(),
-                         decoder_->GetPicSize()));
-      // We'll get rescheduled once ProvidePictureBuffers() finishes.
-      return;
+  VLOGF(4) << "New |curr_input_buffer_|, id: " << curr_input_buffer_->id()
+           << " size: " << curr_input_buffer_->shm()->size() << "B";
+  decoder_->SetStream(
+      curr_input_buffer_->id(),
+      static_cast<uint8_t*>(curr_input_buffer_->shm()->memory()),
+      curr_input_buffer_->shm()->size());
 
-    case AcceleratedVideoDecoder::kRanOutOfStreamData:
-      curr_input_buffer_.reset();
-      break;
+  return true;
+}
 
-    case AcceleratedVideoDecoder::kRanOutOfSurfaces:
-      // We'll be rescheduled once we get a VA Surface via RecycleVASurfaceID().
+void VaapiVideoDecodeAccelerator::ReturnCurrInputBuffer_Locked() {
+  DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
+  lock_.AssertAcquired();
+  DCHECK(curr_input_buffer_.get());
+  curr_input_buffer_.reset();
+}
+
+// TODO(posciak): refactor the whole class to remove sleeping in wait for
+// surfaces, and reschedule DecodeTask instead.
+bool VaapiVideoDecodeAccelerator::WaitForSurfaces_Locked() {
+  DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
+  lock_.AssertAcquired();
+
+  while (available_va_surfaces_.empty() &&
+         (state_ == kDecoding || state_ == kIdle)) {
+    surfaces_available_.Wait();
+  }
+
+  return state_ == kDecoding || state_ == kIdle;
+}
+
+void VaapiVideoDecodeAccelerator::DecodeTask() {
+  DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
+  base::AutoLock auto_lock(lock_);
+
+  if (state_ != kDecoding)
+    return;
+  VLOGF(4) << "Decode task";
+
+  // Try to decode what stream data is (still) in the decoder until we run out
+  // of it.
+  while (GetCurrInputBuffer_Locked()) {
+    DCHECK(curr_input_buffer_.get());
+
+    if (curr_input_buffer_->IsFlushRequest()) {
+      FlushTask();
       break;
+    }
 
-    case AcceleratedVideoDecoder::kNeedContextUpdate:
-      // This shouldn't happen as we return false from IsFrameContextRequired().
-      NOTREACHED() << "Context updates not supported";
-      return;
+    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_);
+      TRACE_EVENT0("media,gpu", "VAVDA::Decode");
+      res = decoder_->Decode();
+    }
 
-    case AcceleratedVideoDecoder::kDecodeError:
-      RETURN_AND_NOTIFY_ON_FAILURE(false, "Error decoding stream",
-                                   PLATFORM_FAILURE, );
-      return;
+    switch (res) {
+      case AcceleratedVideoDecoder::kAllocateNewSurfaces:
+        VLOGF(2) << "Decoder requesting a new set of surfaces";
+        task_runner_->PostTask(
+            FROM_HERE,
+            base::Bind(&VaapiVideoDecodeAccelerator::InitiateSurfaceSetChange,
+                       weak_this_, decoder_->GetRequiredNumOfPictures(),
+                       decoder_->GetPicSize()));
+        // We'll get rescheduled once ProvidePictureBuffers() finishes.
+        return;
+
+      case AcceleratedVideoDecoder::kRanOutOfStreamData:
+        ReturnCurrInputBuffer_Locked();
+        break;
+
+      case AcceleratedVideoDecoder::kRanOutOfSurfaces:
+        // No more output buffers in the decoder, try getting more or go to
+        // sleep waiting for them.
+        if (!WaitForSurfaces_Locked())
+          return;
+
+        break;
+
+      case AcceleratedVideoDecoder::kNeedContextUpdate:
+        // This should not happen as we return false from
+        // IsFrameContextRequired().
+        NOTREACHED() << "Context updates not supported";
+        return;
+
+      case AcceleratedVideoDecoder::kDecodeError:
+        RETURN_AND_NOTIFY_ON_FAILURE(false, "Error decoding stream",
+                                     PLATFORM_FAILURE, );
+        return;
+    }
   }
 }
 
@@ -476,9 +542,7 @@ void VaapiVideoDecodeAccelerator::RecycleVASurfaceID(
   base::AutoLock auto_lock(lock_);
 
   available_va_surfaces_.push_back(va_surface_id);
-  decoder_thread_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&VaapiVideoDecodeAccelerator::DecodeTask,
-                                base::Unretained(this)));
+  surfaces_available_.Signal();
 }
 
 void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
@@ -503,7 +567,6 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
                                      buffers.size(), &va_surface_ids),
       "Failed creating VA Surfaces", PLATFORM_FAILURE, );
   DCHECK_EQ(va_surface_ids.size(), buffers.size());
-  available_va_surfaces_.assign(va_surface_ids.begin(), va_surface_ids.end());
 
   for (size_t i = 0; i < buffers.size(); ++i) {
     uint32_t client_id = !buffers[i].client_texture_ids().empty()
@@ -533,6 +596,9 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
         pictures_.insert(std::make_pair(buffers[i].id(), std::move(picture)))
             .second;
     DCHECK(inserted);
+
+    available_va_surfaces_.push_back(va_surface_ids[i]);
+    surfaces_available_.Signal();
   }
 
   // Resume DecodeTask if it is still in decoding state.
@@ -692,7 +758,7 @@ void VaapiVideoDecodeAccelerator::ResetTask() {
 
   // Return current input buffer, if present.
   if (curr_input_buffer_)
-    curr_input_buffer_.reset();
+    ReturnCurrInputBuffer_Locked();
 
   // And let client know that we are done with reset.
   task_runner_->PostTask(
@@ -717,6 +783,9 @@ void VaapiVideoDecodeAccelerator::Reset() {
   decoder_thread_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VaapiVideoDecodeAccelerator::ResetTask,
                             base::Unretained(this)));
+
+  input_ready_.Signal();
+  surfaces_available_.Signal();
 }
 
 void VaapiVideoDecodeAccelerator::FinishReset() {
@@ -778,6 +847,11 @@ void VaapiVideoDecodeAccelerator::Cleanup() {
   // TODO(mcasas): consider deleting |decoder_| on
   // |decoder_thread_task_runner_|, https://crbug.com/789160.
 
+  // Signal all potential waiters on the decoder_thread_, let them early-exit,
+  // as we've just moved to the kDestroying state, and wait for all tasks
+  // to finish.
+  input_ready_.Signal();
+  surfaces_available_.Signal();
   {
     base::AutoUnlock auto_unlock(lock_);
     decoder_thread_.Stop();

media/gpu/vaapi/vaapi_video_decode_accelerator.h

@@ -23,6 +23,7 @@
 #include "base/memory/linked_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "media/base/bitstream_buffer.h"
@@ -115,6 +116,21 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
   // Queue a input buffer for decode.
   void QueueInputBuffer(const BitstreamBuffer& bitstream_buffer);
 
+  // Gets a new |current_input_buffer_| from |input_buffers_| and sets it up in
+  // |decoder_|. This method will sleep if no |input_buffers_| are available.
+  // Returns true if a new buffer has been set up, false if an early exit has
+  // been requested (due to initiated reset/flush/destroy).
+  bool GetCurrInputBuffer_Locked();
+
+  // Signals the client that |curr_input_buffer_| has been read and can be
+  // returned. Will also release the mapping.
+  void ReturnCurrInputBuffer_Locked();
+
+  // Waits for more surfaces to become available. Returns true once they do or
+  // false if an early exit has been requested (due to an initiated
+  // reset/flush/destroy).
+  bool WaitForSurfaces_Locked();
+
   // Continue decoding given input buffers and sleep waiting for input/output
   // as needed. Will exit if a new set of surfaces or reset/flush/destroy
   // is requested.
@@ -188,20 +204,18 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
     kDestroying,
   };
 
-  // Protects input buffer and surface queues and |state_|.
+  // Protects input buffer and surface queues and state_.
   base::Lock lock_;
   State state_;
   Config::OutputMode output_mode_;
 
-  // Queue of input InputBuffers (PictureBuffer ids) to decode.
+  // Queue of available InputBuffers (picture_buffer_ids).
   base::queue<std::unique_ptr<InputBuffer>> input_buffers_;
-  // Current InputBuffer at |decoder_|.
-  // Only accessed on |decoder_thread_task_runner_| (needs no |lock_|)
-  std::unique_ptr<InputBuffer> curr_input_buffer_;
+  // Signalled when input buffers are queued onto |input_buffers_| queue.
+  base::ConditionVariable input_ready_;
 
-  // VA Surfaces no longer in use that can be passed back to the decoder for
-  // reuse, once it requests them.
-  std::list<VASurfaceID> available_va_surfaces_;
+  // Current input buffer at decoder.
+  std::unique_ptr<InputBuffer> curr_input_buffer_;
 
   // Queue for incoming output buffers (texture ids).
   using OutputBuffers = base::queue<int32_t>;
@@ -223,6 +237,13 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
   // Return a VaapiPicture associated with given client-provided id.
   VaapiPicture* PictureById(int32_t picture_buffer_id);
 
+  // VA Surfaces no longer in use that can be passed back to the decoder for
+  // reuse, once it requests them.
+  std::list<VASurfaceID> available_va_surfaces_;
+  // Signalled when output surfaces are queued onto the available_va_surfaces_
+  // queue.
+  base::ConditionVariable surfaces_available_;
+
   // Pending output requests from the decoder. When it indicates that we should
   // output a surface and we have an available Picture (i.e. texture) ready
   // to use, we'll execute the callback passing the Picture. The callback