GLHelper: Address inconsistent mapping of SkColorType to GL format

content/common/gpu/client/gl_helper.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/gpu/client/gl_helper.h"
 
 #include <queue>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 144 matching lines @@
                             unsigned char* out,
                             SkColorType color_type,
                             const base::Callback<void(bool)>& callback);
 
   // Reads back bytes from the currently bound frame buffer.
   // Note that dst_size is specified in bytes, not pixels.
   void ReadbackAsync(const gfx::Size& dst_size,
                      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,
                      const scoped_refptr<media::VideoFrame>& target,
                      int plane,
                      int size_shift,
                      const gfx::Rect& dst_subrect,
-                     ReadbackSwizzle swizzle,
                      const base::Callback<void(bool)>& callback);
 
   GLuint CopyAndScaleTexture(GLuint texture,
                              const gfx::Size& src_size,
                              const gfx::Size& dst_size,
                              bool vertically_flip_texture,
                              GLHelper::ScalerQuality quality);
 
   ReadbackYUVInterface* CreateReadbackPipelineYUV(
       GLHelper::ScalerQuality quality,
       const gfx::Size& src_size,
       const gfx::Rect& src_subrect,
       const gfx::Size& dst_size,
       const gfx::Rect& dst_subrect,
       bool flip_vertically,
       bool use_mrt);
 
   // Returns the maximum number of draw buffers available,
   // 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
   // thread, by resetting the texture and pixels fields. Alternatively, the
   // thread marks that it handles the request by resetting the pixels field
   // (meaning it guarantees that the callback with be called).
   // In either case, the callback must be called exactly once, and the texture
   // must be deleted by the main thread gl.
   struct Request {
     Request(const gfx::Size& size_,
@@ skipping 149 matching lines @@
 
 GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
     GLuint src_texture,
     const gfx::Size& src_size,
     const gfx::Rect& src_subrect,
     const gfx::Size& dst_size,
     bool vertically_flip_texture,
     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:
         format = GL_RGB;
         type = GL_UNSIGNED_SHORT_5_6_5;
         break;
       default:
-        NOTREACHED();
-        break;
+        return 0;
     }
     gl_->TexImage2D(GL_TEXTURE_2D,
                     0,
                     format,
                     dst_size.width(),
                     dst_size.height(),
                     0,
                     format,
                     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;
 }
 
 void GLHelper::CopyTextureToImpl::ReadbackAsync(
     const gfx::Size& dst_size,
     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,
                   bytes_per_pixel * dst_size.GetArea(),
                   NULL,
                   GL_STREAM_READ);
 
   request->query = 0u;
   gl_->GenQueriesEXT(1, &request->query);
   gl_->BeginQueryEXT(GL_ASYNC_PIXEL_PACK_COMPLETED_CHROMIUM, request->query);
@@ skipping 13 matching lines @@
 }
 void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     GLuint src_texture,
     const gfx::Size& src_size,
     const gfx::Rect& src_subrect,
     const gfx::Size& dst_size,
     unsigned char* out,
     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);
   ScopedFramebuffer dst_framebuffer(gl_);
   ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(gl_,
                                                              dst_framebuffer);
   ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, texture);
   gl_->FramebufferTexture2D(GL_FRAMEBUFFER,
                             GL_COLOR_ATTACHMENT0,
                             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);
 }
 
 void GLHelper::CopyTextureToImpl::ReadbackTextureSync(
     GLuint texture,
     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_,
                                                              dst_framebuffer);
   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(),
                   src_rect.height(),
                   format,
                   type,
                   out);
 }
 
 void GLHelper::CopyTextureToImpl::ReadbackTextureAsync(
     GLuint texture,
     const gfx::Size& dst_size,
     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_,
                                                              dst_framebuffer);
   ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, texture);
   gl_->FramebufferTexture2D(GL_FRAMEBUFFER,
                             GL_COLOR_ATTACHMENT0,
                             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);
 }
 
 GLuint GLHelper::CopyTextureToImpl::CopyAndScaleTexture(
     GLuint src_texture,
     const gfx::Size& src_size,
     const gfx::Size& dst_size,
     bool vertically_flip_texture,
     GLHelper::ScalerQuality quality) {
   return ScaleTexture(src_texture,
                       src_size,
                       gfx::Rect(src_size),
                       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",
                "GLHelper::CopyTextureToImpl::CheckReadbackFramebufferComplete");
   finished_request->done = true;
 
   // We process transfer requests in the order they were received, regardless
   // of the order we get the callbacks in.
   while (!request_queue_.empty()) {
     Request* request = request_queue_.front();
@@ skipping 45 matching lines @@
   delete request;
 }
 
 void GLHelper::CopyTextureToImpl::CancelRequests() {
   while (!request_queue_.empty()) {
     Request* request = request_queue_.front();
     FinishRequest(request, false);
   }
 }
 
+
+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),
       readback_support_(new GLHelperReadbackSupport(gl)) {}
 
 GLHelper::~GLHelper() {}
 
 void GLHelper::CropScaleReadbackAndCleanTexture(
     GLuint src_texture,
     const gfx::Size& src_size,
@@ skipping 217 matching lines @@
 void GLHelper::Flush() {
   gl_->Flush();
 }
 
 void GLHelper::CopyTextureToImpl::ReadbackPlane(
     TextureFrameBufferPair* source,
     const scoped_refptr<media::VideoFrame>& target,
     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);
 }
 
 const float GLHelper::CopyTextureToImpl::kRGBtoYColorWeights[] = {
     0.257f, 0.504f, 0.098f, 0.0625f};
 const float GLHelper::CopyTextureToImpl::kRGBtoUColorWeights[] = {
     -0.148f, -0.291f, 0.439f, 0.5f};
 const float GLHelper::CopyTextureToImpl::kRGBtoVColorWeights[] = {
     0.439f, -0.368f, -0.071f, 0.5f};
 
