Make ui::Snapshot asynchronous on Android, remove CompositeAndReadback.

Make ui::Snapshot asynchronous on Android, remove CompositeAndReadback.

The android pixel tests use the ui::Snapshot code to get a pixel copy
of the content_shell's output. Make this behaviour match the aura
implementation of ui::Snapshot by returning false in the synchronous
verion, causing callsites to fall back to the async one.

This implements the async snapshots by moving the aura implementation
out into snapshot_async.cc and reusing that code (with one #if for
RGBA vs BGRA ordering).

After this, the pixel tests get a bitmap from cc instead of creating
their own bitmap. The result is that the bitmap has a slightly
different size (384x528) which is the size of the root layer. The
pixel tests were previously trying to crop to 400x300, which is
outside the bounds of this bitmap, so change them to crop to 300x300
instead, and increment their versions to pick up the new pngs.

BUG=252046, 371592
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270778

[danakj, 2014-05-13 23:07:18]

Thanks everyone for your help on this.

sievers: please review
tedchoc: please review ui/snapshot
kbr: please review content/test/gpu/page_sets/pixel_tests.py
sky: ui/snapshot OWNERS

[danakj, 2014-05-13 23:11:07]

https://codereview.chromium.org/281003002/diff/1/content/browser/android/cont...
File content/browser/android/content_view_render_view.h (right):

https://codereview.chromium.org/281003002/diff/1/content/browser/android/cont...
content/browser/android/content_view_render_view.h:16: class CopyOutputResult;
oops some leftovers in here... *^^*

[danakj, 2014-05-13 23:14:04]

https://codereview.chromium.org/281003002/diff/1/content/browser/android/cont...
File content/browser/android/content_view_render_view.h (right):

https://codereview.chromium.org/281003002/diff/1/content/browser/android/cont...
content/browser/android/content_view_render_view.h:16: class CopyOutputResult;
On 2014/05/13 23:11:09, danakj wrote:
> oops some leftovers in here... *^^*

Fixed.

[Ken Russell (switch to Gerrit), 2014-05-13 23:14:44]

Thanks for fixing this.

LGTM overall as long as this has been tested locally. One comment.

https://codereview.chromium.org/281003002/diff/1/content/test/gpu/page_sets/p...
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/281003002/diff/1/content/test/gpu/page_sets/p...
content/test/gpu/page_sets/pixel_tests.py:52: revision=7,
The GPU try servers aren't allowed to generate new reference images, so all of
the GPU try jobs (including the linux_gpu_triggered_tests job which is now part
of the CQ) will fail. Along with the increment of these version numbers, please
modify src/content/test/gpu/gpu_tests/pixel_expectations.py in this CL and mark
all of the tests as failing, pointing to this bug. Once this CL lands, then
please submit another one which removes the failure expectations. Thanks.

[danakj, 2014-05-13 23:18:06]

Yep I have tested this on my N4 and I get nice pngs back :)

https://codereview.chromium.org/281003002/diff/1/content/test/gpu/page_sets/p...
File content/test/gpu/page_sets/pixel_tests.py (right):

https://codereview.chromium.org/281003002/diff/1/content/test/gpu/page_sets/p...
content/test/gpu/page_sets/pixel_tests.py:52: revision=7,
On 2014/05/13 23:14:45, Ken Russell wrote:
> The GPU try servers aren't allowed to generate new reference images, so all of
> the GPU try jobs (including the linux_gpu_triggered_tests job which is now
part
> of the CQ) will fail. Along with the increment of these version numbers,
please
> modify src/content/test/gpu/gpu_tests/pixel_expectations.py in this CL and
mark
> all of the tests as failing, pointing to this bug. Once this CL lands, then
> please submit another one which removes the failure expectations. Thanks.

ok done, can you please verify i did it correctly?

[Ken Russell (switch to Gerrit), 2014-05-13 23:21:21]

https://codereview.chromium.org/281003002/diff/40001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/281003002/diff/40001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/pixel_expectations.py:28: ['android'], bug=371592)
The temporary failure expectations are needed on all platforms. Per entries in
webgl_conformance_expectations.py, just remove the ['android'], so this should
read

  self.Fail('Pixel.Canvas2DRedBox', bug=371592)

[danakj, 2014-05-13 23:22:45]

https://codereview.chromium.org/281003002/diff/40001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/pixel_expectations.py (right):

https://codereview.chromium.org/281003002/diff/40001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/pixel_expectations.py:28: ['android'], bug=371592)
On 2014/05/13 23:21:21, Ken Russell wrote:
> The temporary failure expectations are needed on all platforms. Per entries in
> webgl_conformance_expectations.py, just remove the ['android'], so this should
> read
> 
>   self.Fail('Pixel.Canvas2DRedBox', bug=371592)

Ahh right. Thanks! Done.

[Ken Russell (switch to Gerrit), 2014-05-13 23:28:27]

LGTM again.

