wip: Improving GestureNav screenshotting performance

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 c61737bce38d9d8bd727adce27d32bcf958b1ef9..392a15e89dcf0751f588d95791a21685ffc5312c 100644
--- a/content/common/gpu/client/gl_helper.cc
+++ b/content/common/gpu/client/gl_helper.cc
@@ -319,6 +319,20 @@ class GLHelper::CopyTextureToImpl
                       SkBitmap::Config config,
                       GLHelper::ScalerQuality quality);
 
+  // Converts each four consecutive pixels of the source texture into one pixel
+  // in the result texture with each pixel channel representing the grayscale
+  // color of one of the four original pixels:
+  // R1B1G1A1 R2B2G2A2 R3B3G3A3 R4G4B4A4 -> X1X2X3X4
+  // The resulting texture is an RGBA texture which is 4 times narrower than the
+  // original, but when read back the pixel data can be used to construct the

[danakj, 2014/07/10 19:13:02]

just drop the reference to bitmaps here, this function itself has nothing to do
with bitmaps

[mfomitchev, 2014/07/11 19:46:08]

Done. See if you like the new version.

+  // kA8_Config bitmap representing the grayscale version of the original
+  // texture.
+  GLuint TransformTextureToA8(GLuint src_texture,

[danakj, 2014/07/10 19:13:03]

TransformTextureToGrayscale. There's no A8 bitmaps here.

[mfomitchev, 2014/07/11 19:46:07]

Done.

+                              const gfx::Size& src_size,
+                              const gfx::Rect& src_subrect,
+                              gfx::Size* const result_size,
+                              bool swizzle);
+
   static void nullcallback(bool success) {}
   void ReadbackDone(Request *request, int bytes_per_pixel);
   void FinishRequest(Request* request, bool result);
@@ -327,6 +341,7 @@ class GLHelper::CopyTextureToImpl
   static const float kRGBtoYColorWeights[];
   static const float kRGBtoUColorWeights[];
   static const float kRGBtoVColorWeights[];
+  static const float kRGBtoA8ColorWeights[];

[danakj, 2014/07/10 19:13:03]

toGrayscale

[mfomitchev, 2014/07/11 19:46:07]

Done.

 
   GLES2Interface* gl_;
   gpu::ContextSupport* context_support_;
@@ -407,6 +422,45 @@ GLuint GLHelper::CopyTextureToImpl::ScaleTexture(
   return dst_texture;
 }
 
