Reland: simplify VaapiPictureFactory::Create() method and VaapiPicture members

This CL is a reland of crrev.com/c/1079834 and crrev.com/c/1086131
that got reverted (the second is just adding a few forgotten DCHECKs).
This is because  GpuArcVideoDecodeAccelerator sends empty client and
texture id vectors [0]; this CL handles gracefully these cases.

[0] https://cs.chromium.org/chromium/src/components/arc/video_accelerator/gpu_arc_video_decode_accelerator.cc?l=468

original cl description ------------------------------------------------

This CL moves part of the logic of VaapiPicture creation from VaVDA to
VaapiPictureFactory, passing the PictureBuffer wholesale since we use
most of it anyway.  The net result is less lines of code.

VaapiPicture and derived classes hold on to a |texture_id_| and a
|client_texture_id_|, the latter only used with |bind_image_cb_|:
- |texture_id_| cannot be zero, either in the tests or in reality, because
  is a GL texture id; this CL enforces this via a DCHECK() in ctor. That
  simplifies the body a few methods.
- |client_texture_id_| can be zero, and is always zero in the v_d_a_unittests,
  but it doesn't matter because |bind_image_cb_| is dummy for tests ([1]),
  so there's no point of enforcing it to be non zero.  Moreover, since
  it's a client-side texture id, it can be any number, so let's skip
  the confusing checks.

Sprinkled a couple of consts and also, there's no need to do
!callback.is_null() because CallbackBase provides bool(), so
s/!callback.is_null()/callback/

[1] https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unittest.cc?sq=package:chromium&dr&g=0&l=452

Bug: 822346
Test: compiled+run simplechrome, v_d_a_unittests on eve. media_unittests.
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I8b211d97e6bcb3f86149224a1c130ca6a24984e8
Reviewed-on: https://chromium-review.googlesource.com/1100535Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567293}

media/gpu/vaapi/vaapi_picture.h

@@ -45,8 +45,7 @@ class MEDIA_GPU_EXPORT VaapiPicture {
 
   virtual bool AllowOverlay() const;
 
-  // Downloads the |va_surface| into the picture, potentially scaling
-  // it if needed.
+  // Downloads |va_surface| into the picture, potentially scaling it if needed.
   virtual bool DownloadFromSurface(
       const scoped_refptr<VASurface>& va_surface) = 0;
 
@@ -60,7 +59,7 @@ class MEDIA_GPU_EXPORT VaapiPicture {
                uint32_t client_texture_id,
                uint32_t texture_target);
 
-  scoped_refptr<VaapiWrapper> vaapi_wrapper_;
+  const scoped_refptr<VaapiWrapper> vaapi_wrapper_;
 
   const MakeGLContextCurrentCallback make_context_current_cb_;
   const BindGLImageCallback bind_image_cb_;

media/gpu/vaapi/vaapi_picture_factory.cc

@@ -5,6 +5,7 @@
 #include "media/gpu/vaapi/vaapi_picture_factory.h"
 
 #include "media/gpu/vaapi/vaapi_wrapper.h"
+#include "media/video/picture.h"
 #include "ui/gl/gl_bindings.h"
 
 #if defined(USE_X11)
@@ -43,12 +44,18 @@ std::unique_ptr<VaapiPicture> VaapiPictureFactory::Create(
     const scoped_refptr<VaapiWrapper>& vaapi_wrapper,
     const MakeGLContextCurrentCallback& make_context_current_cb,
     const BindGLImageCallback& bind_image_cb,
-    int32_t picture_buffer_id,
-    const gfx::Size& size,
-    uint32_t texture_id,
-    uint32_t client_texture_id,
-    uint32_t texture_target) {
-  DCHECK_EQ(texture_target, GetGLTextureTarget());
+    const PictureBuffer& picture_buffer) {
+  DCHECK_EQ(picture_buffer.texture_target(), GetGLTextureTarget());
+
+  // |client_texture_ids| and |service_texture_ids| are empty from ARC++.
+  const uint32_t client_texture_id =
+      !picture_buffer.client_texture_ids().empty()
+          ? picture_buffer.client_texture_ids()[0]
+          : 0;
+  const uint32_t service_texture_id =
+      !picture_buffer.service_texture_ids().empty()
+          ? picture_buffer.service_texture_ids()[0]
+          : 0;
 
   std::unique_ptr<VaapiPicture> picture;
 
@@ -61,25 +68,24 @@ std::unique_ptr<VaapiPicture> VaapiPictureFactory::Create(
     case kVaapiImplementationDrm:
       picture.reset(new VaapiPictureNativePixmapOzone(
           vaapi_wrapper, make_context_current_cb, bind_image_cb,
-          picture_buffer_id, size, texture_id, client_texture_id,
-          texture_target));
+          picture_buffer.id(), picture_buffer.size(), service_texture_id,
+          client_texture_id, picture_buffer.texture_target()));
       break;
 #elif defined(USE_EGL)
     case kVaapiImplementationDrm:
       picture.reset(new VaapiPictureNativePixmapEgl(
           vaapi_wrapper, make_context_current_cb, bind_image_cb,
-          picture_buffer_id, size, texture_id, client_texture_id,
-          texture_target));
+          picture_buffer.id(), picture_buffer.size(), service_texture_id,
+          client_texture_id, picture_buffer.texture_target()));
       break;
 #endif
 
 #if defined(USE_X11)
     case kVaapiImplementationX11:
