Replace GLImageNativePixmap internalformat with BufferFormat

All callers pass an internalformat consistent with
gpu::InternalFormatForGpuMemoryBufferFormat, so there's no need to
specify redundantly the internalformat and the BufferFormat. Instead
just pass in the BufferFormat, which simplifies all callers and
increases consistency.

Bug: 900044
Change-Id: I19916a6f53a19e1419e3312a2ea4368d79fd95e4
Reviewed-on: https://chromium-review.googlesource.com/c/1405286Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622094}

gpu/command_buffer/tests/gl_test_utils.cc

@@ -451,8 +451,7 @@ GpuCommandBufferTestEGL::CreateGLImageNativePixmap(gfx::BufferFormat format,
   EXPECT_NE(0u, tex_service_id);
 
   // Create an EGLImage from the real texture id.
-  scoped_refptr<gl::GLImageNativePixmap> image(new gl::GLImageNativePixmap(
-      size, gl::GLImageNativePixmap::GetInternalFormatForTesting(format)));
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
   bool result = image->InitializeFromTexture(tex_service_id);
   DCHECK(result);
 

gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc

@@ -124,13 +124,10 @@ GpuMemoryBufferFactoryNativePixmap::CreateImageForGpuMemoryBuffer(
     }
   }
 
-  unsigned internalformat = gpu::InternalFormatForGpuMemoryBufferFormat(format);
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size, internalformat));
-  if (!image->Initialize(pixmap.get(), format)) {
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
+  if (!image->Initialize(pixmap.get())) {
     LOG(ERROR) << "Failed to create GLImage " << size.ToString() << ", "
-               << gfx::BufferFormatToString(format) << ", |internalformat|: "
-               << gl::GLEnums::GetStringEnum(internalformat);
+               << gfx::BufferFormatToString(format);
     return nullptr;
   }
   return image;
@@ -164,13 +161,10 @@ GpuMemoryBufferFactoryNativePixmap::CreateAnonymousImage(
                << gfx::BufferFormatToString(format);
     return nullptr;
   }
-  unsigned internalformat = gpu::InternalFormatForGpuMemoryBufferFormat(format);
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size, internalformat));
-  if (!image->Initialize(pixmap.get(), format)) {
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
+  if (!image->Initialize(pixmap.get())) {
     LOG(ERROR) << "Failed to create GLImage " << size.ToString() << ", "
-               << gfx::BufferFormatToString(format) << ", |internalformat|: "
-               << gl::GLEnums::GetStringEnum(internalformat);
+               << gfx::BufferFormatToString(format);
     return nullptr;
   }
   *is_cleared = true;

media/gpu/v4l2/generic_v4l2_device.cc

@@ -332,19 +332,15 @@ scoped_refptr<gl::GLImage> GenericV4L2Device::CreateGLImage(
   }
 
   gfx::BufferFormat buffer_format = gfx::BufferFormat::BGRA_8888;
-  unsigned internal_format = GL_BGRA_EXT;
   switch (fourcc) {
     case DRM_FORMAT_ARGB8888:
       buffer_format = gfx::BufferFormat::BGRA_8888;
-      internal_format = GL_BGRA_EXT;
       break;
     case DRM_FORMAT_NV12:
       buffer_format = gfx::BufferFormat::YUV_420_BIPLANAR;
-      internal_format = GL_RGB_YCBCR_420V_CHROMIUM;
       break;
     case DRM_FORMAT_YVU420:
       buffer_format = gfx::BufferFormat::YVU_420;
-      internal_format = GL_RGB_YCRCB_420_CHROMIUM;
       break;
     default:
       NOTREACHED();
@@ -358,9 +354,9 @@ scoped_refptr<gl::GLImage> GenericV4L2Device::CreateGLImage(
 
   DCHECK(pixmap);
 
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size, internal_format));
-  bool ret = image->Initialize(pixmap.get(), buffer_format);
+  auto image =
+      base::MakeRefCounted<gl::GLImageNativePixmap>(size, buffer_format);
+  bool ret = image->Initialize(pixmap.get());
   DCHECK(ret);
   return image;
 }

media/gpu/vaapi/vaapi_picture_native_pixmap.cc

@@ -50,28 +50,6 @@ VASurfaceID VaapiPictureNativePixmap::va_surface_id() const {
   return va_surface_->id();
 }
 
