Revert "vaapi: cleanup a few comments and var names"

This reverts commit c813adf631.

Reason for revert: Needed to revert other CLs that caused a
regression in video_VideoSeek: https://crbug.com/834146
Bug: 834146

Original change's description:
> vaapi: cleanup a few comments and var names
>
> This CL cleans up a few things that I realized while
> debugging other CLs in the area:
> - |available_va_surfaces_| is a std::list on ToT, but has
>  queue semantics, so it's changed int his CL.
> -  s/output_buffers_/available_va_surfaces_/ because that's
>  what they are.
> - s/TryOutputSurface/TryOutputPicture/ because that's what
>  the method does.
> - s/pictures_/picture_map_/ to better define what it is. After
>  dcastagna@ suggestion, it's made a base::small_map in PS7
>
> Comments updated throughout these variables.
>
> No change in behaviour intended, but tested nonetheless with
> vp8/9-h264 crosvideo playback.
>
> Bug: 822346
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
> Change-Id: I2117ceac29f7d6ed23d698c4abd5c72e0140ae47
> Reviewed-on: https://chromium-review.googlesource.com/988512
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547552}

TBR=mcasas@chromium.org,dcastagna@chromium.org,hoegsberg@chromium.org

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

Bug: 822346
Change-Id: I7d242ad92a68596766f6cfe0ad2be37da3c4fb20
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1020005
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552166}

media/gpu/vaapi/vaapi_video_decode_accelerator.cc

