GLHelper: Address inconsistent mapping of SkColorType to GL format

content/common/gpu/client/gl_helper_readback_support.cc

 // Copyright 2014 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_readback_support.h"
 #include "base/logging.h"
+#include "gpu/GLES2/gl2extchromium.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 
 namespace content {
 
 GLHelperReadbackSupport::GLHelperReadbackSupport(gpu::gles2::GLES2Interface* gl)
     : gl_(gl) {
   InitializeReadbackSupport();
 }
 
 GLHelperReadbackSupport::~GLHelperReadbackSupport() {}
 
 void GLHelperReadbackSupport::InitializeReadbackSupport() {
   // We are concerned about 16, 32-bit formats only.
   // The below are the most used 16, 32-bit formats.
   // In future if any new format support is needed that should be added here.
-  // Initialize the array with FORMAT_NOT_SUPPORTED as we dont know the
+  // Initialize the array with kReadbackNotSupported as we dont know the
   // supported formats yet.
   for (int i = 0; i <= kLastEnum_SkColorType; ++i) {
-    format_support_table_[i] = FORMAT_NOT_SUPPORTED;
+    format_support_table_[i] = kReadbackNotSupported;
   }
   CheckForReadbackSupport(kRGB_565_SkColorType);
   CheckForReadbackSupport(kARGB_4444_SkColorType);
-  CheckForReadbackSupport(kN32_SkColorType);
+  CheckForReadbackSupport(kRGBA_8888_SkColorType);
+  CheckForReadbackSupport(kBGRA_8888_SkColorType);
   // Further any formats, support should be checked here.
 }
 
 void GLHelperReadbackSupport::CheckForReadbackSupport(
     SkColorType texture_format) {
   bool supports_format = false;
   switch (texture_format) {
     case kRGB_565_SkColorType:
       supports_format = SupportsFormat(GL_RGB, GL_UNSIGNED_SHORT_5_6_5);
       break;
-    case kN32_SkColorType:
+    case kRGBA_8888_SkColorType:
       // This is the baseline, assume always true.
       supports_format = true;
       break;
+    case kBGRA_8888_SkColorType:
+      supports_format = SupportsFormat(GL_BGRA_EXT, GL_UNSIGNED_BYTE);
+      break;
     case kARGB_4444_SkColorType:
       supports_format = false;
       break;
     default:
       NOTREACHED();
       supports_format = false;
       break;
   }
   DCHECK((int)texture_format <= (int)kLastEnum_SkColorType);
   format_support_table_[texture_format] =
-      supports_format ? FORMAT_SUPPORTED : FORMAT_NOT_SUPPORTED;
+      supports_format ? kReadbackSupported : kReadbackNotSupported;
 }
 
 void GLHelperReadbackSupport::GetAdditionalFormat(GLint format, GLint type,
                                                   GLint *format_out,
                                                   GLint *type_out) {
   for (unsigned int i = 0; i < format_cache_.size(); i++) {
     if (format_cache_[i].format == format && format_cache_[i].type == type) {
       *format_out = format_cache_[i].read_format;
       *type_out = format_cache_[i].read_type;
       return;
@@ skipping 14 matching lines @@
                                                              dst_framebuffer);
   gl_->FramebufferTexture2D(
       GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, dst_texture, 0);
   gl_->GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT, format_out);
   gl_->GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE, type_out);
 
   struct FormatCacheEntry entry = { format, type, *format_out, *type_out };
   format_cache_.push_back(entry);
 }
 
 bool GLHelperReadbackSupport::SupportsFormat(GLint format, GLint type) {

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

I also needed to change this to GLenum too.

   // GLES2.0 Specification says this pairing is always supported
   // with additional format from GL_IMPLEMENTATION_COLOR_READ_FORMAT/TYPE
   if (format == GL_RGBA && type == GL_UNSIGNED_BYTE)
     return true;
+
+  if (format == GL_BGRA_EXT && type == GL_UNSIGNED_BYTE) {
+    const GLubyte* tmp = gl_->GetString(GL_EXTENSIONS);
+    std::string extensions =
+        " " + std::string(reinterpret_cast<const char*>(tmp)) + " ";
+    if (extensions.find(" GL_EXT_read_format_bgra ") != std::string::npos) {
+      return true;
+    }
+  }
+
   bool supports_format = false;
   GLint ext_format = 0, ext_type = 0;
   GetAdditionalFormat(format, type, &ext_format, &ext_type);
   if ((ext_format == format) && (ext_type == type)) {
     supports_format = true;
   }
   return supports_format;
 }
 