[sky, 2014-05-13 23:43:05]

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
File ui/snapshot/snapshot_async.cc (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.cc:40: unsigned char* pixels =
Don't you need to lockPixels before accessing them?

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.cc:41: reinterpret_cast<unsigned
char*>(bitmap.pixelRef()->pixels());
Might pixelRef return NULL?

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
File ui/snapshot/snapshot_async.h (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.h:37: SnapshotAsync();  // Not instantiable.
SnapshotAsync->DISALLOW_IMPLICIT_CONSTRUCTORS

[no sievers, 2014-05-13 23:57:53]

lgtm for android

[no sievers, 2014-05-13 23:58:00]

https://codereview.chromium.org/281003002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/281003002/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:625: if (root_layer_)
nit: The root_layer_ is always there

[Ted C, 2014-05-14 01:13:18]

lgtm (addressing sky's feedback)

https://codereview.chromium.org/281003002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/281003002/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:625: if (root_layer_)
On 2014/05/13 23:58:00, sievers wrote:
> nit: The root_layer_ is always there

And if we don't remove this conditional, do we need to handle the false case by
signaling the request failed?

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_and...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_and...
ui/snapshot/snapshot_android.cc:9: #include
"cc/resources/single_release_callback.h"
Do we need this include or the following one?

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
File ui/snapshot/snapshot_async.cc (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.cc:1: // Copyright (c) 2014 The Chromium Authors. All
rights reserved.
same here

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
File ui/snapshot/snapshot_async.h (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.h:1: // Copyright (c) 2014 The Chromium Authors. All
rights reserved.
don't need the (c) anymore

[danakj, 2014-05-14 14:33:20]

sky: PTAL

Thanks for the reviews! I had to add cc to the ui/base/DEPS since it should
really be including copy_output_request.h from the window_android_compositor.h

https://codereview.chromium.org/281003002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/281003002/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:625: if (root_layer_)
On 2014/05/14 01:13:18, Ted C wrote:
> On 2014/05/13 23:58:00, sievers wrote:
> > nit: The root_layer_ is always there

Oh good, done.

> And if we don't remove this conditional, do we need to handle the false case
by
> signaling the request failed?

No, when the CopyOutputRequest is destroyed before reaching the compositor it
runs the callback with an empty CopyOutputResult.

SnapshotAsync::ScaleCopyOutputResult and SnapshotAsync::EncodeCopyOutputResult
both check for the empty result.

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_and...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_and...
ui/snapshot/snapshot_android.cc:9: #include
"cc/resources/single_release_callback.h"
On 2014/05/14 01:13:18, Ted C wrote:
> Do we need this include or the following one?

Yeh, it doesn't compile otherwise because the copy_output_request.h header has a
scoped_ptr<SingleReleaseCallback> and it needs the destructor to compile that. I
should move this header over there though, I'll do that.

The SkBitmap header is also needed to compile tho I didn't figure out why it is
so.

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
File ui/snapshot/snapshot_async.cc (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.cc:1: // Copyright (c) 2014 The Chromium Authors. All
rights reserved.
On 2014/05/14 01:13:18, Ted C wrote:
> same here

Done.

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.cc:40: unsigned char* pixels =
On 2014/05/13 23:43:13, sky wrote:
> Don't you need to lockPixels before accessing them?

We should, I'll add that.

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.cc:41: reinterpret_cast<unsigned
char*>(bitmap.pixelRef()->pixels());
On 2014/05/13 23:43:13, sky wrote:
> Might pixelRef return NULL?

No, the bitmap here always has content in it returned from the compositor. It
would return a NULL bitmap instead if the readback didn't happen.

I changed it to bitmap.getPixels() anyway since I want to use SkAutoLockPixels
which is on the bitmap.

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
File ui/snapshot/snapshot_async.h (right):

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.h:1: // Copyright (c) 2014 The Chromium Authors. All
rights reserved.
On 2014/05/14 01:13:18, Ted C wrote:
> don't need the (c) anymore

Done.

https://codereview.chromium.org/281003002/diff/60001/ui/snapshot/snapshot_asy...
ui/snapshot/snapshot_async.h:37: SnapshotAsync();  // Not instantiable.
On 2014/05/13 23:43:13, sky wrote:
> SnapshotAsync->DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

[sky, 2014-05-14 18:21:37]

LGTM

[danakj, 2014-05-14 18:25:42]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-14 18:26:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/281003002/80001

[Ken Russell (switch to Gerrit), 2014-05-14 18:48:19]

This won't pass the CQ until https://codereview.chromium.org/281193002 lands. Or
at least it shouldn't, because linux_gpu_triggered_tests will fail without that
CL and that's now a required step on the CQ. Please don't sidestep this failure;
let's make sure everything's working as expected.

[danakj, 2014-05-14 19:29:59]

On 2014/05/14 18:48:19, Ken Russell wrote:
> This won't pass the CQ until https://codereview.chromium.org/281193002 lands.
Or
> at least it shouldn't, because linux_gpu_triggered_tests will fail without
that
> CL and that's now a required step on the CQ. Please don't sidestep this
failure;
> let's make sure everything's working as expected.

Oh I hadn't realized that it was already part of the CQ.

[commit-bot: I haz the power, 2014-05-14 21:06:07]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-05-14 21:12:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 21:12:42]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[danakj, 2014-05-15 14:21:47]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-15 14:22:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/281003002/80001

[commit-bot: I haz the power, 2014-05-15 15:20:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[danakj, 2014-05-15 15:23:27]

linux_gpu passes now, thanks ken

[commit-bot: I haz the power, 2014-05-15 15:24:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:24:35]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[danakj, 2014-05-15 15:29:22]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:31:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/281003002/80001

[danakj, 2014-05-15 15:32:54]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:34:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/281003002/100001

[danakj, 2014-05-15 15:34:10]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:36:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/281003002/120001

[danakj, 2014-05-15 16:13:09]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-15 16:14:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/281003002/140001

[commit-bot: I haz the power, 2014-05-15 17:46:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-15 19:36:40]

Change committed as 270778