@@ -129,8 +129,8 @@ void VaapiVideoDecodeAccelerator::NotifyError(Error error) {
 
 VaapiPicture* VaapiVideoDecodeAccelerator::PictureById(
     int32_t picture_buffer_id) {
-  PictureMap::iterator it = picture_map_.find(picture_buffer_id);
-  if (it == picture_map_.end()) {
+  Pictures::iterator it = pictures_.find(picture_buffer_id);
+  if (it == pictures_.end()) {
     VLOGF(4) << "Picture id " << picture_buffer_id << " does not exist";
     return NULL;
   }
@@ -245,22 +245,22 @@ void VaapiVideoDecodeAccelerator::OutputPicture(
   }
 }
 
-void VaapiVideoDecodeAccelerator::TryOutputPicture() {
+void VaapiVideoDecodeAccelerator::TryOutputSurface() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Handle Destroy() arriving while pictures are queued for output.
   if (!client_)
     return;
 
-  if (pending_output_cbs_.empty() || available_picture_buffers_.empty())
+  if (pending_output_cbs_.empty() || output_buffers_.empty())
     return;
 
   OutputCB output_cb = pending_output_cbs_.front();
   pending_output_cbs_.pop();
 
-  VaapiPicture* picture = PictureById(available_picture_buffers_.front());
+  VaapiPicture* picture = PictureById(output_buffers_.front());
   DCHECK(picture);
-  available_picture_buffers_.pop();
+  output_buffers_.pop();
 
   output_cb.Run(picture);
 
@@ -409,9 +409,9 @@ void VaapiVideoDecodeAccelerator::TryFinishSurfaceSetChange(
     return;
 
   if (!pending_output_cbs_.empty() ||
-      picture_map_.size() != available_va_surfaces_.size()) {
+      pictures_.size() != available_va_surfaces_.size()) {
     // Either: Not all |pending_output_cbs_| have been executed yet
-    // (i.e. they're waiting for resources in TryOutputPicture()), or |client_|
+    // (i.e. they're waiting for resources in TryOutputSurface()), or |client_|
     // hasn't returned all the |available_va_surfaces_| (via
     // RecycleVASurfaceID), In any case, give some time for both to happen.
     DVLOGF(2) << "Awaiting pending output/surface release callbacks to finish";
@@ -427,16 +427,16 @@ void VaapiVideoDecodeAccelerator::TryFinishSurfaceSetChange(
 
   // All surfaces released, destroy them and dismiss all PictureBuffers.
   awaiting_va_surfaces_recycle_ = false;
-  available_va_surfaces_ = {};
+  available_va_surfaces_.clear();
   vaapi_wrapper_->DestroySurfaces();
 
-  for (PictureMap::iterator iter = picture_map_.begin();
-       iter != picture_map_.end(); ++iter) {
+  for (Pictures::iterator iter = pictures_.begin(); iter != pictures_.end();
+       ++iter) {
     VLOGF(2) << "Dismissing picture id: " << iter->first;
     if (client_)
       client_->DismissPictureBuffer(iter->first);
   }
-  picture_map_.clear();
+  pictures_.clear();
 
   // And ask for a new set as requested.
   VLOGF(2) << "Requesting " << requested_num_pics_
@@ -482,7 +482,7 @@ void VaapiVideoDecodeAccelerator::RecycleVASurfaceID(
   DCHECK(task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
 
-  available_va_surfaces_.push(va_surface_id);
+  available_va_surfaces_.push_back(va_surface_id);
   decoder_thread_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&VaapiVideoDecodeAccelerator::DecodeTask,
                                 base::Unretained(this)));
@@ -492,9 +492,10 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
     const std::vector<PictureBuffer>& buffers) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
-  DCHECK(picture_map_.empty());
+  DCHECK(pictures_.empty());
 
-  available_picture_buffers_ = {};
+  while (!output_buffers_.empty())
+    output_buffers_.pop();
 
   RETURN_AND_NOTIFY_ON_FAILURE(
       buffers.size() >= requested_num_pics_,
@@ -509,8 +510,7 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
                                      buffers.size(), &va_surface_ids),
       "Failed creating VA Surfaces", PLATFORM_FAILURE, );
   DCHECK_EQ(va_surface_ids.size(), buffers.size());
-  for (const auto id : va_surface_ids)
-    available_va_surfaces_.push(id);
+  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()
@@ -534,11 +534,12 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
       RETURN_AND_NOTIFY_ON_FAILURE(
           picture->Allocate(vaapi_picture_factory_->GetBufferFormat()),
           "Failed to allocate memory for a VaapiPicture", PLATFORM_FAILURE, );
-      available_picture_buffers_.push(buffers[i].id());
+      output_buffers_.push(buffers[i].id());
     }
-    const auto result = picture_map_.emplace(
-        std::make_pair(buffers[i].id(), std::move(picture)));
-    DCHECK(result.second);
+    bool inserted =
+        pictures_.insert(std::make_pair(buffers[i].id(), std::move(picture)))
+            .second;
+    DCHECK(inserted);
   }
 
   // Resume DecodeTask if it is still in decoding state.
@@ -611,8 +612,8 @@ void VaapiVideoDecodeAccelerator::ReusePictureBuffer(
   --num_frames_at_client_;
   TRACE_COUNTER1("media,gpu", "Vaapi frames at client", num_frames_at_client_);
 
-  available_picture_buffers_.push(picture_buffer_id);
-  TryOutputPicture();
+  output_buffers_.push(picture_buffer_id);
+  TryOutputSurface();
 }
 
 void VaapiVideoDecodeAccelerator::FlushTask() {
@@ -829,7 +830,7 @@ void VaapiVideoDecodeAccelerator::VASurfaceReady(
       base::Bind(&VaapiVideoDecodeAccelerator::OutputPicture, weak_this_,
                  va_surface, bitstream_id, visible_rect));
 
-  TryOutputPicture();
+  TryOutputSurface();
 }
 
 scoped_refptr<VASurface> VaapiVideoDecodeAccelerator::CreateVASurface() {
@@ -843,7 +844,7 @@ scoped_refptr<VASurface> VaapiVideoDecodeAccelerator::CreateVASurface() {
   scoped_refptr<VASurface> va_surface(new VASurface(
       available_va_surfaces_.front(), requested_pic_size_,
       vaapi_wrapper_->va_surface_format(), va_surface_release_cb_));
-  available_va_surfaces_.pop();
+  available_va_surfaces_.pop_front();
 
   return va_surface;
 }

media/gpu/vaapi/vaapi_video_decode_accelerator.h

@@ -18,7 +18,6 @@
 #include <vector>
 
 #include "base/containers/queue.h"
-#include "base/containers/small_map.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -160,7 +159,7 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
                      VaapiPicture* picture);
 
   // Try to OutputPicture() if we have both a ready surface and picture.
-  void TryOutputPicture();
+  void TryOutputSurface();
 
   // Called when a VASurface is no longer in use by the decoder or is not being
   // synced/waiting to be synced to a picture. Returns it to available surfaces
@@ -190,15 +189,19 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
   State state_;
   Config::OutputMode output_mode_;
 
-  // Queue of InputBuffers (BitstreamBuffer id and data) to decode.
+  // Queue of input InputBuffers (PictureBuffer ids) to decode.
   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_;
 
-  // VASurfaceIDs available to use by |decoder_|, i.e. not assigned to a
-  // VASurface -- active between CreateVASurface() - VASurfaceReady().
-  base::queue<VASurfaceID> available_va_surfaces_;
+  // 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_;
+
+  // Queue for incoming output buffers (texture ids).
+  using OutputBuffers = base::queue<int32_t>;
+  OutputBuffers output_buffers_;
 
   std::unique_ptr<VaapiPictureFactory> vaapi_picture_factory_;
 
@@ -206,16 +209,14 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
   scoped_refptr<VaapiWrapper> vaapi_wrapper_;
   std::unique_ptr<AcceleratedVideoDecoder> decoder_;
 
-  // Queue of PictureBuffer ids available to be sent to |client_| via
-  // OutputPicture() (|client_| returns them via ReusePictureBuffer()).
-  base::queue<int32_t> available_picture_buffers_;
-  // Available VaapiPictures allocated by |client_| in AssignPictureBuffers()
-  // (and/or TryFinishSurfaceSetChange()). These pictures are indexed by the
-  // |available_picture_buffers_|.
-  using PictureMap =
-      base::small_map<std::map<int32_t, std::unique_ptr<VaapiPicture>>>;
-  PictureMap picture_map_;
-  // Returns the VaapiPicture indexed by |picture_buffer_id|, or nullptr.
+  // All allocated Pictures, regardless of their current state. Pictures are
+  // allocated once using |create_vaapi_picture_callback_| and destroyed at the
+  // end of decode. Comes after |vaapi_wrapper_| to ensure all pictures are
+  // destroyed before said |vaapi_wrapper_| is destroyed.
+  using Pictures = std::map<int32_t, std::unique_ptr<VaapiPicture>>;
+  Pictures pictures_;
+
+  // Return a VaapiPicture associated with given client-provided id.
   VaapiPicture* PictureById(int32_t picture_buffer_id);
 
   // Pending output requests from the decoder. When it indicates that we should