media/gpu/vaapi: Release NativePixmap in CreateVASurfaceForPixmap()

VA-API driver shares the ownership of the imported buffer. A caller
doesn't have to keep NativePixmap after calling VaapiWrapper's
CreateVASurfaceForPixmap(). This CL changes the code to release
NativePixmap in CreateVASurfaceForPixmap().

This CL also introduces CreateVASurfaceForVideoFrame() to gather
together code represented in multiple call sites.

Bug: 1001413
Test: tast run video.DecodeAccel*
Test: tast run camera.EncodeAccelJPEG
Test: tast run camera.DecodeAccelJPEG
Change-Id: Iaab8923a07d722a0851a07956dafb1ed70fa5c6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1787493
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697118}

media/gpu/vaapi/vaapi_dmabuf_video_frame_mapper.cc

@@ -12,7 +12,6 @@
 #include "media/gpu/macros.h"
 #include "media/gpu/vaapi/vaapi_utils.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
-#include "ui/gfx/linux/native_pixmap_dmabuf.h"
 
 namespace media {
 
@@ -126,14 +125,8 @@ scoped_refptr<VideoFrame> VaapiDmaBufVideoFrameMapper::Map(
     return nullptr;
   }
 
-  const scoped_refptr<gfx::NativePixmap> pixmap =
-      CreateNativePixmapDmaBuf(video_frame.get());
-  if (!pixmap) {
-    VLOGF(1) << "Failed to create NativePixmap";
-    return nullptr;
-  }
   scoped_refptr<VASurface> va_surface =
-      vaapi_wrapper_->CreateVASurfaceForPixmap(pixmap);
+      vaapi_wrapper_->CreateVASurfaceForVideoFrame(video_frame.get());
   if (!va_surface) {
     VLOGF(1) << "Failed to create VASurface";
     return nullptr;

media/gpu/vaapi/vaapi_jpeg_encode_accelerator.cc

@@ -27,7 +27,6 @@
 #include "media/gpu/macros.h"
 #include "media/gpu/vaapi/vaapi_jpeg_encoder.h"
 #include "media/parsers/jpeg_parser.h"
-#include "ui/gfx/linux/native_pixmap_dmabuf.h"
 
 namespace media {
 
@@ -171,14 +170,8 @@ void VaapiJpegEncodeAccelerator::Encoder::EncodeWithDmaBufTask(
   // We need to explicitly blit the bound input surface here to make sure the
   // input we sent to VAAPI encoder is in tiled NV12 format since implicit
   // tiling logic is not contained in every driver.
-  auto input_pixmap = CreateNativePixmapDmaBuf(input_frame.get());
-  if (!input_pixmap) {
-    VLOGF(1) << "Cannot create native pixmap for input frame";
-    notify_error_cb_.Run(task_id, PLATFORM_FAILURE);
-    return;
-  }
   auto input_surface =
-      vpp_vaapi_wrapper_->CreateVASurfaceForPixmap(input_pixmap);
+      vpp_vaapi_wrapper_->CreateVASurfaceForVideoFrame(input_frame.get());
   if (!input_surface) {
     VLOGF(1) << "Failed to create input va surface";
     notify_error_cb_.Run(task_id, PLATFORM_FAILURE);

media/gpu/vaapi/vaapi_mjpeg_decode_accelerator.cc

@@ -43,8 +43,6 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/gpu_memory_buffer.h"
-#include "ui/gfx/linux/native_pixmap_dmabuf.h"
-#include "ui/gfx/native_pixmap.h"
 
 namespace media {
 
@@ -378,14 +376,8 @@ bool VaapiMjpegDecodeAccelerator::OutputPictureVppOnTaskRunner(
   TRACE_EVENT1("jpeg", __func__, "input_buffer_id", input_buffer_id);
 
   // Bind a VA surface to |video_frame|.
-  scoped_refptr<gfx::NativePixmap> pixmap =
-      CreateNativePixmapDmaBuf(video_frame.get());
-  if (!pixmap) {
-    VLOGF(1) << "Cannot create native pixmap for output buffer";
-    return false;
-  }
   scoped_refptr<VASurface> output_surface =
-      vpp_vaapi_wrapper_->CreateVASurfaceForPixmap(pixmap);
+      vpp_vaapi_wrapper_->CreateVASurfaceForVideoFrame(video_frame.get());
   if (!output_surface) {
     VLOGF(1) << "Cannot create VA surface for output buffer";
     return false;

media/gpu/vaapi/vaapi_picture_native_pixmap.h

@@ -18,10 +18,6 @@ namespace gl {
 class GLImage;
 }
 
-namespace gfx {
-class NativePixmap;
-}
-
 namespace media {
 
 class VaapiWrapper;
@@ -46,9 +42,6 @@ class VaapiPictureNativePixmap : public VaapiPicture {
   VASurfaceID va_surface_id() const override;
 
  protected:
-  // Ozone buffer, the storage of the EGLImage and the VASurface.
-  scoped_refptr<gfx::NativePixmap> pixmap_;
-
   // GLImage bound to the GL textures used by the VDA client.
   scoped_refptr<gl::GLImage> gl_image_;
 

media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc

@@ -46,21 +46,19 @@ VaapiPictureNativePixmapEgl::~VaapiPictureNativePixmapEgl() {
   }
 }
 
-bool VaapiPictureNativePixmapEgl::Initialize() {
+bool VaapiPictureNativePixmapEgl::Initialize(
+    scoped_refptr<gfx::NativePixmap> pixmap) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(pixmap_);
+  DCHECK(pixmap);
+  DCHECK(pixmap->AreDmaBufFdsValid());
 
   // Create a |va_surface_| from dmabuf fds (pixmap->GetDmaBufFd)
-  va_surface_ = vaapi_wrapper_->CreateVASurfaceForPixmap(pixmap_);
+  va_surface_ = vaapi_wrapper_->CreateVASurfaceForPixmap(std::move(pixmap));
   if (!va_surface_) {
     LOG(ERROR) << "Failed creating VASurface for NativePixmap";
     return false;
   }
 
-  // On non-ozone, no need to import dmabuf fds into output the gl texture
-  // because the dmabuf fds have been made from it.
-  DCHECK(pixmap_->AreDmaBufFdsValid());
-
   if (bind_image_cb_ &&
       !bind_image_cb_.Run(client_texture_id_, texture_target_, gl_image_,
                           true /* can_bind_to_sampler */)) {
@@ -106,13 +104,13 @@ bool VaapiPictureNativePixmapEgl::Allocate(gfx::BufferFormat format) {
     return false;
   }
 
-  // The |pixmap_| takes ownership of the dmabuf fds. So the only reason
-  // to keep a reference on the image is because the GPU service needs to
-  // track this image as it will be attached to a client texture.
-  pixmap_ = native_pixmap_dmabuf;
+  // The |va_surface_| created from |native_pixmap_dmabuf| shares the ownership
+  // of the buffer. So the only reason to keep a reference on the image is
+  // because the GPU service needs to track this image as it will be attached
+  // to a client texture.
   gl_image_ = image;
 
-  return Initialize();
+  return Initialize(std::move(native_pixmap_dmabuf));
 }
 
 bool VaapiPictureNativePixmapEgl::ImportGpuMemoryBufferHandle(

media/gpu/vaapi/vaapi_picture_native_pixmap_egl.h

@@ -14,6 +14,10 @@
 #include "ui/gfx/buffer_types.h"
 #include "ui/gfx/geometry/size.h"
 
+namespace gfx {
+class NativePixmap;
+}  // namespace gfx
+
 namespace media {
 
 class VaapiWrapper;
@@ -41,7 +45,7 @@ class VaapiPictureNativePixmapEgl : public VaapiPictureNativePixmap {
       gfx::GpuMemoryBufferHandle gpu_memory_buffer_handle) override;
 
  private:
-  bool Initialize();
+  bool Initialize(scoped_refptr<gfx::NativePixmap> pixmap);
 
   DISALLOW_COPY_AND_ASSIGN(VaapiPictureNativePixmapEgl);
 };

media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc

@@ -7,7 +7,6 @@
 #include "media/gpu/vaapi/va_surface.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 #include "ui/gfx/gpu_memory_buffer.h"
-#include "ui/gfx/linux/native_pixmap_dmabuf.h"
 #include "ui/gfx/native_pixmap.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_image_native_pixmap.h"
@@ -48,12 +47,13 @@ VaapiPictureNativePixmapOzone::~VaapiPictureNativePixmapOzone() {
   }
 }
 
-bool VaapiPictureNativePixmapOzone::Initialize() {
+bool VaapiPictureNativePixmapOzone::Initialize(
+    scoped_refptr<gfx::NativePixmap> pixmap) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(pixmap_);
-
+  DCHECK(pixmap);
+  DCHECK(pixmap->AreDmaBufFdsValid());
   // Create a |va_surface_| from dmabuf fds (pixmap->GetDmaBufFd)
-  va_surface_ = vaapi_wrapper_->CreateVASurfaceForPixmap(pixmap_);
+  va_surface_ = vaapi_wrapper_->CreateVASurfaceForPixmap(pixmap);
   if (!va_surface_) {
     LOG(ERROR) << "Failed creating VASurface for NativePixmap";
     return false;
@@ -69,10 +69,10 @@ bool VaapiPictureNativePixmapOzone::Initialize() {
 
   gl::ScopedTextureBinder texture_binder(texture_target_, texture_id_);
 
-  const gfx::BufferFormat format = pixmap_->GetBufferFormat();
+  const gfx::BufferFormat format = pixmap->GetBufferFormat();
 
   auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size_, format);
-  if (!image->Initialize(pixmap_)) {
+  if (!image->Initialize(std::move(pixmap))) {
     LOG(ERROR) << "Failed to create GLImage";
     return false;
   }
@@ -97,15 +97,15 @@ bool VaapiPictureNativePixmapOzone::Allocate(gfx::BufferFormat format) {
 
   ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance();
   ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone();
-  pixmap_ = factory->CreateNativePixmap(gfx::kNullAcceleratedWidget,
-                                        VK_NULL_HANDLE, size_, format,
+  auto pixmap = factory->CreateNativePixmap(
+      gfx::kNullAcceleratedWidget, VK_NULL_HANDLE, size_, format,
       gfx::BufferUsage::SCANOUT_VDA_WRITE);
-  if (!pixmap_) {
+  if (!pixmap) {
     LOG(ERROR) << "Failed allocating a pixmap";
     return false;
   }
 
-  return Initialize();
+  return Initialize(std::move(pixmap));
 }
 
 bool VaapiPictureNativePixmapOzone::ImportGpuMemoryBufferHandle(
@@ -116,16 +116,16 @@ bool VaapiPictureNativePixmapOzone::ImportGpuMemoryBufferHandle(
   ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance();
   ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone();
   // CreateNativePixmapFromHandle() will take ownership of the handle.
-  pixmap_ = factory->CreateNativePixmapFromHandle(
+  auto pixmap = factory->CreateNativePixmapFromHandle(
       gfx::kNullAcceleratedWidget, size_, format,
       std::move(gpu_memory_buffer_handle.native_pixmap_handle));
 
-  if (!pixmap_) {
+  if (!pixmap) {
     LOG(ERROR) << "Failed creating a pixmap from a native handle";
     return false;
   }
 
-  return Initialize();
+  return Initialize(std::move(pixmap));
 }
 
 }  // namespace media

media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.h

@@ -14,6 +14,10 @@
 #include "ui/gfx/buffer_types.h"
 #include "ui/gfx/geometry/size.h"
 
+namespace gfx {
+class NativePixmap;
+}  // namespace gfx
+
 namespace media {
 
 class VaapiWrapper;
@@ -40,7 +44,7 @@ class VaapiPictureNativePixmapOzone : public VaapiPictureNativePixmap {
       gfx::GpuMemoryBufferHandle gpu_memory_buffer_handle) override;
 
  private:
-  bool Initialize();
+  bool Initialize(scoped_refptr<gfx::NativePixmap> pixmap);
 
   DISALLOW_COPY_AND_ASSIGN(VaapiPictureNativePixmapOzone);
 };

media/gpu/vaapi/vaapi_video_decoder.cc

@@ -16,15 +16,12 @@
 #include "media/base/video_util.h"
 #include "media/gpu/gpu_video_decode_accelerator_helpers.h"
 #include "media/gpu/linux/dmabuf_video_frame_pool.h"
-#include "media/gpu/linux/platform_video_frame_utils.h"
 #include "media/gpu/macros.h"
 #include "media/gpu/vaapi/va_surface.h"
 #include "media/gpu/vaapi/vaapi_h264_accelerator.h"
 #include "media/gpu/vaapi/vaapi_vp8_accelerator.h"
 #include "media/gpu/vaapi/vaapi_vp9_accelerator.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
-#include "ui/gfx/linux/native_pixmap_dmabuf.h"
-#include "ui/gfx/native_pixmap.h"
 
 namespace media {
 
@@ -409,20 +406,11 @@ scoped_refptr<VASurface> VaapiVideoDecoder::CreateSurface() {
 
   frame->set_timestamp(current_decode_task_->buffer_->timestamp());
 
-  // Create a native pixmap from the video frame.
-  scoped_refptr<gfx::NativePixmap> native_pixmap =
-      CreateNativePixmapDmaBuf(frame.get());
-  if (!native_pixmap) {
-    VLOGF(1) << "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(native_pixmap);
+      vaapi_wrapper_->CreateVASurfaceForVideoFrame(frame.get());
   if (!va_surface || va_surface->id() == VA_INVALID_ID) {
-    VLOGF(1) << "Failed to create VASurface from NativePixmap";
+    VLOGF(1) << "Failed to create VASurface from VideoFrame";
     SetState(State::kError);
     return nullptr;
   }

media/gpu/vaapi/vaapi_wrapper.cc

@@ -41,6 +41,7 @@
 // Auto-generated for dlopen libva libraries
 #include "media/gpu/vaapi/va_stubs.h"
 
+#include "media/gpu/linux/platform_video_frame_utils.h"
 #include "media/gpu/vaapi/vaapi_picture.h"
 #include "media/gpu/vaapi/vaapi_utils.h"
 #include "third_party/libyuv/include/libyuv.h"
@@ -1388,8 +1389,19 @@ bool VaapiWrapper::CreateContext(const gfx::Size& size) {
   return va_res == VA_STATUS_SUCCESS;
 }
 
+scoped_refptr<VASurface> VaapiWrapper::CreateVASurfaceForVideoFrame(
+    const VideoFrame* frame) {
+  DCHECK(frame);
+  scoped_refptr<gfx::NativePixmap> pixmap = CreateNativePixmapDmaBuf(frame);
+  if (!pixmap) {
+    LOG(ERROR) << "Failed to create NativePixmap from VideoFrame";
+    return nullptr;
+  }
+  return CreateVASurfaceForPixmap(std::move(pixmap));
+}
+
 scoped_refptr<VASurface> VaapiWrapper::CreateVASurfaceForPixmap(
-    const scoped_refptr<gfx::NativePixmap>& pixmap) {
+    scoped_refptr<gfx::NativePixmap> pixmap) {
   const gfx::BufferFormat buffer_format = pixmap->GetBufferFormat();
 
   // Create a VASurface for a NativePixmap by importing the underlying dmabufs.
@@ -1445,6 +1457,8 @@ scoped_refptr<VASurface> VaapiWrapper::CreateVASurfaceForPixmap(
     VA_SUCCESS_OR_RETURN(va_res, "Failed to create unowned VASurface", nullptr);
   }
 
+  // VASurface shares an ownership of the buffer referred by the passed file
+  // descriptor. We can release |pixmap| here.
   return new VASurface(va_surface_id, size, va_format,
                        base::BindOnce(&VaapiWrapper::DestroySurface, this));
 }

media/gpu/vaapi/vaapi_wrapper.h

@@ -230,10 +230,20 @@ class MEDIA_GPU_EXPORT VaapiWrapper
       const gfx::Size& size,
       const base::Optional<gfx::Size>& visible_size = base::nullopt);
 
-  // Creates a self-releasing VASurface from |pixmap|. The ownership of the
-  // surface is transferred to the caller.
+  // Creates a self-releasing VASurface from |frame|. The created VASurface
+  // doesn't have the ownership of |frame|, while it shares the ownership of the
+  // underlying buffer represented by |frame|. In other words, the buffer is
+  // alive at least until both |frame| and the created VASurface are destroyed.
+  scoped_refptr<VASurface> CreateVASurfaceForVideoFrame(
+      const VideoFrame* frame);
+
+  // Creates a self-releasing VASurface from |pixmap|. The created VASurface
+  // shares the ownership of the underlying buffer represented by |pixmap|. The
+  // ownership of the surface is transferred to the caller. A caller can destroy
+  // |pixmap| after this method returns and the underlying buffer will be kept
+  // alive by the VASurface.
   scoped_refptr<VASurface> CreateVASurfaceForPixmap(
-      const scoped_refptr<gfx::NativePixmap>& pixmap);
+      scoped_refptr<gfx::NativePixmap> pixmap);
 
   // Syncs and exports |va_surface| as a gfx::NativePixmapDmaBuf. Currently, the
   // only VAAPI surface pixel formats supported are VA_FOURCC_IMC3 and