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

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: I593763ec02dad7bba240c8fed9e71de21637a231
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339494Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795026}

media/gpu/vaapi/vaapi_video_decoder.cc

@@ -96,6 +96,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,
@@ -123,6 +136,9 @@ void VaapiVideoDecoder::Initialize(const VideoDecoderConfig& config,
     DVLOGF(3) << "Reinitializing decoder";
 
     decoder_ = nullptr;
+    // To clear |allocated_va_surfaces_| we have to first DestroyContext().
+    vaapi_wrapper_->DestroyContext();
+    allocated_va_surfaces_.clear();
     vaapi_wrapper_ = nullptr;
     decoder_delegate_ = nullptr;
     SetState(State::kUninitialized);
@@ -298,22 +314,29 @@ 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;
-  }
+  DCHECK(frame->GetGpuMemoryBuffer());
+  const gfx::GpuMemoryBufferId frame_id = frame->GetGpuMemoryBuffer()->GetId();
+  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];
   }
 
   // Store the mapping between surface and video frame, so we know which video
@@ -325,14 +348,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_),
@@ -414,7 +435,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();
@@ -427,8 +452,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);
@@ -445,10 +468,8 @@ void VaapiVideoDecoder::ApplyResolutionChange() {
   }
 }
 
-void VaapiVideoDecoder::ReleaseFrame(scoped_refptr<VASurface> va_surface,
-                                     VASurfaceID surface_id) {
+void VaapiVideoDecoder::ReleaseVideoFrame(VASurfaceID surface_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(decoder_sequence_checker_);
-  DCHECK_EQ(va_surface->id(), surface_id);
   DVLOGF(4);
 
   // The decoder has finished using the frame associated with |surface_id| for

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_;