-bool GLHelperReadbackSupport::IsReadbackConfigSupported(
-    SkColorType texture_format) {
-  switch (format_support_table_[texture_format]) {
-    case FORMAT_SUPPORTED:
-      return true;
-    case FORMAT_NOT_SUPPORTED:
-      return false;
+FormatSupport GLHelperReadbackSupport::ReadbackConfig(
+    SkColorType color_type,
+    const bool& can_swizzle,
+    GLint& format,
+    GLint& type,
+    int32& bytes_per_pixel) {
+  bytes_per_pixel = 4;
+  type = GL_UNSIGNED_BYTE;
+  GLint new_format = 0, new_type = 0;

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

nit: move these into the scope where they are needed (l.139)

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

Done.

+  switch (color_type) {
+    case kRGB_565_SkColorType:
+      if (format_support_table_[color_type] == kReadbackSupported) {
+        format = GL_RGB;
+        type = GL_UNSIGNED_SHORT_5_6_5;
+        bytes_per_pixel = 2;
+        return kReadbackSupported;
+      }
+      break;
+    case kRGBA_8888_SkColorType:
+      format = GL_RGBA;
+      if (can_swizzle) {
+        // Handle preference for readback in GL_BGRA_EXT
+        GetAdditionalFormat(format, type, &new_format, &new_type);

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

Why do we need this path, given that we can always readback as RGBA/UB which
matches kRGBA_8888_SkColorType?

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

This is to handle the case (Intel) where the preferred format for readback is
BGRA and if swizzling is possible then pre-swizzle. (This is the generalisation
of the code that was previously in the YUV codepath only.)

[piman, 2014/07/23 18:07:38]

Ok, can you add a comment to that effect, that we take the presence of an
additional format as a hint that we prefer it for performance?

[no sievers, 2014/08/21 19:41:08]

Just wondering.... why do you assume that the additional format is faster? The
GLES2 spec seems to request that one other format is supported. But what if it's
actually slower?
On Android we use RGBA as the native skia byte ordering, and I assume the
framebuffer internal byte order is usually also RGBA. Then reading back BGRA
would always force the driver to swizzle somehow. Wouldn't that be more likely
to be slower?

[piman, 2014/08/21 20:00:02]

Well, the spec doesn't say anything, but the intent of the
GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal native
format for the buffer, or at least the faster one. If the buffer is actually
GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* should
return.

On Intel and ANGLE, the native format is GL_BGRA_EXT/GL_UNSIGNED_BYTE, and
there's no fast path to readback + swizzle in the driver.

If we find that there are drivers that return GL_BGRA_EXT/GL_UNSIGNED_BYTE for a
RGBA buffer, then we'll need an extra piece of data / workaround entry to
indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* format to
be slower.

+
+        if (new_format == GL_BGRA_EXT && new_type == GL_UNSIGNED_BYTE) {
+          format = GL_BGRA_EXT;
+          return kReadbackSwizzle;
+        }
+      }
+      return kReadbackSupported;
+    case kBGRA_8888_SkColorType:
+      format = GL_BGRA_EXT;
+      if (format_support_table_[color_type] == kReadbackSupported)
+        return kReadbackSupported;
+
+      if (can_swizzle) {
+        format = GL_RGBA;
+        return kReadbackSwizzle;
+      }
+
+      break;
+    case kARGB_4444_SkColorType:
+      return kReadbackNotSupported;
     default:
       NOTREACHED();
-      return false;
+      break;
   }
+
+  return kReadbackNotSupported;
 }
 
 }  // namespace content