GLHelper: Address inconsistent mapping of SkColorType to GL format

content/common/gpu/client/gl_helper_readback_support.cc

Index: content/common/gpu/client/gl_helper_readback_support.cc
diff --git a/content/common/gpu/client/gl_helper_readback_support.cc b/content/common/gpu/client/gl_helper_readback_support.cc
index fc47488408b9fb0c0c10e4e684bddef6807317cd..cb385e65b6227b04a81c45ff4c3ba29530932d0f 100644
--- a/content/common/gpu/client/gl_helper_readback_support.cc
+++ b/content/common/gpu/client/gl_helper_readback_support.cc
@@ -4,6 +4,8 @@
 
 #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 {
 
@@ -18,14 +20,15 @@ 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.
 }
 
@@ -36,10 +39,13 @@ void GLHelperReadbackSupport::CheckForReadbackSupport(
     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;
@@ -50,7 +56,7 @@ void GLHelperReadbackSupport::CheckForReadbackSupport(
   }
   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,
@@ -90,6 +96,16 @@ bool GLHelperReadbackSupport::SupportsFormat(GLint format, GLint type) {
   // 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);
@@ -99,17 +115,55 @@ bool GLHelperReadbackSupport::SupportsFormat(GLint format, GLint type) {
   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