wip: Improving GestureNav screenshotting performance

content/browser/compositor/delegated_frame_host.cc

Index: content/browser/compositor/delegated_frame_host.cc
diff --git a/content/browser/compositor/delegated_frame_host.cc b/content/browser/compositor/delegated_frame_host.cc
index 5db69b70b4c938fb5720c32ba69d9658eddf2757..f1ab500008da5fd355e60b251886da4cd2f99b0d 100644
--- a/content/browser/compositor/delegated_frame_host.cc
+++ b/content/browser/compositor/delegated_frame_host.cc
@@ -18,6 +18,10 @@
 #include "media/base/video_frame.h"
 #include "media/base/video_util.h"
 #include "skia/ext/image_operations.h"
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkPaint.h"
+#include "third_party/skia/include/effects/SkLumaColorFilter.h"
+
 
 namespace content {
 
@@ -122,7 +126,8 @@ void DelegatedFrameHost::CopyFromCompositingSurface(
     const base::Callback<void(bool, const SkBitmap&)>& callback,
     const SkBitmap::Config config) {
   // Only ARGB888 and RGB565 supported as of now.
-  bool format_support = ((config == SkBitmap::kRGB_565_Config) ||
+  bool format_support = ((config == SkBitmap::kA8_Config) ||
+                         (config == SkBitmap::kRGB_565_Config) ||
                          (config == SkBitmap::kARGB_8888_Config));
   DCHECK(format_support);
   if (!CanCopyToBitmap()) {
@@ -438,6 +443,7 @@ void DelegatedFrameHost::CopyFromCompositingSurfaceHasResult(
   }
 
   if (result->HasTexture()) {
+    LOG(ERROR) << "result->HasTexture()";
     PrepareTextureCopyOutputResult(dst_size_in_pixel, config,
                                    callback,
                                    result.Pass());
@@ -445,6 +451,7 @@ void DelegatedFrameHost::CopyFromCompositingSurfaceHasResult(
   }
 
   DCHECK(result->HasBitmap());
+  LOG(ERROR) << "result->HasBitmap()";
   PrepareBitmapCopyOutputResult(dst_size_in_pixel, config, callback,
                                 result.Pass());
 }
@@ -455,6 +462,7 @@ static void CopyFromCompositingSurfaceFinished(
     scoped_ptr<SkBitmap> bitmap,
     scoped_ptr<SkAutoLockPixels> bitmap_pixels_lock,
     bool result) {
+  LOG(ERROR) << "CopyFromCompositingSurfaceFinished";
   bitmap_pixels_lock.reset();
 
   uint32 sync_point = 0;
@@ -466,6 +474,7 @@ static void CopyFromCompositingSurfaceFinished(
   release_callback->Run(sync_point, lost_resource);
 
   callback.Run(result, *bitmap);
+  LOG(ERROR) << "=== CopyFromCompositingSurfaceFinished EXITING";
 }
 
 // static
@@ -474,6 +483,9 @@ void DelegatedFrameHost::PrepareTextureCopyOutputResult(
     const SkBitmap::Config config,
     const base::Callback<void(bool, const SkBitmap&)>& callback,
     scoped_ptr<cc::CopyOutputResult> result) {
+  LOG(ERROR) << "DelegatedFrameHost::PrepareTextureCopyOutputResult"
+      << ", bitmap config=" <<  config;
+
   DCHECK(result->HasTexture());
   base::ScopedClosureRunner scoped_callback_runner(
       base::Bind(callback, false, SkBitmap()));
@@ -523,7 +535,7 @@ void DelegatedFrameHost::PrepareBitmapCopyOutputResult(
     const SkBitmap::Config config,
     const base::Callback<void(bool, const SkBitmap&)>& callback,
     scoped_ptr<cc::CopyOutputResult> result) {
-  if (config != SkBitmap::kARGB_8888_Config) {
+  if (config != SkBitmap::kARGB_8888_Config && config != SkBitmap::kA8_Config) {

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

Any tips for testing this code path? How do I make RequestCopyOfOutput return a
bitmap instead of a texture?

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

Software compositing.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

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

It uses --disable-gpu, but when I try to launch chrome binary with this flag it
crashes on startup:
[6767:6767:0711/150707:FATAL:gpu_process_transport_factory.cc(202)] Failed to
create UI context, but can't use software compositing with browser threaded
compositing. Aborting.

I tried adding --disable-threaded-compositing, but it still crashes with the
same error.

Tried using --disable-gpu-compositing instead, and it launches,  but looks
pretty messed up.

[mfomitchev, 2014/07/11 20:47:50]

Ah, looks like I can call force_bitmap_result() on the request. I'll try that.

[danakj, 2014/07/11 20:52:16]

I'm planning to remove that. You want --ui-disable-threaded-compositing.

What you really want is to call UseSoftwareCompositing() in your test which sets
both these flags.

[mfomitchev, 2014/07/11 21:06:51]

Ok, thanks. 

Does this path actually have value? Looking at the GPU feature dashboard, we use
GPU accelerated compositing in all configurations now. So will this code ever be
executed outside of tests?

[danakj, 2014/07/11 21:15:03]

We don't use software compositing on chromeos. We do use it on every other
platform when gpu is blacklisted, when gpu process is unstable, or when users
turn it off in their prefs.

[mfomitchev, 2014/07/14 14:30:28]

Got it, thanks. On a related note, do you think it's still worth creating a
separate issue for Layer::RequestCopyOfOutput()
(https://code.google.com/p/chromium/issues/detail?id=379983#c26)?

The only thing not covered by this CL is optimizing the software path. We'd also
have to add a way to specify the bitmap color format to CopyOutputRequest. I am
not sure if it's worth it.

[danakj, 2014/07/14 14:51:06]

I was thinking about removing it actually, since the only thing that needs it is
tests and they can use GLHelper instead.

[mfomitchev, 2014/07/14 15:00:40]

Are you talking about force_bitmap_result()? Specifying the color format might
still help performance wise (for the case where it falls back on the software
path and does return a bitmap), I am just not sure it's worth it. So no new
issue for Layer::RequestCopyOfOutput()?

[danakj, 2014/07/14 15:01:52]

Yep I'm talking about that. The bitmap output would only happen for software
compositing. The texture output would only happen for GL compositing.

[mfomitchev, 2014/07/14 15:06:33]

Right, and as you said software compositing may be used as a fallback when gpu
is blacklisted, etc...

I was just looking at the discussion we had about creating a new issue for
Layer::RequestCopyOfOutput() (
https://code.google.com/p/chromium/issues/detail?id=379983#c26 ). This fix
covers the GPU path, so the only thing left is the software path.

     NOTIMPLEMENTED();
     callback.Run(false, SkBitmap());
     return;
@@ -531,12 +543,34 @@ void DelegatedFrameHost::PrepareBitmapCopyOutputResult(
   DCHECK(result->HasBitmap());
   scoped_ptr<SkBitmap> source = result->TakeBitmap();
   DCHECK(source);
-  SkBitmap bitmap = skia::ImageOperations::Resize(
-      *source,
-      skia::ImageOperations::RESIZE_BEST,
-      dst_size_in_pixel.width(),
-      dst_size_in_pixel.height());
-  callback.Run(true, bitmap);
+  SkBitmap scaledBitmap;

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

scaled_bitmap

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

Done.

+  if (source->width() != dst_size_in_pixel.width() ||
+      source->height() != dst_size_in_pixel.height()) {

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

if they are == you want to return the original bitmap instead of an empty one?

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

Oops. Done.

+    scaledBitmap = skia::ImageOperations::Resize(
+        *source,
+        skia::ImageOperations::RESIZE_BEST,
+        dst_size_in_pixel.width(),
+        dst_size_in_pixel.height());
+  }
+  if (config == SkBitmap::kARGB_8888_Config) {
+    callback.Run(true, scaledBitmap);
+    return;
+  }
+  // Else - |config| is kA8_Config.

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

how about DCHECK_EQ(config, kA8_Config) instead of a comment.

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

Done.

+  // Paint |scaledBitmap| to alpha-only |a8Bitmap|.
+  SkBitmap a8Bitmap;

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

grayscale_bitmap

+  a8Bitmap.setConfig(SkBitmap::kA8_Config,

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

setConfig is deprecated. Use an SkImageInfo that you pass to allocPixels

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

Done.

+                     scaledBitmap.width(),
+                     scaledBitmap.height(),
+                     0);
+  a8Bitmap.allocPixels();
+  SkCanvas canvas(a8Bitmap);
+  SkPaint paint;
+  SkColorFilter* filter = SkLumaColorFilter::Create();
+  paint.setColorFilter(filter);
+  filter->unref();

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

no manual refcounting. use skia::RefPtr instead

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

Neat! Done.

+  canvas.drawBitmap(scaledBitmap, SkIntToScalar(0), SkIntToScalar(0), &paint);
+  callback.Run(true, a8Bitmap);
 }
 
 // static
@@ -555,6 +589,7 @@ void DelegatedFrameHost::ReturnSubscriberTexture(
     dfh->idle_frame_subscriber_textures_.push_back(subscriber_texture);
 }
 
+// static
 void DelegatedFrameHost::CopyFromCompositingSurfaceFinishedForVideo(
     base::WeakPtr<DelegatedFrameHost> dfh,
     const base::Callback<void(bool)>& callback,