Implement GLHelperReadbackSupport for GLHelper usage.

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 5a36836a54d892d2a9f6c3b242f588f7a5c452cd..b3e578fcfb3c7c311d7a989c510bd946a38979d5 100644
--- a/content/common/gpu/client/gl_helper.cc
+++ b/content/common/gpu/client/gl_helper.cc
@@ -4,6 +4,7 @@
 
 #include "content/common/gpu/client/gl_helper.h"
 
+#include <algorithm>
 #include <queue>
 #include <string>
 
@@ -178,6 +179,8 @@ class GLHelper::CopyTextureToImpl
   // 0 if GL_EXT_draw_buffers is not available.
   GLint MaxDrawBuffers() const { return max_draw_buffers_; }
 
+  bool IsReadBackConfigSupported(SkBitmap::Config bitmap_config);
+
  private:
   // A single request to CropScaleReadbackAndCleanTexture.
   // The main thread can cancel the request, before it's handled by the helper
@@ -341,15 +344,12 @@ GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
     const gfx::Size& dst_size,
     bool vertically_flip_texture,
     bool swizzle,
-    SkBitmap::Config config,
+    SkBitmap::Config bitmap_config,
     GLHelper::ScalerQuality quality) {
 
-  bool format_support = ((config == SkBitmap::kRGB_565_Config) ||
-                         (config == SkBitmap::kARGB_8888_Config));
-  if (!format_support) {
-    DCHECK(format_support);
+  if (!IsReadBackConfigSupported(bitmap_config))
     return 0;
-  }
+
   scoped_ptr<ScalerInterface> scaler(
       helper_->CreateScaler(quality,
                             src_size,
@@ -364,7 +364,7 @@ GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
   gl_->GenTextures(1, &dst_texture);
   {
     ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, dst_texture);
-    switch (config) {
+    switch (bitmap_config) {
       case SkBitmap::kARGB_8888_Config:
         // Do nothing params already set.
         break;
@@ -395,12 +395,10 @@ void GLHelper::CopyTextureToImpl::ReadbackAsync(
     int32 bytes_per_row,
     int32 row_stride_bytes,
     unsigned char* out,
-    const SkBitmap::Config config,
+    const SkBitmap::Config bitmap_config,
     const base::Callback<void(bool)>& callback) {
-  bool format_support = ((config == SkBitmap::kRGB_565_Config) ||
-                         (config == SkBitmap::kARGB_8888_Config));
-  if (!format_support) {
-    DCHECK(format_support);
+
+   if (!IsReadBackConfigSupported(bitmap_config)) {

[no sievers, 2014/02/18 19:35:44]

See comment in ReadbackTextureAsync().

[sivag, 2014/02/19 15:49:04]

Replied at ReadbackTextureAsync.

     callback.Run(false);
     return;
   }
@@ -413,7 +411,7 @@ void GLHelper::CopyTextureToImpl::ReadbackAsync(
   GLenum format = GL_RGBA, type = GL_UNSIGNED_BYTE;
   int bytes_per_pixel = 4;
 
-  switch (config) {
+  switch (bitmap_config) {
     case SkBitmap::kARGB_8888_Config:
       // Do nothing params already set.
       break;
@@ -459,10 +457,8 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     const SkBitmap::Config bitmap_config,
     const base::Callback<void(bool)>& callback,
     GLHelper::ScalerQuality quality) {
-  bool format_support = ((bitmap_config == SkBitmap::kRGB_565_Config) ||
-                         (bitmap_config == SkBitmap::kARGB_8888_Config));
-  if (!format_support) {
-    DCHECK(format_support);
+
+  if (!IsReadBackConfigSupported(bitmap_config)) {
     callback.Run(false);
     return;
   }
@@ -540,15 +536,12 @@ void GLHelper::CopyTextureToImpl::ReadbackTextureAsync(
     GLuint texture,
     const gfx::Size& dst_size,
     unsigned char* out,
-    SkBitmap::Config config,
+    SkBitmap::Config bitmap_config,
     const base::Callback<void(bool)>& callback) {
-  // Only ARGB888 and RGB565 supported as of now.
-  bool format_support = ((config == SkBitmap::kRGB_565_Config) ||
-                         (config == SkBitmap::kARGB_8888_Config));
-  if (!format_support) {
-    DCHECK(format_support);
+
+  if (!IsReadBackConfigSupported(bitmap_config))

[no sievers, 2014/02/18 19:35:44]

This check is actually not sufficient.
IsReadBackConfigSupported() tells you whether we can read back a 565 texture
1:1. However, what if |texture| here is RGBA8?

Maybe we should just remove the checks in ReadbackTexture(A)Sync, and leave it
to the client to make sure readbacks are not requested with implied format
conversion?

For the internal call sites where we create an intermediate texture, we already
have the checks and create the texture in the destination format.

[sivag, 2014/02/19 15:49:04]

I think then we should actually validate the texture also using
GetTexParameteriv for the GL_TEXTURE_INTERNAL_FORMAT , and match that with the
SkBitmap::Config passed.

Or 

we should remove the config parameter itself , in both the Readbacktexturesync
and async and validate just by using the texture by getting the interformat
details using GetTexParameteriv.

[no sievers, 2014/02/19 19:09:38]

Sounds good. Feel free to try as a follow-up since there's no need to change it
functionally in this patch. But you want to do it as a DCHECK() only since it's
synchronous. Also, is that check with GL_TEXTURE_INTERNAL_FORMAT even supported
in OpenGL ES?

[sivag, 2014/02/21 11:40:50]

When i checked , there is no such support in the api list.
I will confirm this.

     return;
-  }
+
   ScopedFramebuffer dst_framebuffer(gl_);
   ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(gl_,
                                                              dst_framebuffer);
@@ -558,12 +551,12 @@ void GLHelper::CopyTextureToImpl::ReadbackTextureAsync(
                             GL_TEXTURE_2D,
                             texture,
                             0);
-  int bytes_per_pixel = (config == SkBitmap::kRGB_565_Config) ? 2 : 4;
+  int bytes_per_pixel = (bitmap_config == SkBitmap::kRGB_565_Config) ? 2 : 4;
   ReadbackAsync(dst_size,
                 dst_size.width() * bytes_per_pixel,
                 dst_size.width() * bytes_per_pixel,
                 out,
-                config,
+                bitmap_config,
                 callback);
 }
 
@@ -583,6 +576,15 @@ GLuint GLHelper::CopyTextureToImpl::CopyAndScaleTexture(
                       quality);
 }
 
+bool GLHelper::CopyTextureToImpl::IsReadBackConfigSupported(
+    SkBitmap::Config bitmap_config) {
+  if(!helper_){

[piman, 2014/02/18 22:23:15]

nit: spaces

Try git cl format.

[sivag, 2014/02/19 15:49:04]

Done.

+    NOTREACHED();
+    return false;
+  }
+  return helper_->IsReadBackConfigSupported(bitmap_config);
+}
+
 void GLHelper::CopyTextureToImpl::ReadbackDone(Request* finished_request,
                                                int bytes_per_pixel) {
   TRACE_EVENT0("mirror",
@@ -650,9 +652,7 @@ void GLHelper::CopyTextureToImpl::CancelRequests() {
 
 GLHelper::GLHelper(GLES2Interface* gl, gpu::ContextSupport* context_support)
     : gl_(gl),
-      context_support_(context_support),
-      initialized_565_format_check_(false),
-      support_565_format_(false) {}
+      context_support_(context_support) {}
 
 GLHelper::~GLHelper() {}
 
@@ -875,12 +875,10 @@ void GLHelper::CopyTextureFullImage(GLuint texture, const gfx::Size& size) {
       GL_TEXTURE_2D, 0, GL_RGB, 0, 0, size.width(), size.height(), 0);
 }
 
-bool GLHelper::CanUseRgb565Readback() {
-  if(initialized_565_format_check_){
-    return support_565_format_;
-  }
+bool GLHelper::SupportsFormat(GLint format, GLint type) {
   const int kTestSize = 64;
   GLuint dst_texture = 0u;
+  bool supports_format = false;
   gl_->GenTextures(1, &dst_texture);
   ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, dst_texture);
   gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
@@ -889,12 +887,12 @@ bool GLHelper::CanUseRgb565Readback() {
   gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
   gl_->TexImage2D(GL_TEXTURE_2D,
                   0,
-                  GL_RGB,
+                  format,
                   kTestSize,
                   kTestSize,
                   0,
-                  GL_RGB,
-                  GL_UNSIGNED_SHORT_5_6_5,
+                  format,
+                  type,
                   NULL);
   ScopedFramebuffer dst_framebuffer(gl_);
   ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(gl_,
@@ -908,11 +906,34 @@ bool GLHelper::CanUseRgb565Readback() {
   gl_->GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT, &ext_format);
   gl_->GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE, &ext_type);
   gl_->DeleteTextures(1, &dst_texture);
-  if ((ext_format == GL_RGB) && (ext_type == GL_UNSIGNED_SHORT_5_6_5)) {
-    support_565_format_ = true;
+  if ((ext_format == format) && (ext_type == type)) {
+    supports_format = true;
+  }
+  return supports_format;
+}
+
+bool GLHelper::IsReadBackConfigSupported(
+  const SkBitmap::Config& texture_format) {

[no sievers, 2014/02/18 19:35:44]

nit: bitmap_format

[sivag, 2014/02/19 15:49:04]

Done.

+
+  std::map<SkBitmap::Config, bool>::iterator find_format =
+                                          config_map_.find(texture_format);
+  if (find_format != config_map_.end())
+    return find_format->second;
+
+  switch (texture_format) {
+    case SkBitmap::kRGB_565_Config:
+      return config_map_[texture_format] = SupportsFormat(GL_RGB,
+                                                     GL_UNSIGNED_SHORT_5_6_5);

[no sievers, 2014/02/18 19:35:44]

nit: indent

[sivag, 2014/02/19 15:49:04]

Done.

+
+    case SkBitmap::kARGB_8888_Config:
+      return config_map_[texture_format] = SupportsFormat(GL_RGBA,
+                                                         GL_UNSIGNED_BYTE);
+    case SkBitmap::kA8_Config:
+    case SkBitmap::kARGB_4444_Config:
+    default:
+      NOTREACHED();
+      return config_map_[texture_format] = false;
   }
-  initialized_565_format_check_ = true;
-  return support_565_format_;
 }
 
 void GLHelper::CopyTextureToImpl::ReadbackPlane(