-unsigned VaapiPictureNativePixmap::BufferFormatToInternalFormat(
-    gfx::BufferFormat format) const {
-  switch (format) {
-    case gfx::BufferFormat::BGRX_8888:
-    case gfx::BufferFormat::RGBX_8888:
-      return GL_RGB;
-
-    case gfx::BufferFormat::BGRA_8888:
-      return GL_BGRA_EXT;
-
-    case gfx::BufferFormat::YVU_420:
-      return GL_RGB_YCRCB_420_CHROMIUM;
-
-    case gfx::BufferFormat::YUV_420_BIPLANAR:
-      return GL_RGB_YCBCR_420V_CHROMIUM;
-
-    default:
-      NOTREACHED() << gfx::BufferFormatToString(format);
-      return GL_BGRA_EXT;
-  }
-}
-
 // static
 gfx::GpuMemoryBufferHandle
 VaapiPictureNativePixmap::CreateGpuMemoryBufferHandleFromVideoFrame(

media/gpu/vaapi/vaapi_picture_native_pixmap.h

@@ -49,8 +49,6 @@ class VaapiPictureNativePixmap : public VaapiPicture {
   bool AllowOverlay() const override;
   VASurfaceID va_surface_id() const override;
 
-  unsigned BufferFormatToInternalFormat(gfx::BufferFormat format) const;
-
  protected:
   // Ozone buffer, the storage of the EGLImage and the VASurface.
   scoped_refptr<gfx::NativePixmap> pixmap_;

media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc

@@ -5,6 +5,7 @@
 #include "media/gpu/vaapi/vaapi_picture_native_pixmap_egl.h"
 
 #include "base/file_descriptor_posix.h"
+#include "gpu/command_buffer/common/gpu_memory_buffer_support.h"
 #include "media/gpu/vaapi/va_surface.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 #include "ui/gfx/linux/native_pixmap_dmabuf.h"
@@ -76,9 +77,7 @@ bool VaapiPictureNativePixmapEgl::Allocate(gfx::BufferFormat format) {
   if (make_context_current_cb_ && !make_context_current_cb_.Run())
     return false;
 
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size_, BufferFormatToInternalFormat(format)));
-
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size_, format);
   // Create an EGLImage from a gl texture
   if (!image->InitializeFromTexture(texture_id_)) {
     DLOG(ERROR) << "Failed to initialize eglimage from texture id: "

media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc

@@ -71,9 +71,8 @@ bool VaapiPictureNativePixmapOzone::Initialize() {
 
   const gfx::BufferFormat format = pixmap_->GetBufferFormat();
 
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size_, BufferFormatToInternalFormat(format)));
-  if (!image->Initialize(pixmap_.get(), format)) {
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size_, format);
+  if (!image->Initialize(pixmap_.get())) {
     LOG(ERROR) << "Failed to create GLImage";
     return false;
   }

ui/gl/gl_image_native_pixmap.cc

