cc: Async readback.

cc: Async readback.

Provide a path to get a readback of a layer's subtree via an
asynchronous mechanism.

This path is used for all the cc pixel tests, to show that it
works. Also by some unit tests:

LayerTreeHostTestAsyncReadback.GLRenderer_RunSingleThread
LayerTreeHostTestAsyncReadback.GLRenderer_RunMultiThread
LayerTreeHostTestAsyncReadback.SoftwareRenderer_RunSingleThread
LayerTreeHostTestAsyncReadback.SoftwareRenderer_RunMultiThread
LayerTreeHostTestAsyncReadbackLayerDestroyed.RunSingleThread
LayerTreeHostTestAsyncReadbackLayerDestroyed.RunMultiThread

BUG=179896
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196747

[piman, 2013-04-25 02:13:51]

nice!

https://codereview.chromium.org/14060015/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14060015/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:1774: GetFramebufferPixels(bitmap->getPixels(),
gfx::Rect(render_pass_size));
How hard would it be to add the flexibility, and asyncness of
GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture ?

GPU scale is needed on at least some chrome OS devices (Alex at least? Probably
Pixel too) to avoid hundreds of ms jank in the GPU process - reading back the
entire buffer is just too much.
The asyncness is to avoid janking the compositor thread itself, because
readPixels is essentially a glFinish, so it has to wait for GPU process
completion, and it breaks pipelining (the tab capture guys want to do this every
frame).

[danakj, 2013-04-25 18:39:15]

Thanks for lookin'. Using the async readback for all the pixel tests now.

https://codereview.chromium.org/14060015/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14060015/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:1774: GetFramebufferPixels(bitmap->getPixels(),
gfx::Rect(render_pass_size));
On 2013/04/25 02:13:52, piman wrote:
> How hard would it be to add the flexibility, and asyncness of
> GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture ?
> 
> GPU scale is needed on at least some chrome OS devices (Alex at least?
Probably
> Pixel too) to avoid hundreds of ms jank in the GPU process - reading back the
> entire buffer is just too much.

Flexibility is fine, pass around a struct with more params than a callback
alone. I'll do that in a separate followup with tests.

> The asyncness is to avoid janking the compositor thread itself, because
> readPixels is essentially a glFinish, so it has to wait for GPU process
> completion, and it breaks pipelining (the tab capture guys want to do this
every
> frame).

The asyncyness I need to understand better, but also seems totally fine. Also
will do as a followup.

[piman, 2013-04-26 00:15:02]

LGTM + a bunch of nits.

https://codereview.chromium.org/14060015/diff/9001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/14060015/diff/9001/cc/output/direct_renderer....
cc/output/direct_renderer.cc:11: #include "base/hash_tables.h"
nit: not using this?

https://codereview.chromium.org/14060015/diff/9001/cc/output/direct_renderer.h
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/14060015/diff/9001/cc/output/direct_renderer....
cc/output/direct_renderer.h:96: const RenderPass* render_pass);
nit: was fine before one a single line

https://codereview.chromium.org/14060015/diff/9001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/14060015/diff/9001/cc/output/renderer.h#newcode8
cc/output/renderer.h:8: #include <map>
nit: not using this?

https://codereview.chromium.org/14060015/diff/9001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/14060015/diff/9001/cc/quads/render_pass.h#new...
cc/quads/render_pass.h:13: #include "base/hash_tables.h"
nit: not using this

https://codereview.chromium.org/14060015/diff/9001/cc/trees/layer_tree_host_u...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/14060015/diff/9001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest.cc:2753: scoped_refptr<FakeContentLayer> root;
nit: trailing _ on fields, here and below

[danakj, 2013-04-26 01:00:06]

https://codereview.chromium.org/14060015/diff/9001/cc/output/direct_renderer.h
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/14060015/diff/9001/cc/output/direct_renderer....
cc/output/direct_renderer.h:96: const RenderPass* render_pass);
On 2013/04/26 00:15:02, piman wrote:
> nit: was fine before one a single line

Ah, yes I added and removed stuff. Fixed.

https://codereview.chromium.org/14060015/diff/9001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/14060015/diff/9001/cc/output/renderer.h#newcode8
cc/output/renderer.h:8: #include <map>
On 2013/04/26 00:15:02, piman wrote:
> nit: not using this?

Done.

https://codereview.chromium.org/14060015/diff/9001/cc/trees/layer_tree_host_u...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/14060015/diff/9001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest.cc:2753: scoped_refptr<FakeContentLayer> root;
On 2013/04/26 00:15:02, piman wrote:
> nit: trailing _ on fields, here and below

Done.

[commit-bot: I haz the power, 2013-04-26 01:00:57]

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

[commit-bot: I haz the power, 2013-04-26 03:53:06]

Change committed as 196592

[commit-bot: I haz the power, 2013-04-26 13:11:57]

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

[commit-bot: I haz the power, 2013-04-26 13:13:24]

Change committed as 196713

[danakj, 2013-04-26 13:16:15]

Crap, I tried to upload and land, but it just landed again. >_<

[commit-bot: I haz the power, 2013-04-26 13:19:42]

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

[danakj, 2013-04-26 13:20:00]

3rd time's the charm!

[commit-bot: I haz the power, 2013-04-26 15:33:26]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-26 15:34:47]

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

[commit-bot: I haz the power, 2013-04-26 16:18:26]

Change committed as 196747