+GLuint GLHelper::CopyTextureToImpl::TransformTextureToA8(
+    GLuint src_texture,
+    const gfx::Size& src_size,
+    const gfx::Rect& src_subrect,
+    gfx::Size* const result_size,
+    bool swizzle) {
+
+  LOG(ERROR) << "GLHelper::CopyTextureToImpl::TransformTextureToA8";
+
+  GLuint dst_texture = 1u;
+  gl_->GenTextures(1, &dst_texture);
+  {
+    ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(gl_, dst_texture);
+    gl_->TexImage2D(GL_TEXTURE_2D,
+                    0,
+                    GL_BGRA_EXT,
+                    src_size.width(),
+                    src_size.height(),
+                    0,
+                    GL_BGRA_EXT,
+                    GL_UNSIGNED_BYTE,
+                    NULL);
+  }
+
+  // The size of the transformed texture size
+  *result_size = gfx::Size((src_size.width() + 3) / 4, src_size.height());

[danakj, 2014/07/10 19:13:02]

hm, doing this here feels awkward, cuz there's some channels in the last pixel
that are not valid. if we can move this later (inside ReadbackAsync maybe?) that
might be better?

[mfomitchev, 2014/07/11 19:46:07]

Got rid of result_size.

+  helper_->InitScalerImpl();
+  scoped_ptr<ScalerInterface>
+  a8_scaler(helper_->scaler_impl_.get()->CreatePlanarScaler(
+      src_size,
+      gfx::Rect(0, 0, (src_size.width() + 3) & ~3, src_size.height()),
+      *result_size,
+      false,
+      (swizzle == kSwizzleBGRA),
+      kRGBtoA8ColorWeights));
+  a8_scaler->Scale(src_texture, dst_texture);
+  return dst_texture;
+}
+
 void GLHelper::CopyTextureToImpl::ReadbackAsync(
     const gfx::Size& dst_size,
     int32 bytes_per_row,
@@ -475,23 +529,45 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     const SkBitmap::Config bitmap_config,
     const base::Callback<void(bool)>& callback,
     GLHelper::ScalerQuality quality) {
-  if (!IsReadbackConfigSupported(bitmap_config)) {
+  if (bitmap_config != SkBitmap::kA8_Config &&

[danakj, 2014/07/10 19:13:02]

why is this here instead of deciding A8 is ok inside IsReadbackConfigSupported?

[mfomitchev, 2014/07/11 19:46:07]

Responded separately in the reply.

+      !IsReadbackConfigSupported(bitmap_config)) {
     callback.Run(false);
     return;
   }
+
+  SkBitmap::Config texture_config = bitmap_config;
+  if (bitmap_config == SkBitmap::kA8_Config) {
+    // For readback purposes, treat the resulting texture as ARGB of smaller

[danakj, 2014/07/10 19:13:02]

What does it look like if you teach ReadbackAsync to deal with A8 type?

[mfomitchev, 2014/07/11 19:46:07]

Done. If the new version looks ok, I will make similar changes in
ReadbackTextureSync

+    // size.
+    texture_config = SkBitmap::kARGB_8888_Config;
+  }
+
+  bool swizzle =
+#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT
+                                true;
+#else
+                                false;
+#endif
+
   GLuint texture = ScaleTexture(src_texture,
                                 src_size,
                                 src_subrect,
                                 dst_size,
                                 true,
-#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT
-                                true,
-#else
-                                false,
-#endif
-                                bitmap_config,
+                                swizzle,
+                                texture_config,
                                 quality);
-  DCHECK(texture);
+
+  gfx::Size texture_size = dst_size;
+  if (bitmap_config == SkBitmap::kA8_Config) {
+    texture = TransformTextureToA8(texture,
+                                  src_size,

[danakj, 2014/07/10 19:13:03]

git cl format

[mfomitchev, 2014/07/11 19:46:08]

Done.

+                                  src_subrect,
+                                  &texture_size,
+                                  swizzle);
+
+    DCHECK(texture);
+  }
   ScopedFramebuffer dst_framebuffer(gl_);
   ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(gl_,
                                                              dst_framebuffer);
@@ -502,6 +578,7 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
                             texture,
                             0);
   int bytes_per_pixel = 4;
+
   switch (bitmap_config) {
     case SkBitmap::kARGB_8888_Config:
       // Do nothing params already set.
@@ -509,16 +586,20 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     case SkBitmap::kRGB_565_Config:
       bytes_per_pixel = 2;
       break;
+    case SkBitmap::kA8_Config:
+      bytes_per_pixel = 1;
+      break;
     default:
       NOTREACHED();
       break;
   }
-  ReadbackAsync(dst_size,
+
+  ReadbackAsync(texture_size,
                 dst_size.width() * bytes_per_pixel,
                 dst_size.width() * bytes_per_pixel,
                 out,
-                bitmap_config,
-                kSwizzleNone,
+                texture_config,
+                kSwizzleBGRA,

[mfomitchev, 2014/07/09 18:02:10]

The config and swizzle for the A8 case are picked such that we end up using
GL_BGRA_EXT format in ReadbackAsync(). Is this too confusing/hacky or is it
okay?

[danakj, 2014/07/10 19:13:03]

This None->BGRA will affect more than the A8 path. This seems like a separate CL
first with tests to show it's the right thing to do.

Also is BGRA always the right thing to do on all hardware?

[mfomitchev, 2014/07/11 19:46:07]

Good point, changed it back. I will separate this into a separate CL and add
tests before committing.
It sounded like that from @hubbe's comments... Should I add one of the owners of
gl_helper to the review?

                 callback);
   gl_->DeleteTextures(1, &texture);
 }
@@ -625,6 +706,7 @@ void GLHelper::CopyTextureToImpl::ReadbackDone(Request* finished_request,
           GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, GL_READ_ONLY));
       if (data) {
         result = true;
+
         if (request->bytes_per_row == request->size.width() * bytes_per_pixel &&
             request->bytes_per_row == request->row_stride_bytes) {
           memcpy(request->pixels, data,
@@ -928,6 +1010,8 @@ 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};
+const float GLHelper::CopyTextureToImpl::kRGBtoA8ColorWeights[] = {
+    0.213f, 0.715f, 0.072f, 0.0f};
 
 // YUV readback constructors. Initiates the main scaler pipeline and
 // one planar scaler for each of the Y, U and V planes.