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/+/2359975Reviewed-by: Fergus Dall <sidereal@google.com>
Commit-Queue: Fergus Dall <sidereal@google.com>
Cr-Commit-Position: refs/heads/master@{#798968}

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,22 +319,36 @@ scoped_refptr<VASurface> VaapiVideoDecoder::CreateSurface() {
     return nullptr;
   }
 
-  scoped_refptr<gfx::NativePixmap> pixmap =
-      CreateNativePixmapDmaBuf(frame.get());
-  if (!pixmap) {
-    LOG(ERROR) << "Failed to create NativePixmap from VideoFrame";
-    SetState(State::kError);
-    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) {
+      LOG(ERROR) << "Failed to create NativePixmap from VideoFrame";
+      SetState(State::kError);
+      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;
+    }
 
-  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
@@ -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_;