media/gpu: ScopedVASurface in VaapiImageDecoder

This CL refactors VaapiImageDecoder to use
std::unique_ptr<ScopedVASurface>, together with a custom deleter to
destroy the associated context. This ensures the decoder owns and
properly clean the underlying VA surface and VA context. This is useful
because it removes clean-up responsibilities from clients of
ScopedVASurface.

Unfortunately, we have noticed that VaapiWrapper would reduce the manual
ref count of the VaDisplayState upon its demise, which may cause some
ScopedVASurfaces with stale copies of VADisplay to not be able to
self-clean. Therefore this CL introduces a (hopefully) temporary
modification to ScopedVASurface in which it holds a scoped_refptr to the
VaapiWrapper that created it and uses it to self clean.

In addition, the API of image decoders is changed slightly. A
scoped_refptr<VASurface> is no longer returned upon Decode() requests.
Should a client need access to the ScopedVASurface, the API exposes a
GetScopedVASurface(), which returns a const raw pointer to the decoder's
ScopedVASurface. This should feel familiar due to the get() call in
unique_ptrs.

Finally, this CL fixes some of the call-sites that use VaapiJpegDecoder
(i.e. VaapiImageDecoder's new API).

Bug: 877694, 980379
Test: jpeg_decode_accelerator_unittest
Change-Id: I834c3af64813e3004f8a62da4e2ef529585be465
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1696012
Commit-Queue: Gil Dekel <gildekel@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677035}

media/gpu/vaapi/vaapi_image_decoder.cc

@@ -4,15 +4,24 @@
 
 #include "media/gpu/vaapi/vaapi_image_decoder.h"
 
+#include <utility>
+
 #include "base/logging.h"
 #include "media/gpu/macros.h"
 #include "media/gpu/vaapi/va_surface.h"