@@ -35,33 +35,38 @@
 namespace gl {
 namespace {
 
-bool ValidInternalFormat(unsigned internalformat, gfx::BufferFormat format) {
-  switch (internalformat) {
-    case GL_RGB:
-      return format == gfx::BufferFormat::BGR_565 ||
-             format == gfx::BufferFormat::RGBX_8888 ||
-             format == gfx::BufferFormat::BGRX_8888;
-    case GL_RGB10_A2_EXT:
-      return format == gfx::BufferFormat::RGBX_1010102;
-    case GL_RGB_YCRCB_420_CHROMIUM:
-      return format == gfx::BufferFormat::YVU_420;
-    case GL_RGB_YCBCR_420V_CHROMIUM:
-      return format == gfx::BufferFormat::YUV_420_BIPLANAR;
-    case GL_RGBA:
-      return format == gfx::BufferFormat::RGBA_8888 ||
-             format == gfx::BufferFormat::RGBX_1010102;
-    case GL_BGRA_EXT:
-      return format == gfx::BufferFormat::BGRA_8888 ||
-             format == gfx::BufferFormat::BGRX_1010102;
-    case GL_RED_EXT:
-      return format == gfx::BufferFormat::R_8;
-    case GL_R16_EXT:
-      return format == gfx::BufferFormat::R_16;
-    case GL_RG_EXT:
-      return format == gfx::BufferFormat::RG_88;
-    default:
-      return false;
+// Returns corresponding internalformat if supported, and GL_NONE otherwise.
+unsigned GetInternalFormatFromFormat(gfx::BufferFormat format) {
+  switch (format) {
+    case gfx::BufferFormat::R_8:
+      return GL_RED_EXT;
+    case gfx::BufferFormat::R_16:
+      return GL_R16_EXT;
+    case gfx::BufferFormat::RG_88:
+      return GL_RG_EXT;
+    case gfx::BufferFormat::BGR_565:
+    case gfx::BufferFormat::RGBX_8888:
+    case gfx::BufferFormat::BGRX_8888:
+      return GL_RGB;
+    case gfx::BufferFormat::RGBA_8888:
+      return GL_RGBA;
+    case gfx::BufferFormat::RGBX_1010102:
+      return GL_RGB10_A2_EXT;
+    case gfx::BufferFormat::BGRA_8888:
+      return GL_BGRA_EXT;
+    case gfx::BufferFormat::YVU_420:
+      return GL_RGB_YCRCB_420_CHROMIUM;
+    case gfx::BufferFormat::YUV_420_BIPLANAR:
+      return GL_RGB_YCBCR_420V_CHROMIUM;
+    case gfx::BufferFormat::RGBA_4444:
+    case gfx::BufferFormat::BGRX_1010102:
+    case gfx::BufferFormat::RGBA_F16:
+    case gfx::BufferFormat::UYVY_422:
+      return GL_NONE;
   }
+
+  NOTREACHED();
+  return GL_NONE;
 }
 
 EGLint FourCC(gfx::BufferFormat format) {
@@ -135,9 +140,9 @@ gfx::BufferFormat GetBufferFormatFromFourCCFormat(int format) {
 }  // namespace
 
 GLImageNativePixmap::GLImageNativePixmap(const gfx::Size& size,
-                                         unsigned internalformat)
+                                         gfx::BufferFormat format)
     : GLImageEGL(size),
-      internalformat_(internalformat),
+      format_(format),
       has_image_flush_external_(
           gl::GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_flush_external")),
       has_image_dma_buf_export_(
@@ -145,22 +150,13 @@ GLImageNativePixmap::GLImageNativePixmap(const gfx::Size& size,
 
 GLImageNativePixmap::~GLImageNativePixmap() {}
 
-bool GLImageNativePixmap::Initialize(gfx::NativePixmap* pixmap,
-                                     gfx::BufferFormat format) {
+bool GLImageNativePixmap::Initialize(gfx::NativePixmap* pixmap) {
   DCHECK(!pixmap_);
+  if (GetInternalFormatFromFormat(format_) == GL_NONE) {
+    LOG(ERROR) << "Unsupported format: " << gfx::BufferFormatToString(format_);
+    return false;
+  }
   if (pixmap->AreDmaBufFdsValid()) {
-    if (!ValidFormat(format)) {
-      LOG(ERROR) << "Invalid format: " << gfx::BufferFormatToString(format);
-      return false;
-    }
-
-    if (!ValidInternalFormat(internalformat_, format)) {
-      LOG(ERROR) << "Invalid internalformat: "
-                 << GLEnums::GetStringEnum(internalformat_)
-                 << " for format: " << gfx::BufferFormatToString(format);
-      return false;
-    }
-
     // Note: If eglCreateImageKHR is successful for a EGL_LINUX_DMA_BUF_EXT
     // target, the EGL will take a reference to the dma_buf.
     std::vector<EGLint> attrs;
@@ -169,7 +165,7 @@ bool GLImageNativePixmap::Initialize(gfx::NativePixmap* pixmap,
     attrs.push_back(EGL_HEIGHT);
     attrs.push_back(size_.height());
     attrs.push_back(EGL_LINUX_DRM_FOURCC_EXT);
-    attrs.push_back(FourCC(format));
+    attrs.push_back(FourCC(format_));
 
     const EGLint kLinuxDrmModifiers[] = {EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT,
                                          EGL_DMA_BUF_PLANE1_MODIFIER_LO_EXT,
@@ -215,6 +211,10 @@ bool GLImageNativePixmap::Initialize(gfx::NativePixmap* pixmap,
 }
 
 bool GLImageNativePixmap::InitializeFromTexture(uint32_t texture_id) {
+  if (GetInternalFormatFromFormat(format_) == GL_NONE) {
+    LOG(ERROR) << "Unsupported format: " << gfx::BufferFormatToString(format_);
+    return false;
+  }
   GLContext* current_context = GLContext::GetCurrent();
   if (!current_context || !current_context->IsCurrent(nullptr)) {
     LOG(ERROR) << "No gl context bound to the current thread";
@@ -269,14 +269,16 @@ gfx::NativePixmapHandle GLImageNativePixmap::ExportHandle() {
     return gfx::NativePixmapHandle();
   }
 
-  if (!ValidInternalFormat(internalformat_, format)) {
+  if (format != format_) {
     // A driver has returned a format different than what has been requested.
     // This can happen if RGBX is implemented using RGBA. Otherwise there is
     // a real mistake from the user and we have to fail.
-    if (internalformat_ == GL_RGB && format != gfx::BufferFormat::RGBA_8888) {
-      LOG(ERROR) << "Invalid internalformat: "
-                 << GLEnums::GetStringEnum(internalformat_)
-                 << " for format: " << gfx::BufferFormatToString(format);
+    if (GetInternalFormat() == GL_RGB &&
+        format != gfx::BufferFormat::RGBA_8888) {
+      LOG(ERROR) << "Invalid driver format: "
+                 << gfx::BufferFormatToString(format)
+                 << " for requested format: "
+                 << gfx::BufferFormatToString(format_);
       return gfx::NativePixmapHandle();
     }
   }
@@ -325,7 +327,7 @@ gfx::NativePixmapHandle GLImageNativePixmap::ExportHandle() {
 }
 
 unsigned GLImageNativePixmap::GetInternalFormat() {
-  return internalformat_;
+  return GetInternalFormatFromFormat(format_);
 }
 
 bool GLImageNativePixmap::CopyTexImage(unsigned target) {
@@ -383,67 +385,4 @@ void GLImageNativePixmap::OnMemoryDump(
   // TODO(ericrk): Implement GLImage OnMemoryDump. crbug.com/514914
 }
 
-// static
-unsigned GLImageNativePixmap::GetInternalFormatForTesting(
-    gfx::BufferFormat format) {
-  DCHECK(ValidFormat(format));
-  switch (format) {
-    case gfx::BufferFormat::R_8:
-      return GL_RED_EXT;
-    case gfx::BufferFormat::R_16:
-      return GL_R16_EXT;
-    case gfx::BufferFormat::RG_88:
-      return GL_RG_EXT;
-    case gfx::BufferFormat::BGR_565:
-    case gfx::BufferFormat::RGBX_8888:
-    case gfx::BufferFormat::BGRX_8888:
-      return GL_RGB;
-    case gfx::BufferFormat::RGBA_8888:
-      return GL_RGBA;
-    case gfx::BufferFormat::RGBX_1010102:
-      return GL_RGB10_A2_EXT;
-    case gfx::BufferFormat::BGRA_8888:
-      return GL_BGRA_EXT;
-    case gfx::BufferFormat::YVU_420:
-      return GL_RGB_YCRCB_420_CHROMIUM;
-    case gfx::BufferFormat::YUV_420_BIPLANAR:
-      return GL_RGB_YCBCR_420V_CHROMIUM;
-    case gfx::BufferFormat::RGBA_4444:
-    case gfx::BufferFormat::BGRX_1010102:
-    case gfx::BufferFormat::RGBA_F16:
-    case gfx::BufferFormat::UYVY_422:
-      NOTREACHED();
-      return GL_NONE;
-  }
-
-  NOTREACHED();
-  return GL_NONE;
-}
-
-// static
-bool GLImageNativePixmap::ValidFormat(gfx::BufferFormat format) {
-  switch (format) {
-    case gfx::BufferFormat::R_8:
-    case gfx::BufferFormat::R_16:
-    case gfx::BufferFormat::RG_88:
-    case gfx::BufferFormat::BGR_565:
-    case gfx::BufferFormat::RGBA_8888:
-    case gfx::BufferFormat::RGBX_8888:
-    case gfx::BufferFormat::BGRA_8888:
-    case gfx::BufferFormat::BGRX_8888:
-    case gfx::BufferFormat::RGBX_1010102:
-    case gfx::BufferFormat::YVU_420:
-    case gfx::BufferFormat::YUV_420_BIPLANAR:
-      return true;
-    case gfx::BufferFormat::RGBA_4444:
-    case gfx::BufferFormat::BGRX_1010102:
-    case gfx::BufferFormat::RGBA_F16:
-    case gfx::BufferFormat::UYVY_422:
-      return false;
-  }
-
-  NOTREACHED();
-  return false;
-}
-
 }  // namespace gl

ui/gl/gl_image_native_pixmap.h

@@ -17,10 +17,10 @@ namespace gl {
 
 class GL_EXPORT GLImageNativePixmap : public gl::GLImageEGL {
  public:
-  GLImageNativePixmap(const gfx::Size& size, unsigned internalformat);
+  GLImageNativePixmap(const gfx::Size& size, gfx::BufferFormat format);
 
   // Create an EGLImage from a given NativePixmap.
-  bool Initialize(gfx::NativePixmap* pixmap, gfx::BufferFormat format);
+  bool Initialize(gfx::NativePixmap* pixmap);
   // Create an EGLImage from a given GL texture.
   bool InitializeFromTexture(uint32_t texture_id);
   // Export the wrapped EGLImage to dmabuf fds.
@@ -45,15 +45,11 @@ class GL_EXPORT GLImageNativePixmap : public gl::GLImageEGL {
                     uint64_t process_tracing_id,
                     const std::string& dump_name) override;
 
-  static unsigned GetInternalFormatForTesting(gfx::BufferFormat format);
-
  protected:
   ~GLImageNativePixmap() override;
 
  private:
-  static bool ValidFormat(gfx::BufferFormat format);
-
-  unsigned internalformat_;
+  gfx::BufferFormat format_;
   scoped_refptr<gfx::NativePixmap> pixmap_;
   bool has_image_flush_external_;
   bool has_image_dma_buf_export_;

ui/gl/gl_image_native_pixmap_unittest.cc

@@ -49,8 +49,7 @@ class GLImageNativePixmapTestDelegate : public GLImageTestDelegateBase {
     glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, size.width(), size.height(), 0,
                  GL_RGBA, GL_UNSIGNED_BYTE, pixels.get());
 
-    scoped_refptr<gl::GLImageNativePixmap> image(new gl::GLImageNativePixmap(
-        size, gl::GLImageNativePixmap::GetInternalFormatForTesting(format)));
+    auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
     EXPECT_TRUE(image->InitializeFromTexture(texture_id));
 
     glDeleteTextures(1, &texture_id);

ui/ozone/demo/skia/skia_surfaceless_gl_renderer.cc

@@ -114,9 +114,8 @@ bool SurfacelessSkiaGlRenderer::BufferWrapper::Initialize(
       OzonePlatform::GetInstance()
           ->GetSurfaceFactoryOzone()
           ->CreateNativePixmap(widget, size, format, gfx::BufferUsage::SCANOUT);
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size, GL_BGRA_EXT));
-  if (!image->Initialize(pixmap.get(), format)) {
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
+  if (!image->Initialize(pixmap.get())) {
     LOG(ERROR) << "Failed to create GLImage";
     return false;
   }

ui/ozone/demo/surfaceless_gl_renderer.cc

@@ -85,9 +85,8 @@ bool SurfacelessGlRenderer::BufferWrapper::Initialize(
       OzonePlatform::GetInstance()
           ->GetSurfaceFactoryOzone()
           ->CreateNativePixmap(widget, size, format, gfx::BufferUsage::SCANOUT);
-  scoped_refptr<gl::GLImageNativePixmap> image(
-      new gl::GLImageNativePixmap(size, GL_BGRA_EXT));
-  if (!image->Initialize(pixmap.get(), format)) {
+  auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
+  if (!image->Initialize(pixmap.get())) {
     LOG(ERROR) << "Failed to create GLImage";
     return false;
   }

ui/ozone/gl/gl_image_ozone_native_pixmap_unittest.cc

@@ -55,9 +55,8 @@ class GLImageNativePixmapTestDelegate : public GLImageTestDelegateBase {
       client_pixmap->Unmap();
     }
 
-    scoped_refptr<gl::GLImageNativePixmap> image(new gl::GLImageNativePixmap(
-        size, gl::GLImageNativePixmap::GetInternalFormatForTesting(format)));
-    EXPECT_TRUE(image->Initialize(pixmap.get(), pixmap->GetBufferFormat()));
+    auto image = base::MakeRefCounted<gl::GLImageNativePixmap>(size, format);
+    EXPECT_TRUE(image->Initialize(pixmap.get()));
     return image;
   }