RELAND 3: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive

This reverts commit f97ca58f.

Reason for revert: While the bot got green, the revert did not seem to be the cause. Reverting to confirm.

Original change's description:
> Revert "Reland "RELAND 2: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive""
> 
> This reverts commit 49422e89.
> 
> Reason for revert: Speculative revert.
> Suspect of causing some failures that report Gpu issues with Wayland.
> https://ci.chromium.org/p/chromium/builders/ci/linux-lacros-tester-rel/1109
> Will reland if the revert does not fix the bot.
> 
> Original change's description:
> > Reland "RELAND 2: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive"
> > 
> > This is a reland of 02aec841
> > 
> > Speculative revert was incorrect
> > 
> > Original change's description:
> > > RELAND 2: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive
> > >
> > > The relanded CL was reverted due to crashes when used from ARC++:
> > > Video Frames coming from this path are not GpuMemoryBuffers, so there's
> > > a DCHECK not verified, and then a crash. This CL addresses that
> > > situation, fixing `video_decode_accelerator_tests --use_vd_vda`:
> > > see crrev.com/c/2359734/1..3.
> > >
> > > Bug: b:163920993
> > >
> > > Original RELAND description -------------------------------------------
> > >
> > > The original CL was reverted due to crashes in betty initialization:
> > > betty is a VM which doesn't support VA, so the VideoDecoderPipeline
> > > constructed-destructed a VaapiVideoDecoder, hitting an unprotected
> > > nullptr |vaapi_wrapper_| in dtor. Fix in crrev.com/c/2339494/2..3.
> > >
> > > TBR=andrescj@chromium.org
> > >
> > > Original CL description -----------------------------------------------
> > >
> > > Certain platforms and codecs suffer from horrible artifacts (Intel BYT,
> > > H264) or crashes (Intel BSW/BDW, VP9). This was traced to some kind of
> > > error in the tracking of the VASurfaces lifetime in the driver: every
> > > time we get a new resource from the pool to decode onto, this is
> > > imported into libva as a VASurface: this works fine almost everywhere
> > > but doesn't play well in these old platforms (see CreateSurface() body).
> > >
> > > This CL adds a map that keeps the ref-counted VASurfaces alive and
> > > indexed by the unique GpuMemoryBufferId until the VA Context is
> > > destroyed. In so doing, we're basically observing the "contract" of
> > > va.h vaDestroySurfaces() [1] "Surfaces can only be destroyed after all
> > > contexts using these surfaces have been destroyed".
> > >
> > > [1] https://github.com/intel/libva/blob/libva-2.0.0/va/va.h#L1134
> > >
> > > Bug: b:142019786 b:143323596
> > > Change-Id: I16e74eb2b483b892961eca27aed30112240aa8ba
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339494
> > > Reviewed-by: Miguel Casas <mcasas@chromium.org>
> > > Commit-Queue: Miguel Casas <mcasas@chromium.org>
> > > Cr-Original-Commit-Position: refs/heads/master@{#795026}
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359734
> > > Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
> > > Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
> > > Auto-Submit: Miguel Casas <mcasas@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#798931}
> > 
> > TBR=mcasas@chromium.org,andrescj@chromium.org
> > 
> > Bug: b:163920993
> > Bug: b:142019786 b:143323596
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Change-Id: I6124e960d93f6985dbf9a546bc9d34372815e002
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359975
> > Reviewed-by: Fergus Dall <sidereal@google.com>
> > Commit-Queue: Fergus Dall <sidereal@google.com>
> > Cr-Commit-Position: refs/heads/master@{#798968}
> 
> TBR=mcasas@chromium.org,andrescj@chromium.org,sidereal@google.com
> 
> Change-Id: I5a6c92e18abcfd4d16f67942fa882fdb6cbe4001
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: b:163920993
> Bug: b:142019786 b:143323596
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362247
> Reviewed-by: Guido Urdaneta <guidou@chromium.org>
> Commit-Queue: Guido Urdaneta <guidou@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#799038}

TBR=mcasas@chromium.org,guidou@chromium.org,andrescj@chromium.org,sidereal@google.com

# Not skipping CQ checks because this is a reland.

Bug: b:163920993
Bug: b:142019786 b:143323596
Change-Id: I17db054c9f09739a3b9486b53bad3bec42af40ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362747Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799078}

media/gpu/vaapi/vaapi_video_decoder.cc

@@ -97,6 +97,19 @@ VaapiVideoDecoder::~VaapiVideoDecoder() {
   ClearDecodeTaskQueue(DecodeStatus::ABORTED);
 
   weak_this_factory_.InvalidateWeakPtrs();
+
+  // Destroy explicitly to DCHECK() that |vaapi_wrapper_| references are held
+  // inside the accelerator in |decoder_|, by the |allocated_va_surfaces_| and
+  // of course by this class. To clear |allocated_va_surfaces_| we have to first
+  // DestroyContext().
+  decoder_ = nullptr;
+  if (vaapi_wrapper_) {
+    vaapi_wrapper_->DestroyContext();
+    allocated_va_surfaces_.clear();
+
+    DCHECK(vaapi_wrapper_->HasOneRef());
+    vaapi_wrapper_ = nullptr;
+  }
 }
 
 void VaapiVideoDecoder::Initialize(const VideoDecoderConfig& config,
@@ -124,6 +137,12 @@ void VaapiVideoDecoder::Initialize(const VideoDecoderConfig& config,
     DVLOGF(3) << "Reinitializing decoder";
 
     decoder_ = nullptr;
+    DCHECK(vaapi_wrapper_);
+    // To clear |allocated_va_surfaces_| we have to first DestroyContext().
+    vaapi_wrapper_->DestroyContext();
+    allocated_va_surfaces_.clear();
+
+    DCHECK(vaapi_wrapper_->HasOneRef());
     vaapi_wrapper_ = nullptr;
     decoder_delegate_ = nullptr;
     SetState(State::kUninitialized);
@@ -300,6 +319,17 @@ scoped_refptr<VASurface> VaapiVideoDecoder::CreateSurface() {
     return nullptr;
   }
 
+  // |frame|s coming from ARC++ are not GpuMemoryBuffer-backed, but they have
+  // DmaBufs whose fd numbers are consistent along the lifetime of the VA
+  // surfaces they back.
+  DCHECK(frame->GetGpuMemoryBuffer() || frame->HasDmaBufs());
+  const gfx::GpuMemoryBufferId frame_id =
+      frame->GetGpuMemoryBuffer()
+          ? frame->GetGpuMemoryBuffer()->GetId()
+          : gfx::GpuMemoryBufferId(frame->DmabufFds()[0].get());
+
+  scoped_refptr<VASurface> va_surface;
+  if (!base::Contains(allocated_va_surfaces_, frame_id)) {
     scoped_refptr<gfx::NativePixmap> pixmap =
         CreateNativePixmapDmaBuf(frame.get());
     if (!pixmap) {
@@ -308,16 +338,19 @@ scoped_refptr<VASurface> VaapiVideoDecoder::CreateSurface() {
       return nullptr;
     }
 
-  // Create VASurface from the native pixmap.
-  scoped_refptr<VASurface> va_surface =
-      vaapi_wrapper_->CreateVASurfaceForPixmap(std::move(pixmap));
-
+    va_surface = vaapi_wrapper_->CreateVASurfaceForPixmap(std::move(pixmap));
     if (!va_surface || va_surface->id() == VA_INVALID_ID) {
       LOG(ERROR) << "Failed to create VASurface from VideoFrame";
       SetState(State::kError);
       return nullptr;
     }
 
+    allocated_va_surfaces_[frame_id] = va_surface;
+  } else {
+    va_surface = allocated_va_surfaces_[frame_id];
+    DCHECK_EQ(frame->coded_size(), va_surface->size());
+  }
+
   // Store the mapping between surface and video frame, so we know which video
   // frame to output when the surface is ready. It's also important to keep a
   // reference to the video frame during decoding, as the frame will be
@@ -327,14 +360,12 @@ scoped_refptr<VASurface> VaapiVideoDecoder::CreateSurface() {
   output_frames_[surface_id] = frame;
 
   // When the decoder is done using the frame for output or reference, it will
-  // drop its reference to the surface. We can then safely destroy the surface
-  // and remove the associated video frame from |output_frames_|. To be notified
-  // when this happens we wrap the surface in another surface that calls
-  // ReleaseFrame() on destruction. The |va_surface| object is bound to the
-  // destruction callback to keep it alive, since the associated VAAPI surface
-  // will be automatically destroyed when we drop the reference.
-  VASurface::ReleaseCB release_frame_cb = base::BindOnce(
-      &VaapiVideoDecoder::ReleaseFrame, weak_this_, std::move(va_surface));
+  // drop its reference to the surface. We can then safely remove the associated
+  // video frame from |output_frames_|. To be notified when this happens we wrap
+  // the surface in another surface with ReleaseVideoFrame() as destruction
+  // observer.
+  VASurface::ReleaseCB release_frame_cb =
+      base::BindOnce(&VaapiVideoDecoder::ReleaseVideoFrame, weak_this_);
 
   return new VASurface(surface_id, frame->layout().coded_size(),
                        GetVaFormatForVideoCodecProfile(profile_),
@@ -416,7 +447,11 @@ void VaapiVideoDecoder::ApplyResolutionChange() {
   }
 
   // All pending decode operations will be completed before triggering a
-  // resolution change, so we can safely destroy the context here.
+  // resolution change, so we can safely DestroyContext() here; that, in turn,
+  // allows for clearing the |allocated_va_surfaces_|.
+  vaapi_wrapper_->DestroyContext();
+  allocated_va_surfaces_.clear();
+
   if (profile_ != decoder_->GetProfile()) {
     // When a profile is changed, we need to re-initialize VaapiWrapper.
     profile_ = decoder_->GetProfile();
@@ -429,8 +464,6 @@ void VaapiVideoDecoder::ApplyResolutionChange() {
     }
     decoder_delegate_->set_vaapi_wrapper(new_vaapi_wrapper.get());
     vaapi_wrapper_ = std::move(new_vaapi_wrapper);
-  } else {
-    vaapi_wrapper_->DestroyContext();
   }
 
   vaapi_wrapper_->CreateContext(pic_size);
@@ -447,9 +480,7 @@ void VaapiVideoDecoder::ApplyResolutionChange() {
   }
 }
 
-void VaapiVideoDecoder::ReleaseFrame(scoped_refptr<VASurface> va_surface,
-                                     VASurfaceID surface_id) {
-  DCHECK_EQ(va_surface->id(), surface_id);
+void VaapiVideoDecoder::ReleaseVideoFrame(VASurfaceID surface_id) {
   DVLOGF(4);
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 

media/gpu/vaapi/vaapi_video_decoder.h

@@ -15,6 +15,7 @@
 
 #include "base/containers/mru_cache.h"
 #include "base/containers/queue.h"
+#include "base/containers/small_map.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
@@ -29,6 +30,7 @@
 #include "media/video/supported_video_decoder_config.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
+#include "ui/gfx/gpu_memory_buffer.h"
 
 namespace media {
 
@@ -103,13 +105,11 @@ class VaapiVideoDecoder : public DecoderInterface,
   void ClearDecodeTaskQueue(DecodeStatus status);
 
   // Releases the local reference to the VideoFrame associated with the
-  // specified |surface_id| on the decoder thread. This is called when the last
-  // reference to the associated VASurface has been released, which happens when
-  // |decoder_| outputted the video frame, or stopped using it as a reference
-  // frame. Note that this doesn't mean the frame can be reused immediately, as
-  // it might still be used by the client.
-  void ReleaseFrame(scoped_refptr<VASurface> va_surface,
-                    VASurfaceID surface_id);
+  // specified |surface_id| on the decoder thread. This is called when
+  // |decoder_| has outputted the VideoFrame and stopped using it as a
+  // reference frame. Note that this doesn't mean the frame can be reused
+  // immediately, as it might still be used by the client.
+  void ReleaseVideoFrame(VASurfaceID surface_id);
   // Callback for |frame_pool_| to notify of available resources.
   void NotifyFrameAvailable();
 
@@ -159,6 +159,15 @@ class VaapiVideoDecoder : public DecoderInterface,
   // The list of frames currently used as output buffers or reference frames.
   std::map<VASurfaceID, scoped_refptr<VideoFrame>> output_frames_;
 
+  // VASurfaces are created via importing |frame_pool_| resources into libva in
+  // CreateSurface(). The following map keeps those VASurfaces for reuse
+  // according to the expectations of libva vaDestroySurfaces(): "Surfaces can
+  // only be destroyed after all contexts using these surfaces have been
+  // destroyed."
+  // TODO(crbug.com/1040291): remove this keep-alive when using SharedImages.
+  base::small_map<std::map<gfx::GpuMemoryBufferId, scoped_refptr<VASurface>>>
+      allocated_va_surfaces_;
+
   // Platform and codec specific video decoder.
   std::unique_ptr<AcceleratedVideoDecoder> decoder_;
   scoped_refptr<VaapiWrapper> vaapi_wrapper_;