cc: Add tile-free software compositing mode.

cc: Add tile-free software compositing mode.

In Android WebView, CC will be run in a hybrid hardware/software mode
where the renderer used may vary frame by frame.  We're planning to
reserve the ResourceProvider exclusively for GL tiles; for the software
case, we'll draw directly from the PicturePile instead.

This patch implements this draw-direct-from-PicturePile flow by adding
PictureDrawQuad support to SoftwareRenderer and introducing a
OutputSurface-based setting telling PictureLayerImpl to only append
PictureDrawQuads for the current frame.

NOTRY=true
BUG=178398
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199313

[aelias_OOO_until_Jul13, 2013-04-28 03:58:17]

PTAL.

[danakj, 2013-04-28 14:23:32]

Right now software mode doesn't get much from resource provider as it does do
ubercomp. What about when it does shm software support and is our mechanism for
passing off software resources?

Or if this mode is not going to use anything from the compositor except the
pictures, why does it need to involve the hardware compositor instance at all?

[aelias_OOO_until_Jul13, 2013-04-28 20:09:43]

This doc should provide more context:
https://docs.google.com/a/google.com/document/d/1AD5zswEy9Noi6ThAuKgbtSq_nwL5...

On 2013/04/28 14:23:32, danakj wrote:
> Right now software mode doesn't get much from resource provider as it does do
> ubercomp. What about when it does shm software support and is our mechanism
for
> passing off software resources?

This will be used in a single-process Chromium with only one compositor instance
(the renderer compositor).

> 
> Or if this mode is not going to use anything from the compositor except the
> pictures, why does it need to involve the hardware compositor instance at all?

See the "Problem" section in the doc above.  Essentially we are forced to
imitate the architecture of the legacy Android browser, which draws directly
from SkPictures for software draws (used mainly for headless snapshot use-cases)
and has tiled rendering for hardware draws (used for scrolling web browser type
use-cases).

[enne (OOO), 2013-04-28 22:42:27]

I asked skaslev this too[0], but this path needs tests.

I am in the middle of adding software renderer pixel tests[1] and also some
general picture draw quad tests[2].  Once [2] is landed then you can change the
test to exercise the software renderer path too and perhaps add a pixel test for
the needs blending case.

Could you also add a unit test to make sure that appending quads with this flag
only gets you the quad types you're expecting?

[0] https://codereview.chromium.org/14069011/#msg4
[1] https://codereview.chromium.org/14135011/
[2] https://codereview.chromium.org/14001009/

