GLHelper: Address inconsistent mapping of SkColorType to GL format

content/common/gpu/client/gl_helper.cc

Index: content/common/gpu/client/gl_helper.cc
diff --git a/content/common/gpu/client/gl_helper.cc b/content/common/gpu/client/gl_helper.cc
index d172c560278a6dc40d0bdcd589a7bb64a7d6cef1..779879342c1ae917ee5f76fdb87680880c78305c 100644
--- a/content/common/gpu/client/gl_helper.cc
+++ b/content/common/gpu/client/gl_helper.cc
@@ -162,8 +162,9 @@ class GLHelper::CopyTextureToImpl
                      int32 bytes_per_row,     // generally dst_size.width() * 4
                      int32 row_stride_bytes,  // generally dst_size.width() * 4
                      unsigned char* out,
-                     const SkColorType color_type,
-                     ReadbackSwizzle swizzle,
+                     GLenum format,
+                     GLenum type,
+                     int32 bytes_per_pixel,
                      const base::Callback<void(bool)>& callback);
 
   void ReadbackPlane(TextureFrameBufferPair* source,
@@ -171,7 +172,6 @@ class GLHelper::CopyTextureToImpl
                      int plane,
                      int size_shift,
                      const gfx::Rect& dst_subrect,
-                     ReadbackSwizzle swizzle,
                      const base::Callback<void(bool)>& callback);
 
   GLuint CopyAndScaleTexture(GLuint texture,
@@ -193,8 +193,11 @@ class GLHelper::CopyTextureToImpl
   // 0 if GL_EXT_draw_buffers is not available.
   GLint MaxDrawBuffers() const { return max_draw_buffers_; }
 
-  bool IsReadbackConfigSupported(SkColorType color_type);
-
+  FormatSupport ReadbackConfig(SkColorType color_type,

[piman, 2014/07/22 19:57:55]

nit: naming... GetReadbackConfig ?

[robert.bradford, 2014/07/23 16:58:45]

Done.

+                               const bool& can_swizzle,

[piman, 2014/07/22 19:57:54]

use bool instead of const bool&

[robert.bradford, 2014/07/23 16:58:44]

Done.

+                               GLint& format,

[piman, 2014/07/22 19:57:55]

non-const references are forbidden by style guide. Use pointers.
GLenum instead of GLint?

[robert.bradford, 2014/07/23 16:58:44]

Done.

+                               GLint& type,

[piman, 2014/07/22 19:57:54]

GLenum

[robert.bradford, 2014/07/23 16:58:45]

Done.

+                               int32& bytes_per_pixel);

[piman, 2014/07/22 19:57:55]

nit: size_t instead of int32.

[robert.bradford, 2014/07/23 16:58:45]

Done.

  private:
   // A single request to CropScaleReadbackAndCleanTexture.
   // The main thread can cancel the request, before it's handled by the helper
@@ -364,25 +367,16 @@ GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
     bool swizzle,
     SkColorType color_type,
     GLHelper::ScalerQuality quality) {
-  if (!IsReadbackConfigSupported(color_type))
-    return 0;
-
-  scoped_ptr<ScalerInterface> scaler(
-      helper_->CreateScaler(quality,
-                            src_size,
-                            src_subrect,
-                            dst_size,
-                            vertically_flip_texture,
-                            swizzle));
   GLuint dst_texture = 0u;
-  // Start with ARGB8888 params as any other format which is not
-  // supported is already asserted above.
-  GLenum format = GL_RGBA , type = GL_UNSIGNED_BYTE;
   gl_->GenTextures(1, &dst_texture);
   {
+    GLenum format = GL_RGBA , type = GL_UNSIGNED_BYTE;

[piman, 2014/07/22 19:57:54]

nit: no space before ,

[robert.bradford, 2014/07/23 16:58:44]

Done.

     ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, dst_texture);
     switch (color_type) {
-      case kN32_SkColorType:
+      // For creating the scaled texture the layout should be GL_RGBA
+      // independent of any desired readback layout

[piman, 2014/07/22 19:57:55]

I don't understand this comment. Does it only apply to both RGBA_8888/BGRA_8888?
Can we move it to l.380?

[robert.bradford, 2014/07/23 16:58:45]

This is the initialisation of the texture for the creation of the pure copy and
scale codepath as well as the scale and readback code path.

The color_type that comes in has a somewhat overloaded meaning, both the texture
format of the source as well as the destination readback format. However for the
texture source/temporary texture all we really care about is if we're doing
GL_RGB 16-bit or GL_RGBA 32-bit.

I've modified the code to make this clearer along with a comment.

+      case kRGBA_8888_SkColorType:
+      case kBGRA_8888_SkColorType:
         // Do nothing params already set.
         break;
       case kRGB_565_SkColorType:
@@ -390,8 +384,7 @@ GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
         type = GL_UNSIGNED_SHORT_5_6_5;
         break;
       default:
-        NOTREACHED();
-        break;
+        return 0;
     }
     gl_->TexImage2D(GL_TEXTURE_2D,
                     0,
@@ -403,6 +396,13 @@ GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
                     type,
                     NULL);
   }
