Improve Aura overdraw by changing hole calculation

This patch lets the browser window punch a hole in the DesktopBackgroundView,
thus eliminating overdraw.
To do this, the hole punching logic was changed to look at the entire tree.
Drawing the screen is a preorder traversal through all the layers. Thus, to
compute the hole, we have to look at all of the layers after a particular layer
in the preorder traversal.
This is not an efficient algorithm, but it is simplest I could come up with.
Plus we will be moving to the WebKit compositor soon.

Hopefully this will improve the frame rate for the Aura demos

Note: I will put the changes to Transform into another patch.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107360

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107440

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110107

[pkotwicz, 2011-10-21 22:53:41]

This patch lets the browser window punch a hole in the DesktopBackgroundView,
thus eliminating overdraw.
To do this, the hole punching logic was changed to look at the entire tree.
Drawing the screen is a preorder traversal through all the layers. Thus, to
compute the hole, we have to look at all of the layers after a particular layer
in the preorder traversal.
This is not an efficient algorithm, but it is simplest I could come up with.
Plus we will be moving to the WebKit compositor soon.

Hopefully this will improve the frame rate for the Aura demos

[Emmanuel Saint-loubert-Bié, 2011-10-21 23:11:28]

Groovy :-) thanks.

[Ian Vollick, 2011-10-22 01:05:25]

On 2011/10/21 23:11:28, Emmanuel Saint-loubert wrote:
> Groovy :-) thanks.
Nice! Just have a couple of minor comments.

[Ian Vollick, 2011-10-22 01:06:03]

Sorry for the spam. Here are those comments.

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.cc#n...
ui/gfx/compositor/layer.cc:374: layer_traversal[j]->GetCombinedOpacity())
Shouldn't we punch a hole only if layer_traversal[j]->GetCombinedOpacity() is
1.0? Otherwise, we'd still see a bit of layer i under layer j, wouldn't we?

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer_unit...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer_unit...
ui/gfx/compositor/layer_unittest.cc:5: #include <string>
Is this used?

[sky, 2011-10-24 14:58:00]

Looks like the webkit compositor has landed. Do we still need this change?

[jonathan.backer, 2011-10-24 15:09:23]

On 2011/10/24 14:58:00, sky wrote:
> Looks like the webkit compositor has landed. Do we still need this change?

The webkit compositor is behind a flag. It is not ready for general comsumption:
- missing accelerated content (under review but has some regressions)
- has not mechanism to address overdraw (probably 2-3 weeks out)

So yes, I think this is still necesssary for the near term (especially for demos
next week on low powered hardware).