-      picture.reset(new VaapiTFPPicture(vaapi_wrapper, make_context_current_cb,
-                                        bind_image_cb, picture_buffer_id, size,
-                                        texture_id, client_texture_id,
-                                        texture_target));
-
+      picture.reset(new VaapiTFPPicture(
+          vaapi_wrapper, make_context_current_cb, bind_image_cb,
+          picture_buffer.id(), picture_buffer.size(), service_texture_id,
+          client_texture_id, picture_buffer.texture_target()));
       break;
 #endif  // USE_X11
 

media/gpu/vaapi/vaapi_picture_factory.h

@@ -15,6 +15,7 @@
 
 namespace media {
 
+class PictureBuffer;
 class VaapiWrapper;
 
 // Factory of platform dependent VaapiPictures.
@@ -29,18 +30,13 @@ class MEDIA_GPU_EXPORT VaapiPictureFactory {
   VaapiPictureFactory();
   virtual ~VaapiPictureFactory();
 
-  // Creates a VaapiPicture of |size| associated with |picture_buffer_id|. If
-  // provided, bind it to |texture_id|, as well as to |client_texture_id| using
-  // |bind_image_cb|.
+  // Creates a VaapiPicture of picture_buffer.size() associated with
+  // picture_buffer.id().
   virtual std::unique_ptr<VaapiPicture> Create(
       const scoped_refptr<VaapiWrapper>& vaapi_wrapper,
       const MakeGLContextCurrentCallback& make_context_current_cb,
       const BindGLImageCallback& bind_image_cb,
-      int32_t picture_buffer_id,
-      const gfx::Size& size,
-      uint32_t texture_id,
-      uint32_t client_texture_id,
-      uint32_t texture_target);
+      const PictureBuffer& picture_buffer);
 
   // Return the type of the VaapiPicture implementation for the given GL
   // implementation.

media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc

@@ -33,6 +33,8 @@ VaapiPictureNativePixmapEgl::VaapiPictureNativePixmapEgl(
                                client_texture_id,
                                texture_target) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(texture_id);
+  DCHECK(client_texture_id);
 }
 
 VaapiPictureNativePixmapEgl::~VaapiPictureNativePixmapEgl() {
@@ -58,14 +60,12 @@ bool VaapiPictureNativePixmapEgl::Initialize() {
   // because the dmabuf fds have been made from it.
   DCHECK(pixmap_->AreDmaBufFdsValid());
 
-  if (client_texture_id_ != 0 && !bind_image_cb_.is_null()) {
-    if (!bind_image_cb_.Run(client_texture_id_, texture_target_, gl_image_,
-                            true)) {
-      LOG(ERROR) << "Failed to bind client_texture_id";
-      return false;
-    }
+  if (bind_image_cb_ &&
+      !bind_image_cb_.Run(client_texture_id_, texture_target_, gl_image_,
+                          true /* can_bind_to_sampler */)) {
+    LOG(ERROR) << "Failed to bind client_texture_id";
+    return false;
   }
-
   return true;
 }
 
@@ -73,52 +73,45 @@ bool VaapiPictureNativePixmapEgl::Allocate(gfx::BufferFormat format) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Export the gl texture as dmabuf.
-  if (texture_id_ != 0 && !make_context_current_cb_.is_null()) {
-    if (!make_context_current_cb_.Run())
-      return false;
-
-    scoped_refptr<gl::GLImageNativePixmap> image(new gl::GLImageNativePixmap(
-        size_, BufferFormatToInternalFormat(format)));
-
-    // Create an EGLImage from a gl texture
-    if (!image->InitializeFromTexture(texture_id_)) {
-      DLOG(ERROR) << "Failed to initialize eglimage from texture id: "
-                  << texture_id_;
-      return false;
-    }
-
-    // Export the EGLImage as dmabuf.
-    gfx::NativePixmapHandle native_pixmap_handle = image->ExportHandle();
-    if (!native_pixmap_handle.planes.size()) {
-      DLOG(ERROR) << "Failed to export EGLImage as dmabuf fds";
-      return false;
-    }
-
-    // Convert NativePixmapHandle to NativePixmapDmaBuf.
-    scoped_refptr<gfx::NativePixmap> native_pixmap_dmabuf(
-        new gfx::NativePixmapDmaBuf(size_, format, native_pixmap_handle));
-    if (!native_pixmap_dmabuf->AreDmaBufFdsValid()) {
-      DLOG(ERROR) << "Invalid dmabuf fds";
-      return false;
-    }
-
-    if (!image->BindTexImage(texture_target_)) {
-      DLOG(ERROR) << "Failed to bind texture to GLImage";
-      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;
-    gl_image_ = image;
+  if (make_context_current_cb_ && !make_context_current_cb_.Run())
+    return false;
+
+  scoped_refptr<gl::GLImageNativePixmap> image(
+      new gl::GLImageNativePixmap(size_, BufferFormatToInternalFormat(format)));
+
+  // Create an EGLImage from a gl texture
+  if (!image->InitializeFromTexture(texture_id_)) {
+    DLOG(ERROR) << "Failed to initialize eglimage from texture id: "
+                << texture_id_;
+    return false;
   }
 
-  if (!pixmap_) {
-    DVLOG(1) << "Failed allocating a pixmap";
+  // Export the EGLImage as dmabuf.
+  gfx::NativePixmapHandle native_pixmap_handle = image->ExportHandle();
+  if (!native_pixmap_handle.planes.size()) {
+    DLOG(ERROR) << "Failed to export EGLImage as dmabuf fds";
     return false;
   }
 
+  // Convert NativePixmapHandle to NativePixmapDmaBuf.
+  scoped_refptr<gfx::NativePixmap> native_pixmap_dmabuf(
+      new gfx::NativePixmapDmaBuf(size_, format, native_pixmap_handle));
+  if (!native_pixmap_dmabuf->AreDmaBufFdsValid()) {
+    DLOG(ERROR) << "Invalid dmabuf fds";
+    return false;
+  }
+
+  if (!image->BindTexImage(texture_target_)) {
+    DLOG(ERROR) << "Failed to bind texture to GLImage";
+    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;
+  gl_image_ = image;
+
   return Initialize();
 }
 

media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc

@@ -35,6 +35,9 @@ VaapiPictureNativePixmapOzone::VaapiPictureNativePixmapOzone(
                                client_texture_id,
                                texture_target) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Either |texture_id| and |client_texture_id| are both zero, or not.
+  DCHECK((texture_id == 0 && client_texture_id == 0) ||
+         (texture_id != 0 && client_texture_id != 0));
 }
 
 VaapiPictureNativePixmapOzone::~VaapiPictureNativePixmapOzone() {
@@ -56,34 +59,35 @@ bool VaapiPictureNativePixmapOzone::Initialize() {
     return false;
   }
 
+  // ARC++ has no texture ids.
+  if (texture_id_ == 0 && client_texture_id_ == 0)
+    return true;
+
   // Import dmabuf fds into the output gl texture through EGLImage.
-  if (texture_id_ != 0 && !make_context_current_cb_.is_null()) {
-    if (!make_context_current_cb_.Run())
-      return false;
-
-    gl::ScopedTextureBinder texture_binder(texture_target_, texture_id_);
-
-    gfx::BufferFormat format = pixmap_->GetBufferFormat();
-
-    scoped_refptr<gl::GLImageNativePixmap> image(new gl::GLImageNativePixmap(
-        size_, BufferFormatToInternalFormat(format)));
-    if (!image->Initialize(pixmap_.get(), format)) {
-      LOG(ERROR) << "Failed to create GLImage";
-      return false;
-    }
-    gl_image_ = image;
-    if (!gl_image_->BindTexImage(texture_target_)) {
-      LOG(ERROR) << "Failed to bind texture to GLImage";
-      return false;
-    }
+  if (make_context_current_cb_ && !make_context_current_cb_.Run())
+    return false;
+
+  gl::ScopedTextureBinder texture_binder(texture_target_, texture_id_);
+
+  const gfx::BufferFormat format = pixmap_->GetBufferFormat();
+
+  scoped_refptr<gl::GLImageNativePixmap> image(
+      new gl::GLImageNativePixmap(size_, BufferFormatToInternalFormat(format)));
+  if (!image->Initialize(pixmap_.get(), format)) {
+    LOG(ERROR) << "Failed to create GLImage";
+    return false;
+  }
+  gl_image_ = image;
+  if (!gl_image_->BindTexImage(texture_target_)) {
+    LOG(ERROR) << "Failed to bind texture to GLImage";
+    return false;
   }
 
-  if (client_texture_id_ != 0 && !bind_image_cb_.is_null()) {
-    if (!bind_image_cb_.Run(client_texture_id_, texture_target_, gl_image_,
-                            true)) {
-      LOG(ERROR) << "Failed to bind client_texture_id";
-      return false;
-    }
+  if (bind_image_cb_ &&
+      !bind_image_cb_.Run(client_texture_id_, texture_target_, gl_image_,
+                          true /* can_bind_to_sampler */)) {
+    LOG(ERROR) << "Failed to bind client_texture_id";
+    return false;
   }
 
   return true;
@@ -98,7 +102,7 @@ bool VaapiPictureNativePixmapOzone::Allocate(gfx::BufferFormat format) {
       factory->CreateNativePixmap(gfx::kNullAcceleratedWidget, size_, format,
                                   gfx::BufferUsage::SCANOUT_VDA_WRITE);
   if (!pixmap_) {
-    DVLOG(1) << "Failed allocating a pixmap";
+    LOG(ERROR) << "Failed allocating a pixmap";
     return false;
   }
 
@@ -118,7 +122,7 @@ bool VaapiPictureNativePixmapOzone::ImportGpuMemoryBufferHandle(
       gpu_memory_buffer_handle.native_pixmap_handle);
 
   if (!pixmap_) {
-    DVLOG(1) << "Failed creating a pixmap from a native handle";
+    LOG(ERROR) << "Failed creating a pixmap from a native handle";
     return false;
   }
 

media/gpu/vaapi/vaapi_picture_tfp.cc

@@ -33,6 +33,8 @@ VaapiTFPPicture::VaapiTFPPicture(
       x_display_(gfx::GetXDisplay()),
       x_pixmap_(0) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(texture_id);
+  DCHECK(client_texture_id);
 }
 
 VaapiTFPPicture::~VaapiTFPPicture() {
@@ -50,22 +52,20 @@ bool VaapiTFPPicture::Initialize() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(x_pixmap_);
 
-  if (texture_id_ != 0 && !make_context_current_cb_.is_null()) {
-    if (!make_context_current_cb_.Run())
-      return false;
-
-    glx_image_ = new gl::GLImageGLX(size_, GL_RGB);
-    if (!glx_image_->Initialize(x_pixmap_)) {
-      // x_pixmap_ will be freed in the destructor.
-      DLOG(ERROR) << "Failed creating a GLX Pixmap for TFP";
-      return false;
-    }
-
-    gl::ScopedTextureBinder texture_binder(texture_target_, texture_id_);
-    if (!glx_image_->BindTexImage(texture_target_)) {
-      DLOG(ERROR) << "Failed to bind texture to glx image";
-      return false;
-    }
+  if (make_context_current_cb_ && !make_context_current_cb_.Run())
+    return false;
+
+  glx_image_ = new gl::GLImageGLX(size_, GL_RGB);
+  if (!glx_image_->Initialize(x_pixmap_)) {
+    // x_pixmap_ will be freed in the destructor.
+    DLOG(ERROR) << "Failed creating a GLX Pixmap for TFP";
+    return false;
+  }
+
+  gl::ScopedTextureBinder texture_binder(texture_target_, texture_id_);
+  if (!glx_image_->BindTexImage(texture_target_)) {
+    DLOG(ERROR) << "Failed to bind texture to glx image";
+    return false;
   }
 
   return true;

media/gpu/vaapi/vaapi_video_decode_accelerator.cc

@@ -588,22 +588,12 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
   DCHECK_EQ(va_surface_ids.size(), buffers.size());
 
   for (size_t i = 0; i < buffers.size(); ++i) {
-    uint32_t client_id = !buffers[i].client_texture_ids().empty()
-                             ? buffers[i].client_texture_ids()[0]
-                             : 0;
-    uint32_t service_id = !buffers[i].service_texture_ids().empty()
-                              ? buffers[i].service_texture_ids()[0]
-                              : 0;
-
-    DCHECK_EQ(buffers[i].texture_target(),
-              vaapi_picture_factory_->GetGLTextureTarget());
+    DCHECK(requested_pic_size_ == buffers[i].size());
 
     std::unique_ptr<VaapiPicture> picture(vaapi_picture_factory_->Create(
-        vaapi_wrapper_, make_context_current_cb_, bind_image_cb_,
-        buffers[i].id(), requested_pic_size_, service_id, client_id,
-        buffers[i].texture_target()));
-    RETURN_AND_NOTIFY_ON_FAILURE(
-        picture.get(), "Failed creating a VaapiPicture", PLATFORM_FAILURE, );
+        vaapi_wrapper_, make_context_current_cb_, bind_image_cb_, buffers[i]));
+    RETURN_AND_NOTIFY_ON_FAILURE(picture, "Failed creating a VaapiPicture",
+                                 PLATFORM_FAILURE, );
 
     if (output_mode_ == Config::OutputMode::ALLOCATE) {
       RETURN_AND_NOTIFY_ON_FAILURE(

media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

@@ -116,15 +116,14 @@ class MockVaapiPictureFactory : public VaapiPictureFactory {
       const scoped_refptr<VaapiWrapper>& vaapi_wrapper,
       const MakeGLContextCurrentCallback& make_context_current_cb,
       const BindGLImageCallback& bind_image_cb,
-      int32_t picture_buffer_id,
-      const gfx::Size& size,
-      uint32_t texture_id,
-      uint32_t client_texture_id,
-      uint32_t texture_target) override {
-    MockCreateVaapiPicture(vaapi_wrapper.get(), size);
+      const PictureBuffer& picture_buffer) override {
+    const uint32_t service_texture_id = picture_buffer.service_texture_ids()[0];
+    const uint32_t client_texture_id = picture_buffer.client_texture_ids()[0];
+    MockCreateVaapiPicture(vaapi_wrapper.get(), picture_buffer.size());
     return std::make_unique<MockVaapiPicture>(
         vaapi_wrapper, make_context_current_cb, bind_image_cb,
-        picture_buffer_id, size, texture_id, client_texture_id, texture_target);
+        picture_buffer.id(), picture_buffer.size(), service_texture_id,
+        client_texture_id, picture_buffer.texture_target());
   }
 };
 

media/gpu/video_decode_accelerator_unittest.cc

@@ -528,14 +528,15 @@ void GLRenderingVDAClient::ProvidePictureBuffers(
     LOG_ASSERT(texture_ref);
 
     int32_t picture_buffer_id = next_picture_buffer_id_++;
+    int irrelevant_id = picture_buffer_id;
     LOG_ASSERT(
         active_textures_.insert(std::make_pair(picture_buffer_id, texture_ref))
             .second);
 
     PictureBuffer::TextureIds texture_ids(1, texture_id);
     buffers.push_back(PictureBuffer(picture_buffer_id, dimensions,
-                                    PictureBuffer::TextureIds(), texture_ids,
-                                    texture_target, pixel_format));
+                                    PictureBuffer::TextureIds{irrelevant_id++},
+                                    texture_ids, texture_target, pixel_format));
   }
   decoder_->AssignPictureBuffers(buffers);