+#include "media/gpu/vaapi/vaapi_utils.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/linux/native_pixmap_dmabuf.h"
 
 namespace media {
 
+void VAContextAndScopedVASurfaceDeleter::operator()(
+    ScopedVASurface* scoped_va_surface) const {
+  scoped_va_surface->vaapi_wrapper_->DestroyContext();
+  delete scoped_va_surface;
+}
+
 VaapiImageDecoder::VaapiImageDecoder(VAProfile va_profile)
     : va_profile_(va_profile) {}
 
@@ -24,6 +33,34 @@ bool VaapiImageDecoder::Initialize(const base::RepeatingClosure& error_uma_cb) {
   return !!vaapi_wrapper_;
 }
 
+VaapiImageDecodeStatus VaapiImageDecoder::Decode(
+    base::span<const uint8_t> encoded_image) {
+  if (!vaapi_wrapper_) {
+    VLOGF(1) << "VaapiImageDecoder has not been initialized";
+    scoped_va_context_and_surface_.reset();
+    return VaapiImageDecodeStatus::kInvalidState;
+  }
+
+  const VaapiImageDecodeStatus status =
+      AllocateVASurfaceAndSubmitVABuffers(encoded_image);
+  if (status != VaapiImageDecodeStatus::kSuccess) {
+    scoped_va_context_and_surface_.reset();
+    return status;
+  }
+
+  if (!vaapi_wrapper_->ExecuteAndDestroyPendingBuffers(
+          scoped_va_context_and_surface_->id())) {
+    VLOGF(1) << "ExecuteAndDestroyPendingBuffers() failed";
+    scoped_va_context_and_surface_.reset();
+    return VaapiImageDecodeStatus::kExecuteDecodeFailed;
+  }
+  return VaapiImageDecodeStatus::kSuccess;
+}
+
+const ScopedVASurface* VaapiImageDecoder::GetScopedVASurface() const {
+  return scoped_va_context_and_surface_.get();
+}
+
 gpu::ImageDecodeAcceleratorSupportedProfile
 VaapiImageDecoder::GetSupportedProfile() const {
   if (!vaapi_wrapper_) {
@@ -56,21 +93,24 @@ scoped_refptr<gfx::NativePixmapDmaBuf>
 VaapiImageDecoder::ExportAsNativePixmapDmaBuf(VaapiImageDecodeStatus* status) {
   DCHECK(status);
 
-  // We need to take ownership of the VASurface so that the next decode doesn't
-  // attempt to use the same VASurface. Otherwise, it could overwrite the result
-  // of the current decode before it's used by the caller. This VASurface will
-  // self-clean at the end of this scope, but the underlying buffer should stay
-  // alive because of the exported FDs.
-  scoped_refptr<VASurface> va_surface = ReleaseVASurface();
-  if (!va_surface) {
+  // We need to take ownership of the ScopedVASurface so that the next Decode()
+  // doesn't attempt to use the same ScopedVASurface. Otherwise, it could
+  // overwrite the result of the current decode before it's used by the caller.
+  // This ScopedVASurface will self-clean at the end of this scope, but the
+  // underlying buffer should stay alive because of the exported FDs.
+  ScopedVAContextAndSurface temp_scoped_va_surface =
+      std::move(scoped_va_context_and_surface_);
+
+  if (!temp_scoped_va_surface) {
     DVLOGF(1) << "No decoded image available";
     *status = VaapiImageDecodeStatus::kInvalidState;
     return nullptr;
   }
+  DCHECK(temp_scoped_va_surface->IsValid());
 
-  DCHECK_NE(VA_INVALID_ID, va_surface->id());
   scoped_refptr<gfx::NativePixmapDmaBuf> pixmap =
-      vaapi_wrapper_->ExportVASurfaceAsNativePixmapDmaBuf(va_surface->id());
+      vaapi_wrapper_->ExportVASurfaceAsNativePixmapDmaBuf(
+          temp_scoped_va_surface->id());
   if (!pixmap) {
     *status = VaapiImageDecodeStatus::kCannotExportSurface;
     return nullptr;
@@ -79,8 +119,10 @@ VaapiImageDecoder::ExportAsNativePixmapDmaBuf(VaapiImageDecodeStatus* status) {
   // In Intel's iHD driver the size requested for the surface may be different
   // than the buffer size of the NativePixmap because of additional alignment.
   // See https://git.io/fj6nA.
-  DCHECK_LE(va_surface->size().width(), pixmap->GetBufferSize().width());
-  DCHECK_LE(va_surface->size().height(), pixmap->GetBufferSize().height());
+  DCHECK_LE(temp_scoped_va_surface->size().width(),
+            pixmap->GetBufferSize().width());
+  DCHECK_LE(temp_scoped_va_surface->size().height(),
+            pixmap->GetBufferSize().height());
 
   *status = VaapiImageDecodeStatus::kSuccess;
   return pixmap;

media/gpu/vaapi/vaapi_image_decoder.h

@@ -7,6 +7,8 @@
 
 #include <stdint.h>
 
+#include <memory>
+
 #include <va/va.h>
 
 #include "base/callback_forward.h"
@@ -21,9 +23,16 @@ class NativePixmapDmaBuf;
 
 namespace media {
 
-class VASurface;
+class ScopedVASurface;
 class VaapiWrapper;
 
+struct VAContextAndScopedVASurfaceDeleter {
+  void operator()(ScopedVASurface* scoped_va_surface) const;
+};
+
+using ScopedVAContextAndSurface =
+    std::unique_ptr<ScopedVASurface, VAContextAndScopedVASurfaceDeleter>;
+
 enum class VaapiImageDecodeStatus : uint32_t {
   kSuccess,
   kParseFailed,
@@ -54,13 +63,15 @@ class VaapiImageDecoder {
 
   // Decodes a picture. It will fill VA-API parameters and call the
   // corresponding VA-API methods according to the image in |encoded_image|.
-  // The image will be decoded into an internally allocated VA surface. It
-  // will be returned as an unowned VASurface, which remains valid until the
-  // next Decode() call or destruction of this class. Returns nullptr on
-  // failure and sets *|status| to the reason for failure.
-  virtual scoped_refptr<VASurface> Decode(
-      base::span<const uint8_t> encoded_image,
-      VaapiImageDecodeStatus* status) = 0;
+  // The image will be decoded into an internally allocated ScopedVASurface.
+  // This VA surface will remain valid until the next Decode() call or
+  // destruction of this class. Returns a VaapiImageDecodeStatus that will
+  // indicate whether the decode succeeded or the reason it failed. Note that
+  // the internal ScopedVASurface is destroyed on failure.
+  VaapiImageDecodeStatus Decode(base::span<const uint8_t> encoded_image);
+
+  // Returns a pointer to the internally managed ScopedVASurface.
+  const ScopedVASurface* GetScopedVASurface() const;
 
   // Returns the type of image supported by this decoder.
   virtual gpu::ImageDecodeAcceleratorType GetType() const = 0;
@@ -78,10 +89,14 @@ class VaapiImageDecoder {
  protected:
   explicit VaapiImageDecoder(VAProfile va_profile);
 
-  // Transfers ownership of the VASurface that contains the result of the last
-  // decode to the caller. The returned VASurface is self-cleaning.
-  // Returns a nullptr on failure.
-  virtual scoped_refptr<VASurface> ReleaseVASurface() = 0;
+  // Submits an image to the VA-API by filling its parameters and calling on the
+  // corresponding methods according to the image in |encoded_image|. Returns a
+  // VaapiImageDecodeStatus that will indicate whether the operation succeeded
+  // or the reason it failed.
+  virtual VaapiImageDecodeStatus AllocateVASurfaceAndSubmitVABuffers(
+      base::span<const uint8_t> encoded_image) = 0;
+
+  ScopedVAContextAndSurface scoped_va_context_and_surface_;
 
   scoped_refptr<VaapiWrapper> vaapi_wrapper_;
 

media/gpu/vaapi/vaapi_jpeg_decode_accelerator_worker.cc

@@ -55,10 +55,8 @@ void DecodeTask(
       mojo::WrapCallbackWithDefaultInvokeIfNotRun(std::move(decode_cb),
                                                   nullptr);
   DCHECK(decoder);
-  VaapiImageDecodeStatus status;
-  decoder->Decode(
-      base::make_span<const uint8_t>(encoded_data.data(), encoded_data.size()),
-      &status);
+  VaapiImageDecodeStatus status = decoder->Decode(
+      base::make_span<const uint8_t>(encoded_data.data(), encoded_data.size()));
   if (status != VaapiImageDecodeStatus::kSuccess) {
     DVLOGF(1) << "Failed to decode - status = "
               << static_cast<uint32_t>(status);

media/gpu/vaapi/vaapi_jpeg_decoder.cc

@@ -8,21 +8,18 @@
 
 #include <iostream>
 #include <type_traits>
-#include <vector>
 
 #include <va/va.h>
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/stl_util.h"
 #include "media/base/video_types.h"
 #include "media/gpu/macros.h"
-#include "media/gpu/vaapi/va_surface.h"
 #include "media/gpu/vaapi/vaapi_utils.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 #include "media/parsers/jpeg_parser.h"
+#include "ui/gfx/geometry/size.h"
 
 namespace media {
 
@@ -214,33 +211,20 @@ unsigned int VaSurfaceFormatForJpeg(const JpegFrameHeader& frame_header) {
 }
 
 VaapiJpegDecoder::VaapiJpegDecoder()
-    : VaapiImageDecoder(VAProfileJPEGBaseline),
-      va_surface_id_(VA_INVALID_SURFACE),
-      va_rt_format_(kInvalidVaRtFormat) {}
-
-VaapiJpegDecoder::~VaapiJpegDecoder() {
-  if (vaapi_wrapper_) {
-    vaapi_wrapper_->DestroyContextAndSurfaces(
-        std::vector<VASurfaceID>({va_surface_id_}));
-  }
-}
+    : VaapiImageDecoder(VAProfileJPEGBaseline) {}
 
-scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
-    base::span<const uint8_t> encoded_image,
-    VaapiImageDecodeStatus* status) {
-  if (!vaapi_wrapper_) {
-    VLOGF(1) << "VaapiJpegDecoder has not been initialized";
-    *status = VaapiImageDecodeStatus::kInvalidState;
-    return nullptr;
-  }
+VaapiJpegDecoder::~VaapiJpegDecoder() = default;
+
+VaapiImageDecodeStatus VaapiJpegDecoder::AllocateVASurfaceAndSubmitVABuffers(
+    base::span<const uint8_t> encoded_image) {
+  DCHECK(vaapi_wrapper_);
 
   // Parse the JPEG encoded data.
   JpegParseResult parse_result;
   if (!ParseJpegPicture(encoded_image.data(), encoded_image.size(),
                         &parse_result)) {
     VLOGF(1) << "ParseJpegPicture failed";
-    *status = VaapiImageDecodeStatus::kParseFailed;
-    return nullptr;
+    return VaapiImageDecodeStatus::kParseFailed;
   }
 
   // Figure out the right format for the VaSurface.
@@ -248,37 +232,35 @@ scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
       VaSurfaceFormatForJpeg(parse_result.frame_header);
   if (picture_va_rt_format == kInvalidVaRtFormat) {
     VLOGF(1) << "Unsupported subsampling";
-    *status = VaapiImageDecodeStatus::kUnsupportedSubsampling;
-    return nullptr;
+    return VaapiImageDecodeStatus::kUnsupportedSubsampling;
   }
 
   // Make sure this JPEG can be decoded.
   if (!IsVaapiSupportedJpeg(parse_result)) {
     VLOGF(1) << "The supplied JPEG is unsupported";
-    *status = VaapiImageDecodeStatus::kUnsupportedImage;
-    return nullptr;
+    return VaapiImageDecodeStatus::kUnsupportedImage;
   }
 
   // Prepare the VaSurface for decoding.
   const gfx::Size new_coded_size(
       base::strict_cast<int>(parse_result.frame_header.coded_width),
       base::strict_cast<int>(parse_result.frame_header.coded_height));
-  if (new_coded_size != coded_size_ || va_surface_id_ == VA_INVALID_SURFACE ||
-      picture_va_rt_format != va_rt_format_) {
-    vaapi_wrapper_->DestroyContextAndSurfaces(
-        std::vector<VASurfaceID>({va_surface_id_}));
-    va_surface_id_ = VA_INVALID_SURFACE;
-    va_rt_format_ = picture_va_rt_format;
-
-    std::vector<VASurfaceID> va_surfaces;
-    if (!vaapi_wrapper_->CreateContextAndSurfaces(va_rt_format_, new_coded_size,
-                                                  1, &va_surfaces)) {
-      VLOGF(1) << "Could not create the context or the surface";
-      *status = VaapiImageDecodeStatus::kSurfaceCreationFailed;
-      return nullptr;
+  DCHECK(!scoped_va_context_and_surface_ ||
+         scoped_va_context_and_surface_->IsValid());
+  if (!scoped_va_context_and_surface_ ||
+      new_coded_size != scoped_va_context_and_surface_->size() ||
+      picture_va_rt_format != scoped_va_context_and_surface_->format()) {
+    scoped_va_context_and_surface_.reset();
+    scoped_va_context_and_surface_ =
+        ScopedVAContextAndSurface(vaapi_wrapper_
+                                      ->CreateContextAndScopedVASurface(
+                                          picture_va_rt_format, new_coded_size)
+                                      .release());
+    if (!scoped_va_context_and_surface_) {
+      VLOGF(1) << "CreateContextAndScopedVASurface() failed";
+      return VaapiImageDecodeStatus::kSurfaceCreationFailed;
     }
-    va_surface_id_ = va_surfaces[0];
-    coded_size_ = new_coded_size;
+    DCHECK(scoped_va_context_and_surface_->IsValid());
   }
 
   // Set picture parameters.
@@ -286,8 +268,7 @@ scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
   FillPictureParameters(parse_result.frame_header, &pic_param);
   if (!vaapi_wrapper_->SubmitBuffer(VAPictureParameterBufferType, &pic_param)) {
     VLOGF(1) << "Could not submit VAPictureParameterBufferType";
-    *status = VaapiImageDecodeStatus::kSubmitVABuffersFailed;
-    return nullptr;
+    return VaapiImageDecodeStatus::kSubmitVABuffersFailed;
   }
 
   // Set quantization table.
@@ -295,8 +276,7 @@ scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
   FillIQMatrix(parse_result.q_table, &iq_matrix);
   if (!vaapi_wrapper_->SubmitBuffer(VAIQMatrixBufferType, &iq_matrix)) {
     VLOGF(1) << "Could not submit VAIQMatrixBufferType";
-    *status = VaapiImageDecodeStatus::kSubmitVABuffersFailed;
-    return nullptr;
+    return VaapiImageDecodeStatus::kSubmitVABuffersFailed;
   }
 
   // Set huffman table.
@@ -305,8 +285,7 @@ scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
                    &huffman_table);
   if (!vaapi_wrapper_->SubmitBuffer(VAHuffmanTableBufferType, &huffman_table)) {
     VLOGF(1) << "Could not submit VAHuffmanTableBufferType";
-    *status = VaapiImageDecodeStatus::kSubmitVABuffersFailed;
-    return nullptr;
+    return VaapiImageDecodeStatus::kSubmitVABuffersFailed;
   }
 
   // Set slice parameters.
@@ -314,8 +293,7 @@ scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
   FillSliceParameters(parse_result, &slice_param);
   if (!vaapi_wrapper_->SubmitBuffer(VASliceParameterBufferType, &slice_param)) {
     VLOGF(1) << "Could not submit VASliceParameterBufferType";
-    *status = VaapiImageDecodeStatus::kSubmitVABuffersFailed;
-    return nullptr;
+    return VaapiImageDecodeStatus::kSubmitVABuffersFailed;
   }
 
   // Set scan data.
@@ -323,21 +301,10 @@ scoped_refptr<VASurface> VaapiJpegDecoder::Decode(
                                     parse_result.data_size,
                                     const_cast<char*>(parse_result.data))) {
     VLOGF(1) << "Could not submit VASliceDataBufferType";
-    *status = VaapiImageDecodeStatus::kSubmitVABuffersFailed;
-    return nullptr;
-  }
-
-  // Execute the decode.
-  if (!vaapi_wrapper_->ExecuteAndDestroyPendingBuffers(va_surface_id_)) {
-    VLOGF(1) << "Executing the decode failed";
-    *status = VaapiImageDecodeStatus::kExecuteDecodeFailed;
-    return nullptr;
+    return VaapiImageDecodeStatus::kSubmitVABuffersFailed;
   }
 
-  *status = VaapiImageDecodeStatus::kSuccess;
-  return base::MakeRefCounted<VASurface>(va_surface_id_, coded_size_,
-                                         va_rt_format_,
-                                         base::DoNothing() /* release_cb */);
+  return VaapiImageDecodeStatus::kSuccess;
 }
 
 gpu::ImageDecodeAcceleratorType VaapiJpegDecoder::GetType() const {
@@ -347,23 +314,26 @@ gpu::ImageDecodeAcceleratorType VaapiJpegDecoder::GetType() const {
 std::unique_ptr<ScopedVAImage> VaapiJpegDecoder::GetImage(
     uint32_t preferred_image_fourcc,
     VaapiImageDecodeStatus* status) {
-  if (va_surface_id_ == VA_INVALID_ID) {
+  if (!scoped_va_context_and_surface_) {
     VLOGF(1) << "No decoded JPEG available";
     *status = VaapiImageDecodeStatus::kInvalidState;
     return nullptr;
   }
 
+  DCHECK(scoped_va_context_and_surface_->IsValid());
   DCHECK(vaapi_wrapper_);
   uint32_t image_fourcc;
   if (!VaapiWrapper::GetJpegDecodeSuitableImageFourCC(
-          va_rt_format_, preferred_image_fourcc, &image_fourcc)) {
+          scoped_va_context_and_surface_->format(), preferred_image_fourcc,
+          &image_fourcc)) {
     VLOGF(1) << "Cannot determine the output FOURCC";
     *status = VaapiImageDecodeStatus::kCannotGetImage;
     return nullptr;
   }
   VAImageFormat image_format{.fourcc = image_fourcc};
-  auto scoped_image =
-      vaapi_wrapper_->CreateVaImage(va_surface_id_, &image_format, coded_size_);
+  auto scoped_image = vaapi_wrapper_->CreateVaImage(
+      scoped_va_context_and_surface_->id(), &image_format,
+      scoped_va_context_and_surface_->size());
   if (!scoped_image) {
     VLOGF(1) << "Cannot get VAImage, FOURCC = "
              << FourccToString(image_format.fourcc);
@@ -375,28 +345,4 @@ std::unique_ptr<ScopedVAImage> VaapiJpegDecoder::GetImage(
   return scoped_image;
 }
 
-scoped_refptr<VASurface> VaapiJpegDecoder::ReleaseVASurface() {
-  if (va_surface_id_ == VA_INVALID_ID)
-    return nullptr;
-
-  DCHECK(vaapi_wrapper_);
-
-  // Prepare a new VASurface object. Note this VASurface will self-destruct.
-  auto va_surface = base::MakeRefCounted<VASurface>(
-      va_surface_id_, coded_size_, va_rt_format_,
-      base::BindOnce(&VaapiWrapper::DestroySurface, vaapi_wrapper_));
-
-  // Destroy the context. It is no longer needed since the only surface is going
-  // to be given away. A new surface and context will be created the next time
-  // Decode() is called
-  vaapi_wrapper_->DestroyContext();
-
-  // Invalidate the decoder's internal state.
-  va_surface_id_ = VA_INVALID_ID;
-  coded_size_.SetSize(0, 0);
-  va_rt_format_ = kInvalidVaRtFormat;
-
-  return va_surface;
-}
-
 }  // namespace media

media/gpu/vaapi/vaapi_jpeg_decoder.h

@@ -9,10 +9,8 @@
 
 #include <memory>
 
-#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "media/gpu/vaapi/vaapi_image_decoder.h"
-#include "ui/gfx/geometry/size.h"
 
 namespace media {
 
@@ -32,8 +30,6 @@ class VaapiJpegDecoder : public VaapiImageDecoder {
   ~VaapiJpegDecoder() override;
 
   // VaapiImageDecoder implementation.
-  scoped_refptr<VASurface> Decode(base::span<const uint8_t> encoded_image,
-                                  VaapiImageDecodeStatus* status) override;
   gpu::ImageDecodeAcceleratorType GetType() const override;
 
   // Get the decoded data from the last Decode() call as a ScopedVAImage. The
@@ -45,17 +41,9 @@ class VaapiJpegDecoder : public VaapiImageDecoder {
                                           VaapiImageDecodeStatus* status);
 
  protected:
-  scoped_refptr<VASurface> ReleaseVASurface() override;
-
- private:
-  FRIEND_TEST_ALL_PREFIXES(VaapiJpegDecoderTest, ReleaseVASurface);
-
-  // The current VA surface for decoding.
-  VASurfaceID va_surface_id_;
-  // The coded size associated with |va_surface_id_|.
-  gfx::Size coded_size_;
-  // The VA RT format associated with |va_surface_id_|.
-  unsigned int va_rt_format_;
+  // VaapiImageDecoder implementation.
+  VaapiImageDecodeStatus AllocateVASurfaceAndSubmitVABuffers(
+      base::span<const uint8_t> encoded_image) override;
 
   DISALLOW_COPY_AND_ASSIGN(VaapiJpegDecoder);
 };

media/gpu/vaapi/vaapi_jpeg_decoder_unittest.cc

@@ -390,10 +390,9 @@ std::unique_ptr<ScopedVAImage> VaapiJpegDecoderTest::Decode(
     base::span<const uint8_t> encoded_image,
     uint32_t preferred_fourcc,
     VaapiImageDecodeStatus* status) {
-  VaapiImageDecodeStatus decode_status;
-  scoped_refptr<VASurface> surface =
-      decoder_.Decode(encoded_image, &decode_status);
-  EXPECT_EQ(!!surface, decode_status == VaapiImageDecodeStatus::kSuccess);
+  const VaapiImageDecodeStatus decode_status = decoder_.Decode(encoded_image);
+  EXPECT_EQ(!!decoder_.GetScopedVASurface(),
+            decode_status == VaapiImageDecodeStatus::kSuccess);
 
   // Still try to get image when decode fails.
   VaapiImageDecodeStatus image_status;
@@ -419,10 +418,9 @@ scoped_refptr<gfx::NativePixmapDmaBuf>
 VaapiJpegDecoderTest::DecodeToNativePixmapDmaBuf(
     base::span<const uint8_t> encoded_image,
     VaapiImageDecodeStatus* status) {
-  VaapiImageDecodeStatus decode_status;
-  scoped_refptr<VASurface> surface =
-      decoder_.Decode(encoded_image, &decode_status);
-  EXPECT_EQ(!!surface, decode_status == VaapiImageDecodeStatus::kSuccess);
+  const VaapiImageDecodeStatus decode_status = decoder_.Decode(encoded_image);
+  EXPECT_EQ(!!decoder_.GetScopedVASurface(),
+            decode_status == VaapiImageDecodeStatus::kSuccess);
 
   // Still try to get the pixmap when decode fails.
   VaapiImageDecodeStatus pixmap_status;
@@ -464,52 +462,6 @@ TEST_F(VaapiJpegDecoderTest, MinimalImageFormatSupport) {
   }
 }
 
-TEST_F(VaapiJpegDecoderTest, ReleaseVASurface) {
-  base::FilePath input_file = FindTestDataFilePath(kYuv420Filename);
-  std::string jpeg_data;
-  ASSERT_TRUE(base::ReadFileToString(input_file, &jpeg_data))
-      << "failed to read input data from " << input_file.value();
-  const auto encoded_image = base::make_span<const uint8_t>(
-      reinterpret_cast<const uint8_t*>(jpeg_data.data()), jpeg_data.size());
-
-  VAImageFormat i420_format{};
-  i420_format.fourcc = VA_FOURCC_I420;
-  EXPECT_TRUE(VaapiWrapper::IsImageFormatSupported(i420_format));
-
-  // Get the first VASurface and validate success.
-  VaapiImageDecodeStatus decode_status;
-  scoped_refptr<VASurface> pre_release_surface =
-      decoder_.Decode(encoded_image, &decode_status);
-  ASSERT_TRUE(pre_release_surface);
-  ASSERT_EQ(VaapiImageDecodeStatus::kSuccess, decode_status);
-
-  // Take ownership of the |decoder_| surface.
-  scoped_refptr<VASurface> post_release_surface = decoder_.ReleaseVASurface();
-  ASSERT_TRUE(post_release_surface);
-
-  EXPECT_EQ(pre_release_surface->id(), post_release_surface->id());
-  EXPECT_EQ(pre_release_surface->size(), post_release_surface->size());
-  EXPECT_EQ(pre_release_surface->format(), post_release_surface->format());
-
-  // GetImage() should fail now because ReleaseVASurface() removed the internal
-  // |decoder_| surface.
-  VaapiImageDecodeStatus image_status;
-  ASSERT_FALSE(decoder_.GetImage(i420_format.fourcc, &image_status));
-  ASSERT_EQ(VaapiImageDecodeStatus::kInvalidState, image_status);
-
-  // Decode the same image again and get the new VASurface.
-  scoped_refptr<VASurface> new_surface =
-      decoder_.Decode(encoded_image, &decode_status);
-  ASSERT_TRUE(new_surface);
-  ASSERT_EQ(VaapiImageDecodeStatus::kSuccess, decode_status);
-
-  // The new VASurface should have a different surface ID than that of the old
-  // one, but same size and format since the same exact image was decoded.
-  EXPECT_NE(new_surface->id(), post_release_surface->id());
-  EXPECT_EQ(new_surface->size(), post_release_surface->size());
-  EXPECT_EQ(new_surface->format(), post_release_surface->format());
-}
-
 TEST_P(VaapiJpegDecoderTest, DecodeSucceeds) {
   base::FilePath input_file = FindTestDataFilePath(GetParam().filename);
   std::string jpeg_data;
@@ -563,6 +515,13 @@ TEST_P(VaapiJpegDecoderTest, DecodeSucceeds) {
     std::unique_ptr<ScopedVAImage> scoped_image =
         Decode(encoded_image, image_format.fourcc);
     ASSERT_TRUE(scoped_image);
+    ASSERT_TRUE(decoder_.GetScopedVASurface());
+    EXPECT_TRUE(decoder_.GetScopedVASurface()->IsValid());
+    EXPECT_EQ(decoder_.GetScopedVASurface()->size().width(),
+              base::strict_cast<int>(parse_result.frame_header.coded_width));
+    EXPECT_EQ(decoder_.GetScopedVASurface()->size().height(),
+              base::strict_cast<int>(parse_result.frame_header.coded_height));
+    EXPECT_EQ(rt_format, decoder_.GetScopedVASurface()->format());
     const uint32_t actual_fourcc = scoped_image->image()->format.fourcc;
     // TODO(andrescj): CompareImages() only supports I420, NV12, YUY2, and YUYV.
     // Make it support all the image formats we expect and call it
@@ -583,6 +542,8 @@ TEST_P(VaapiJpegDecoderTest, DecodeSucceeds) {
 // successfully.
 //
 // TODO(andrescj): for now, this assumes 4:2:0. Handle other formats.
+// TODO(andrescj): consider recreating the decoder for every size so that no
+// state is retained.
 TEST_F(VaapiJpegDecoderTest, DecodeSucceedsForSupportedSizes) {
   gfx::Size min_supported_size;
   ASSERT_TRUE(VaapiWrapper::GetDecodeMinResolution(VAProfileJPEGBaseline,
@@ -619,6 +580,8 @@ TEST_F(VaapiJpegDecoderTest, DecodeSucceedsForSupportedSizes) {
     std::unique_ptr<ScopedVAImage> scoped_image = Decode(jpeg_data_span);
     ASSERT_TRUE(scoped_image)
         << "Decode unexpectedly failed for size = " << test_size.ToString();
+    ASSERT_TRUE(decoder_.GetScopedVASurface());
+    EXPECT_TRUE(decoder_.GetScopedVASurface()->IsValid());
     EXPECT_TRUE(CompareImages(
         jpeg_data_span, ScopedVAImageToDecodedVAImage(scoped_image.get())))
         << "The SSIM check unexpectedly failed for size = "
@@ -651,6 +614,7 @@ TEST_F(VaapiJpegDecoderTest, DecodeAndExportAsNativePixmapDmaBuf) {
   scoped_refptr<gfx::NativePixmapDmaBuf> pixmap =
       DecodeToNativePixmapDmaBuf(encoded_image, &status);
   ASSERT_EQ(VaapiImageDecodeStatus::kSuccess, status);
+  EXPECT_FALSE(decoder_.GetScopedVASurface());
   ASSERT_TRUE(pixmap);
 
   // After exporting the surface, we should not be able to obtain a VAImage with
@@ -752,6 +716,7 @@ TEST_F(VaapiJpegDecoderTest, DecodeFailsForBelowMinSize) {
                         &status))
         << "Decode unexpectedly succeeded for size = " << test_size.ToString();
     EXPECT_EQ(VaapiImageDecodeStatus::kUnsupportedImage, status);
+    EXPECT_FALSE(decoder_.GetScopedVASurface());
   }
 }
 
@@ -801,6 +766,7 @@ TEST_F(VaapiJpegDecoderTest, DecodeFailsForAboveMaxSize) {
                         &status))
         << "Decode unexpectedly succeeded for size = " << test_size.ToString();
     EXPECT_EQ(VaapiImageDecodeStatus::kUnsupportedImage, status);
+    EXPECT_FALSE(decoder_.GetScopedVASurface());
   }
 }
 
@@ -816,6 +782,7 @@ TEST_F(VaapiJpegDecoderTest, DecodeFails) {
           reinterpret_cast<const uint8_t*>(jpeg_data.data()), jpeg_data.size()),
       &status));
   EXPECT_EQ(VaapiImageDecodeStatus::kUnsupportedSubsampling, status);
+  EXPECT_FALSE(decoder_.GetScopedVASurface());
 }
 
 std::string TestParamToString(

media/gpu/vaapi/vaapi_mjpeg_decode_accelerator.cc

@@ -227,10 +227,8 @@ void VaapiMjpegDecodeAccelerator::DecodeTask(
   DCHECK(decoder_task_runner_->BelongsToCurrentThread());
   TRACE_EVENT0("jpeg", "DecodeTask");
 
-  VaapiImageDecodeStatus status;
-  decoder_.Decode(
-      base::make_span(static_cast<const uint8_t*>(shm->memory()), shm->size()),
-      &status);
+  VaapiImageDecodeStatus status = decoder_.Decode(
+      base::make_span(static_cast<const uint8_t*>(shm->memory()), shm->size()));
   if (status != VaapiImageDecodeStatus::kSuccess) {
     NotifyError(bitstream_buffer_id, VaapiJpegDecodeStatusToError(status));
     return;

media/gpu/vaapi/vaapi_utils.cc

@@ -5,6 +5,7 @@
 #include "media/gpu/vaapi/vaapi_utils.h"
 
 #include <type_traits>
+#include <utility>
 
 #include <va/va.h>
 
@@ -107,27 +108,20 @@ ScopedVAImage::~ScopedVAImage() {
   }
 }
 
-ScopedVASurface::ScopedVASurface(base::Lock* va_lock,
-                                 VADisplay va_display,
+ScopedVASurface::ScopedVASurface(scoped_refptr<VaapiWrapper> vaapi_wrapper,
                                  VASurfaceID va_surface_id,
                                  const gfx::Size& size,
                                  unsigned int va_rt_format)
-    : va_lock_(va_lock),
-      va_display_(va_display),
+    : vaapi_wrapper_(std::move(vaapi_wrapper)),
       va_surface_id_(va_surface_id),
       size_(size),
-      va_rt_format_(va_rt_format) {}
+      va_rt_format_(va_rt_format) {
+  DCHECK(vaapi_wrapper_);
+}
 
 ScopedVASurface::~ScopedVASurface() {
-  if (VA_INVALID_ID == va_surface_id_)
-    return;
-
-  base::AutoLock auto_lock(*va_lock_);
-  VASurfaceID va_surface_to_destroy = va_surface_id_;
-  const VAStatus va_res =
-      vaDestroySurfaces(va_display_, &va_surface_to_destroy, 1);
-  if (VA_STATUS_SUCCESS != va_res)
-    LOG(ERROR) << "vaDestroySurfaces failed: " << vaErrorStr(va_res);
+  if (va_surface_id_ != VA_INVALID_ID)
+    vaapi_wrapper_->DestroySurface(va_surface_id_);
 }
 
 bool ScopedVASurface::IsValid() const {

media/gpu/vaapi/vaapi_utils.h

@@ -26,6 +26,7 @@ class Lock;
 namespace media {
 class VaapiWrapper;
 class Vp8ReferenceFrameVector;
+struct VAContextAndScopedVASurfaceDeleter;
 struct Vp8FrameHeader;
 
 // Class to map a given VABuffer, identified by |buffer_id|, for its lifetime.
@@ -90,13 +91,10 @@ class ScopedVAImage {
 };
 
 // A VA-API-specific surface used by video/image codec accelerators to work on.
-// As the name suggests, this class is self-cleaning. It calls
-// vaDestroySurfaces() on the underlying VA surface upon destruction. Thus
-// |va_lock| is acquired for destruction purposes.
+// As the name suggests, this class is self-cleaning.
 class ScopedVASurface {
  public:
-  ScopedVASurface(base::Lock* va_lock,
-                  VADisplay va_display,
+  ScopedVASurface(scoped_refptr<VaapiWrapper> vaapi_wrapper,
                   VASurfaceID va_surface_id,
                   const gfx::Size& size,
                   unsigned int va_rt_format);
@@ -108,8 +106,8 @@ class ScopedVASurface {
   unsigned int format() const { return va_rt_format_; }
 
  private:
-  base::Lock* const va_lock_;
-  const VADisplay va_display_ GUARDED_BY(va_lock_);
+  friend struct VAContextAndScopedVASurfaceDeleter;
+  const scoped_refptr<VaapiWrapper> vaapi_wrapper_;
   const VASurfaceID va_surface_id_;
   const gfx::Size size_;
   const unsigned int va_rt_format_;

media/gpu/vaapi/vaapi_wrapper.cc

@@ -2028,7 +2028,7 @@ std::unique_ptr<ScopedVASurface> VaapiWrapper::CreateScopedVASurface(
       << "Invalid VA surface id after vaCreateSurfaces";
 
   auto scoped_va_surface = std::make_unique<ScopedVASurface>(
-      va_lock_, va_display_, va_surface_id, size, va_rt_format);
+      this, va_surface_id, size, va_rt_format);
 
   DCHECK(scoped_va_surface);
   DCHECK(scoped_va_surface->IsValid());