[sky, 2011-10-24 15:44:52]

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.cc#n...
ui/gfx/compositor/layer.cc:336: void Layer::RecomputeHole() {
I don't like all the tree walking we do on any interesting change. Imagine if
during one animation cycle we changed the bounds of a bunch of layers.
Could we instead set a boolean indicating we need to update the regions to paint
and then in DrawTree update them?

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.cc#n...
ui/gfx/compositor/layer.cc:357: // Compute transform to go from bounds of layer
|j| to local bounds of layer |i|.
This line is > 80 chars

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.cc#n...
ui/gfx/compositor/layer.cc:359:
layer_traversal[j]->GetTransformRelativeTo(root_layer,
This is a lot of transform building. Rather than having
GeneratePreorderTraversal generate Layers, maybe it should generate:
struct {
  Transform transform_relative_to_root;
  Layer* layer;
}

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8368013/diff/2008/ui/gfx/compositor/layer.h#ne...
ui/gfx/compositor/layer.h:193: // which cannot punch a hole in  another layer
such as non visible layers
'in  another' -> 'in another'

[piman, 2011-10-24 16:32:46]

On 2011/10/24 15:09:23, jonathan.backer wrote:
> On 2011/10/24 14:58:00, sky wrote:
> > Looks like the webkit compositor has landed. Do we still need this change?

At least, please merge with it and check that it still compiles and tests pass
when use_webkit_compositor=1

> 
> The webkit compositor is behind a flag. It is not ready for general
comsumption:
> - missing accelerated content (under review but has some regressions)
> - has not mechanism to address overdraw (probably 2-3 weeks out)
> 
> So yes, I think this is still necesssary for the near term (especially for
demos
> next week on low powered hardware).

[pkotwicz, 2011-10-24 22:46:21]

Tested with webkit compositor.

[sky, 2011-10-24 23:25:50]

http://codereview.chromium.org/8368013/diff/12002/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8368013/diff/12002/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:428: for (size_t i = 0; i < layer_traversal.size();
i++) {
Seems a lot better to have GeneratePreorderTraversal generate all this for you.
That way GetCombinedOpacity and GetTransformRelativeTo aren't rewalking the
tree.

If you store the bounds of each layer relative to the root, do you even need to
store the transform? Wouldn't this also save computing this information multiple
times per layer?

http://codereview.chromium.org/8368013/diff/12002/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:466: if (opacities[i] != 1.0f || opacities[j] !=
1.0f)
This check is cheap. We should do it first.

http://codereview.chromium.org/8368013/diff/12002/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:480: RecomputeDrawsContent();
Shouldn't this be outside the for loop?

http://codereview.chromium.org/8368013/diff/12002/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:483: recompute_hole_ = false;
Shouldn't this be outside the for loop?

http://codereview.chromium.org/8368013/diff/12002/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:529: if (!layer_updated_externally_)
Change this to call DropTexture.

[pkotwicz, 2011-10-25 21:17:24]

Changes as requested.

Tested patch on low end hardware with Aura. And this improved the dragging
experience when many windows were open on the screen

[sky, 2011-10-25 21:44:09]

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:410: const ui::Transform& current_transform, float
current_opacity) {
each param on its own line.

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:410: const ui::Transform& current_transform, float
current_opacity) {
All of the params should be for the parent, not this. The implementation here
should multiply by this's opacity/transform/bounds. This slightly simplifies the
implementation, is less error prone and avoids unnecessary calculations.

You can see how it's wrong now in Layer::RecomputeHole where you pass in 1.0f,
which means you're ignoring the opacity of the root.

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:411: if (!visible_)
Should this return if opacity == 0 too?

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:429: static_cast<float>(child->bounds().x()),
Wrapped lines should be indented by 4.

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:448: gfx::Rect bounds =
gfx::Rect(layer->bounds().size());
These calculations are expensive. Is it worth only doing this for largish
layers?

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:469: if (traversal[i].combined_opacity != 1.0f ||
Shouldn't we make GeneratePreorderTraversal ignore layers whose opacity is not 1
then?

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:29: namespace {
Move this to the private section of Layer.

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:31: // contains info for a layer with respect to the
root layer.
Contains

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:33: public:
don't use a public section with structs.

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:38: // Copy and assignment are allowed.
Remove this comment. Generally when we use structs in the code we want
copy/assignment, so you would only comment if you didn't want them.

http://codereview.chromium.org/8368013/diff/16005/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:230: const ui::Transform& current_transform, float
current_opacity);
When you wrap, you want each argument on its own line. Also, the style guide
says out params should be last.
And I think this method should be renamed to something like GetLayerProperties

[sky, 2011-10-26 00:08:28]

LGTM

http://codereview.chromium.org/8368013/diff/21017/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8368013/diff/21017/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:197: public:
Indentation is off here. Should be like a class.

http://codereview.chromium.org/8368013/diff/21017/ui/gfx/compositor/layer.h#n...
ui/gfx/compositor/layer.h:223: void
GetLayerProperties(std::vector<LayerProperties>* traversal,
our params last.

[sky, 2011-10-26 00:08:39]

'our params' -> out params

[pkotwicz, 2011-10-26 17:54:59]

Fixed views_unittests. No other changes. Making sure change is still ok

[sky, 2011-10-26 21:03:14]

LGTM

[pkotwicz, 2011-11-09 21:41:49]

Relanding now that http://codereview.chromium.org/8418035/ has landed

[pkotwicz, 2011-11-09 21:46:34]

Relanding now that http://codereview.chromium.org/8418035/ has landed

[sky, 2011-11-14 21:49:48]

SLGTM