https://codereview.chromium.org/14417014/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/14417014/diff/1/cc/layers/picture_layer_impl....
cc/layers/picture_layer_impl.cc:199: if
(append_quads_data->cannot_append_tile_draw_quads &&
I think you should not append one picture quad per tile, and maybe just pull all
this up to the top and append one picture quad for the whole layer like this
does:
https://codereview.chromium.org/13863015/diff/4001/cc/layers/picture_layer_im...

https://codereview.chromium.org/14417014/diff/1/cc/layers/picture_layer_impl....
cc/layers/picture_layer_impl.cc:555: if
(layer_tree_impl()->settings().tile_free_software_compositing)
Settings are considered to be immutable.  I'm not sure how you can have a hybrid
hardware/software layer tree if you have this setting turned on, since this
conditional is undesirable for normal operation.

To be honest, I'm confused how activation enters into consideration for the
capture picture mode.

https://codereview.chromium.org/14417014/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/1/cc/output/software_renderer.c...
cc/output/software_renderer.cc:279: gfx::RectFToSkRect(quad->tex_coord_rect),
I would actually like to get rid of this tex coord rect off of PictureDrawQuad. 
I don't think it has any business being there.  Given a content rect, a geometry
rect, and pile, the renderer can make the decision to add border texels if it
needs to.

[kaanb1, 2013-04-29 16:56:26]

https://codereview.chromium.org/14417014/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14417014/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1397: ResourceProvider*
software_resource_provider = NULL;
nit: consider s/software_resource_provider/resource_provider_for_software/
Right now it reads like as if we have a provider for software resources.

[aelias_OOO_until_Jul13, 2013-04-30 07:28:20]

PTAL, I got rid of the stopgap LayerTreeSetting and command-line flag and
switched to a proper per-frame call from OutputSurface.  (Will write tests
soon.)

https://codereview.chromium.org/14417014/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/14417014/diff/1/cc/layers/picture_layer_impl....
cc/layers/picture_layer_impl.cc:199: if
(append_quads_data->cannot_append_tile_draw_quads &&
On 2013/04/28 22:42:27, enne wrote:
> I think you should not append one picture quad per tile, and maybe just pull
all
> this up to the top and append one picture quad for the whole layer like this
> does:
>
https://codereview.chromium.org/13863015/diff/4001/cc/layers/picture_layer_im...

Agreed, although cut-and-pasting that snippet results in broken rendering for
me.  Did you change the meaning of one of the rects as part of that patch?

https://codereview.chromium.org/14417014/diff/1/cc/layers/picture_layer_impl....
cc/layers/picture_layer_impl.cc:555: if
(layer_tree_impl()->settings().tile_free_software_compositing)
I deleted this, it was a workaround for the issue fixed by
https://codereview.chromium.org/14061028/

https://codereview.chromium.org/14417014/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14417014/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1397: ResourceProvider*
software_resource_provider = NULL;
On 2013/04/29 16:56:26, kaanb1 wrote:
> nit: consider s/software_resource_provider/resource_provider_for_software/
> Right now it reads like as if we have a provider for software resources.

Deleted this code anyway.

[aelias_OOO_until_Jul13, 2013-05-07 02:04:38]

PTAL.  I added a quad generation test and turned on your pixel tests (fixing a
mistake in green_quad tex_coords_rect that SoftwareRenderer was more sensitive
to).  I didn't add a blending test because your existing tests fail without
blending already.

> I would actually like to get rid of this tex coord rect off of
PictureDrawQuad.

Looks like that would be fairly involved so I'd rather not do it as part of this
patch.

[enne (OOO), 2013-05-07 06:11:22]

https://codereview.chromium.org/14417014/diff/19001/cc/layers/append_quads_da...
File cc/layers/append_quads_data.h (right):

https://codereview.chromium.org/14417014/diff/19001/cc/layers/append_quads_da...
cc/layers/append_quads_data.h:15: : cannot_append_tile_draw_quads(false),
Please don't use negative booleans.  :)

https://codereview.chromium.org/14417014/diff/19001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/19001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:323: if (!resource_provider_)
Does this line imply that you can't have render passes for a software-only
WebView?

https://codereview.chromium.org/14417014/diff/19001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/19001/cc/output/software_render...
cc/output/software_renderer.cc:75: if (resource_provider_) {
Can you fill in these values here with something other than the defaults of 0?

[aelias_OOO_until_Jul13, 2013-05-07 23:51:47]

https://codereview.chromium.org/14417014/diff/19001/cc/layers/append_quads_da...
File cc/layers/append_quads_data.h (right):

https://codereview.chromium.org/14417014/diff/19001/cc/layers/append_quads_da...
cc/layers/append_quads_data.h:15: : cannot_append_tile_draw_quads(false),
On 2013/05/07 06:11:22, enne wrote:
> Please don't use negative booleans.  :)

Renamed to allow_tile_draw_quads.

https://codereview.chromium.org/14417014/diff/19001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/19001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:323: if (!resource_provider_)
On 2013/05/07 06:11:22, enne wrote:
> Does this line imply that you can't have render passes for a software-only
> WebView?

Yes, for now.  We may revisit and add support later if there's a compelling use
case.

https://codereview.chromium.org/14417014/diff/19001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/19001/cc/output/software_render...
cc/output/software_renderer.cc:75: if (resource_provider_) {
On 2013/05/07 06:11:22, enne wrote:
> Can you fill in these values here with something other than the defaults of 0?

Hmm, that doesn't seem quite right.  Nobody ever looks at the capabilities of a
local-variable SoftwareRenderer so that would be dead code.  If we think the
zero defaults are bad, seems to me it should be changed in the
RendererCapabilities constructor.

[enne (OOO), 2013-05-09 17:35:43]

lgtm

https://codereview.chromium.org/14417014/diff/19001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/19001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:323: if (!resource_provider_)
On 2013/05/07 23:51:47, aelias wrote:
> On 2013/05/07 06:11:22, enne wrote:
> > Does this line imply that you can't have render passes for a software-only
> > WebView?
> 
> Yes, for now.  We may revisit and add support later if there's a compelling
use
> case.

Ok.  You're the one making the requirements here and I don't think punting on
this adds complication.

I do think that render passes would be trickier to implement in a way that would
also work for capture picture (without rasterizing intermediate bitmaps for the
surfaces) since you'd have to get saveLayer calls in the right place.

https://codereview.chromium.org/14417014/diff/19001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/14417014/diff/19001/cc/output/software_render...
cc/output/software_renderer.cc:75: if (resource_provider_) {
On 2013/05/07 23:51:47, aelias wrote:
> On 2013/05/07 06:11:22, enne wrote:
> > Can you fill in these values here with something other than the defaults of
0?
> 
> Hmm, that doesn't seem quite right.  Nobody ever looks at the capabilities of
a
> local-variable SoftwareRenderer so that would be dead code.  If we think the
> zero defaults are bad, seems to me it should be changed in the
> RendererCapabilities constructor.

Ok, I'll buy that argument.

[commit-bot: I haz the power, 2013-05-10 00:05:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/14417014/36001

[commit-bot: I haz the power, 2013-05-10 00:06:38]

Change committed as 199313