+  scoped_ptr<ScalerInterface> scaler(
+      helper_->CreateScaler(quality,
+                            src_size,
+                            src_subrect,
+                            dst_size,
+                            vertically_flip_texture,
+                            swizzle));
   scaler->Scale(src_texture, dst_texture);
   return dst_texture;
 }
@@ -412,36 +412,15 @@ void GLHelper::CopyTextureToImpl::ReadbackAsync(
     int32 bytes_per_row,
     int32 row_stride_bytes,
     unsigned char* out,
-    const SkColorType color_type,
-    ReadbackSwizzle swizzle,
+    GLenum format,
+    GLenum type,
+    int32 bytes_per_pixel,

[piman, 2014/07/22 19:57:54]

size_t

[robert.bradford, 2014/07/23 16:58:44]

Done.

     const base::Callback<void(bool)>& callback) {
-  if (!IsReadbackConfigSupported(color_type)) {
-    callback.Run(false);
-    return;
-  }
   Request* request =
       new Request(dst_size, bytes_per_row, row_stride_bytes, out, callback);
   request_queue_.push(request);
   request->buffer = 0u;
-  // Start with ARGB8888 params as any other format which is not
-  // supported is already asserted above.
-  GLenum format = GL_RGBA, type = GL_UNSIGNED_BYTE;
-  int bytes_per_pixel = 4;
-
-  switch (color_type) {
-    case kN32_SkColorType:
-      if (swizzle == kSwizzleBGRA)
-        format = GL_BGRA_EXT;
-      break;
-    case kRGB_565_SkColorType:
-      format = GL_RGB;
-      type = GL_UNSIGNED_SHORT_5_6_5;
-      bytes_per_pixel = 2;
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
+
   gl_->GenBuffers(1, &request->buffer);
   gl_->BindBuffer(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, request->buffer);
   gl_->BufferData(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM,
@@ -475,20 +454,23 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     const SkColorType color_type,
     const base::Callback<void(bool)>& callback,
     GLHelper::ScalerQuality quality) {
-  if (!IsReadbackConfigSupported(color_type)) {
+  GLint format, type, bytes_per_pixel;
+  FormatSupport supported = ReadbackConfig(color_type,
+                                           true,
+                                           format,
+                                           type,
+                                           bytes_per_pixel);
+  if (supported == kReadbackNotSupported) {
     callback.Run(false);
     return;
   }
+
   GLuint texture = ScaleTexture(src_texture,
                                 src_size,
                                 src_subrect,
                                 dst_size,
                                 true,
-#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT
-                                true,
-#else
-                                false,
-#endif
+                                (supported == kReadbackSwizzle ? true : false),

[piman, 2014/07/22 19:57:55]

? true : false unneeded. (supported == kReadbackSwizzle) is already a bool.

[robert.bradford, 2014/07/23 16:58:45]

Done.

                                 color_type,
                                 quality);
   DCHECK(texture);
@@ -501,24 +483,13 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
                             GL_TEXTURE_2D,
                             texture,
                             0);
-  int bytes_per_pixel = 4;
-  switch (color_type) {
-    case kN32_SkColorType:
-      // Do nothing params already set.
-      break;
-    case kRGB_565_SkColorType:
-      bytes_per_pixel = 2;
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
   ReadbackAsync(dst_size,
                 dst_size.width() * bytes_per_pixel,
                 dst_size.width() * bytes_per_pixel,
                 out,
-                color_type,
-                kSwizzleNone,
+                format,
+                type,
+                bytes_per_pixel,
                 callback);
   gl_->DeleteTextures(1, &texture);
 }
@@ -528,8 +499,15 @@ void GLHelper::CopyTextureToImpl::ReadbackTextureSync(
     const gfx::Rect& src_rect,
     unsigned char* out,
     SkColorType color_type) {
-  if (!IsReadbackConfigSupported(color_type))
+  GLint format, type, bytes_per_pixel;
+  FormatSupport supported = ReadbackConfig(color_type,
+                                           false,
+                                           format,
+                                           type,
+                                           bytes_per_pixel);
+  if (supported == kReadbackNotSupported) {
     return;
+  }
 
   ScopedFramebuffer dst_framebuffer(gl_);
   ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(gl_,
@@ -537,11 +515,6 @@ void GLHelper::CopyTextureToImpl::ReadbackTextureSync(
   ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, texture);
   gl_->FramebufferTexture2D(
       GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
-  GLenum format =
-      (color_type == kRGB_565_SkColorType) ? GL_RGB : GL_RGBA;
-  GLenum type = (color_type == kRGB_565_SkColorType)
-                    ? GL_UNSIGNED_SHORT_5_6_5
-                    : GL_UNSIGNED_BYTE;
   gl_->ReadPixels(src_rect.x(),
                   src_rect.y(),
                   src_rect.width(),
@@ -557,8 +530,16 @@ void GLHelper::CopyTextureToImpl::ReadbackTextureAsync(
     unsigned char* out,
     SkColorType color_type,
     const base::Callback<void(bool)>& callback) {
-  if (!IsReadbackConfigSupported(color_type))
+  GLint format, type, bytes_per_pixel;
+  FormatSupport supported = ReadbackConfig(color_type,
+                                           false,
+                                           format,
+                                           type,
+                                           bytes_per_pixel);
+  if (supported == kReadbackNotSupported) {
+    callback.Run(false);
     return;
+  }
 
   ScopedFramebuffer dst_framebuffer(gl_);
   ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(gl_,
@@ -569,13 +550,13 @@ void GLHelper::CopyTextureToImpl::ReadbackTextureAsync(
                             GL_TEXTURE_2D,
                             texture,
                             0);
-  int bytes_per_pixel = (color_type == kRGB_565_SkColorType) ? 2 : 4;
   ReadbackAsync(dst_size,
                 dst_size.width() * bytes_per_pixel,
                 dst_size.width() * bytes_per_pixel,
                 out,
-                color_type,
-                kSwizzleNone,
+                format,
+                type,
+                bytes_per_pixel,
                 callback);
 }
 
@@ -591,19 +572,10 @@ GLuint GLHelper::CopyTextureToImpl::CopyAndScaleTexture(
                       dst_size,
                       vertically_flip_texture,
                       false,
-                      kN32_SkColorType,
+                      kRGBA_8888_SkColorType, // GL_RGBA

[piman, 2014/07/22 19:57:54]

nit: 2 spaces before //

[robert.bradford, 2014/07/23 16:58:45]

Done.

                       quality);
 }
 
-bool GLHelper::CopyTextureToImpl::IsReadbackConfigSupported(
-    SkColorType color_type) {
-  if (!helper_) {
-    DCHECK(helper_);
-    return false;
-  }
-  return helper_->IsReadbackConfigSupported(color_type);
-}
-
 void GLHelper::CopyTextureToImpl::ReadbackDone(Request* finished_request,
                                                int bytes_per_pixel) {
   TRACE_EVENT0("mirror",
@@ -669,6 +641,21 @@ void GLHelper::CopyTextureToImpl::CancelRequests() {
   }
 }
 
+
+FormatSupport GLHelper::CopyTextureToImpl::ReadbackConfig(
+    SkColorType color_type,
+    const bool& can_swizzle,
+    GLint& format,
+    GLint& type,
+    int32& bytes_per_pixel) {
+  return helper_->readback_support_->ReadbackConfig(
+      color_type,
+      can_swizzle,
+      format,
+      type,
+      bytes_per_pixel);
+}
+
 GLHelper::GLHelper(GLES2Interface* gl, gpu::ContextSupport* context_support)
     : gl_(gl),
       context_support_(context_support),
@@ -906,17 +893,24 @@ void GLHelper::CopyTextureToImpl::ReadbackPlane(
     int plane,
     int size_shift,
     const gfx::Rect& dst_subrect,
-    ReadbackSwizzle swizzle,
     const base::Callback<void(bool)>& callback) {
   gl_->BindFramebuffer(GL_FRAMEBUFFER, source->framebuffer());
   size_t offset = target->stride(plane) * (dst_subrect.y() >> size_shift) +
       (dst_subrect.x() >> size_shift);
+  GLint format, type, bytes_per_pixel;
+  // Pass true in for swizzle to match earlier call

[piman, 2014/07/22 19:57:55]

What does this comment mean?

[robert.bradford, 2014/07/23 16:58:45]

By adopting your proposed solution in the YUVReadbackImpl constructor this
comment and call has now gone.

+  ReadbackConfig(kRGBA_8888_SkColorType,
+                 true,
+                 format,
+                 type,
+                 bytes_per_pixel);
   ReadbackAsync(source->size(),
                 dst_subrect.width() >> size_shift,
                 target->stride(plane),
                 target->data(plane) + offset,
-                kN32_SkColorType,
-                swizzle,
+                format,
+                type,
+                bytes_per_pixel,
                 callback);
 }
 
@@ -1033,14 +1027,12 @@ void GLHelper::CopyTextureToImpl::ReadbackYUVImpl::ReadbackYUV(
                             media::VideoFrame::kYPlane,
                             0,
                             dst_subrect_,
-                            swizzle_,

[piman, 2014/07/22 19:57:54]

I would prefer if we didn't query ReadbackConfig twice with the same parameters
for 2 different reasons (which shader to use, and which format to read back in)
and expect them to match. It's very confusing.

Instead, you can either:
1- keep this, and DCHECK the format/type/bpp combination in
CreateReadbackPipelineYUV (since we can't handle anything but GL_RGBA or
GL_BGRA_EXT on this path, with GL_UNSIGNED_BYTE). Then in ReadbackPlane, you can
choose the format based on this flag (GL_RGBA or GL_BGRA_EXT), and keep
GL_UNSIGNED_BYTE/4 for type/bpp
2- pass the triplet all the way through.

[robert.bradford, 2014/07/23 16:58:45]

I implemented option 1. here.

                             base::Bind(&nullcallback));
   copy_impl_->ReadbackPlane(u_.texture_and_framebuffer(),
                             target,
                             media::VideoFrame::kUPlane,
                             1,
                             dst_subrect_,
-                            swizzle_,
                             base::Bind(&nullcallback));
   copy_impl_->ReadbackPlane(
       v_.texture_and_framebuffer(),
@@ -1048,7 +1040,6 @@ void GLHelper::CopyTextureToImpl::ReadbackYUVImpl::ReadbackYUV(
       media::VideoFrame::kVPlane,
       1,
       dst_subrect_,
-      swizzle_,
       base::Bind(&CallbackKeepingVideoFrameAlive, target, callback));
   gl_->BindFramebuffer(GL_FRAMEBUFFER, 0);
   media::LetterboxYUV(target, dst_subrect_);
@@ -1171,14 +1162,12 @@ void GLHelper::CopyTextureToImpl::ReadbackYUV_MRT::ReadbackYUV(
                             media::VideoFrame::kYPlane,
                             0,
                             dst_subrect_,
-                            swizzle_,
                             base::Bind(&nullcallback));
   copy_impl_->ReadbackPlane(&u_,
                             target,
                             media::VideoFrame::kUPlane,
                             1,
                             dst_subrect_,
-                            swizzle_,
                             base::Bind(&nullcallback));
   copy_impl_->ReadbackPlane(
       &v_,
@@ -1186,15 +1175,21 @@ void GLHelper::CopyTextureToImpl::ReadbackYUV_MRT::ReadbackYUV(
       media::VideoFrame::kVPlane,
       1,
       dst_subrect_,
-      swizzle_,
       base::Bind(&CallbackKeepingVideoFrameAlive, target, callback));
   gl_->BindFramebuffer(GL_FRAMEBUFFER, 0);
   media::LetterboxYUV(target, dst_subrect_);
 }
 
-bool GLHelper::IsReadbackConfigSupported(SkColorType texture_format) {
+bool GLHelper::IsReadbackConfigSupported(SkColorType color_type) {
   DCHECK(readback_support_.get());
-  return readback_support_.get()->IsReadbackConfigSupported(texture_format);
+  GLint format, type, bytes_per_pixel;
+  FormatSupport support = readback_support_->ReadbackConfig(color_type,
+                                                            false,
+                                                            format,
+                                                            type,
+                                                            bytes_per_pixel);
+
+  return (support == kReadbackSupported);
 }
 
 ReadbackYUVInterface* GLHelper::CopyTextureToImpl::CreateReadbackPipelineYUV(
@@ -1206,15 +1201,20 @@ ReadbackYUVInterface* GLHelper::CopyTextureToImpl::CreateReadbackPipelineYUV(
     bool flip_vertically,
     bool use_mrt) {
   helper_->InitScalerImpl();
-  // Query preferred format for glReadPixels, if is is GL_BGRA then use that
-  // and trigger the appropriate swizzle in the YUV shaders.
-  GLint format = 0, type = 0;
+  // Just query if the best readback configuration needs a swizzle In
+  // ReadbackPlane() we make the same request but use the format and type
+  // returned
+  GLint format, type, bytes_per_pixel;
+  FormatSupport supported = ReadbackConfig(kRGBA_8888_SkColorType,
+                                           true,
+                                           format,
+                                           type,
+                                           bytes_per_pixel);
+
   ReadbackSwizzle swizzle = kSwizzleNone;
-  helper_->readback_support_.get()->GetAdditionalFormat(GL_RGBA,
-                                                        GL_UNSIGNED_BYTE,
-                                                        &format, &type);
-  if (format == GL_BGRA_EXT && type == GL_UNSIGNED_BYTE)
+  if (supported == kReadbackSwizzle)
     swizzle = kSwizzleBGRA;
+
   if (max_draw_buffers_ >= 2 && use_mrt) {
     return new ReadbackYUV_MRT(gl_,
                                this,