@@ skipping 96 matching lines @@
     return;
   }
 
   // Read back planes, one at a time. Keep the video frame alive while doing the
   // readback.
   copy_impl_->ReadbackPlane(y_.texture_and_framebuffer(),
                             target,
                             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(),
       target,
       media::VideoFrame::kVPlane,
       1,
       dst_subrect_,
-      swizzle_,
       base::Bind(&CallbackKeepingVideoFrameAlive, target, callback));
   gl_->BindFramebuffer(GL_FRAMEBUFFER, 0);
   media::LetterboxYUV(target, dst_subrect_);
 }
 
 // YUV readback constructors. Initiates the main scaler pipeline and
 // one planar scaler for each of the Y, U and V planes.
 GLHelper::CopyTextureToImpl::ReadbackYUV_MRT::ReadbackYUV_MRT(
     GLES2Interface* gl,
     CopyTextureToImpl* copy_impl,
@@ skipping 102 matching lines @@
     callback.Run(false);
     return;
   }
 
   // Read back planes, one at a time.
   copy_impl_->ReadbackPlane(&y_,
                             target,
                             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_,
       target,
       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(
     GLHelper::ScalerQuality quality,
     const gfx::Size& src_size,
     const gfx::Rect& src_subrect,
     const gfx::Size& dst_size,
     const gfx::Rect& dst_subrect,
     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,
                                helper_->scaler_impl_.get(),
                                quality,
                                src_size,
                                src_subrect,
                                dst_size,
                                dst_subrect,
                                flip_vertically,
@@ skipping 23 matching lines @@
   return copy_texture_to_impl_->CreateReadbackPipelineYUV(quality,
                                                           src_size,
                                                           src_subrect,
                                                           dst_size,
                                                           dst_subrect,
                                                           flip_vertically,
                                                           use_mrt);
 }
 
 }  // namespace content