exo: Only create new SurfaceLayer if layer properties change.

exo: Only create new SurfaceLayer if layer properties change.

This allows us to avoid an unnecessary browser compositor commit and draw if only the contents of the buffer change, which saves CPU.

BUG=610086
Committed: https://crrev.com/f4c3f293f20d87a6383da5c8d2df4763d89f2999
Cr-Commit-Position: refs/heads/master@{#399339}

[jbauman, 2016-06-08 21:10:07]

Description was changed from

==========
exo: Only create new SurfaceLayer if layer properties change.
BUG=
==========

to

==========
exo: Only create new SurfaceLayer if layer properties change.

This allows us to avoid an unnecessary browser compositor commit and draw if
only the contents of the buffer change, which saves CPU.

BUG=610086
==========

[jbauman, 2016-06-08 21:10:16]

jbauman@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2016-06-09 14:19:29]

https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
components/exo/surface.cc:272: modified_layer_info_ = true;
Can we evaluate this as part of CommitSurfaceHierarchy() instead? Same goes for
the other functions that set pending state below. This way when more complicated
clients make a large number of changes before they commit and some changes that
cancel out other changes, tht will not result in unnecessary work at the time of
commit().

https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
components/exo/surface.cc:464: CheckForCommittingToNewSurface();
Instead of adding this function, can the requirement to use a new surface be a
return value of CommitSurfaceHierarchy?

https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
components/exo/surface.cc:814: bool new_surface = false;
is this variable needed? would "surface_id_ != old_surface_id" not be the same?

https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.h#ne...
components/exo/surface.h:266: 
Check my comments in the .cc file but I'm curious to know if this additional
state can't be avoided using some better integration with
CommitSurfaceHierarchy().

[jbauman, 2016-06-09 18:25:31]

On 2016/06/09 14:19:29, reveman wrote:
> https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc
> File components/exo/surface.cc (right):
> 
>
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
> components/exo/surface.cc:272: modified_layer_info_ = true;
> Can we evaluate this as part of CommitSurfaceHierarchy() instead? Same goes
for
> the other functions that set pending state below. This way when more
complicated
> clients make a large number of changes before they commit and some changes
that
> cancel out other changes, tht will not result in unnecessary work at the time
of
> commit().

We need to evaluate all this before CommitSurfaceHierarchy so it can deal
properly with synchronized subtrees. For example if we have 
  A
 / \
B   C
|
D

(where A is root) and all are synchronized, then a change to the bounds of B
needs to cause every surface to make a new SurfaceId. So C and A need to know
that B's bounds changed before they do CommitSurfaceHierarchy so they can commit
their contents to a new surface.
> 
>
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
> components/exo/surface.cc:464: CheckForCommittingToNewSurface();
> Instead of adding this function, can the requirement to use a new surface be a
> return value of CommitSurfaceHierarchy?
> 
>
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
> components/exo/surface.cc:814: bool new_surface = false;
> is this variable needed? would "surface_id_ != old_surface_id" not be the
same?
> 
> https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.h
> File components/exo/surface.h (right):
> 
>
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.h#ne...
> components/exo/surface.h:266: 
> Check my comments in the .cc file but I'm curious to know if this additional
> state can't be avoided using some better integration with
> CommitSurfaceHierarchy().

[reveman, 2016-06-09 18:48:49]

On 2016/06/09 at 18:25:31, jbauman wrote:
> On 2016/06/09 14:19:29, reveman wrote:
> > https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc
> > File components/exo/surface.cc (right):
> > 
> >
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
> > components/exo/surface.cc:272: modified_layer_info_ = true;
> > Can we evaluate this as part of CommitSurfaceHierarchy() instead? Same goes
for
> > the other functions that set pending state below. This way when more
complicated
> > clients make a large number of changes before they commit and some changes
that
> > cancel out other changes, tht will not result in unnecessary work at the
time of
> > commit().
> 
> We need to evaluate all this before CommitSurfaceHierarchy so it can deal
properly with synchronized subtrees. For example if we have 
>   A
>  / \
> B   C
> |
> D
> 
> (where A is root) and all are synchronized, then a change to the bounds of B
needs to cause every surface to make a new SurfaceId. So C and A need to know
that B's bounds changed before they do CommitSurfaceHierarchy so they can commit
their contents to a new surface.


Makes sense that we need some temporary state and new function in addition to
CommitSurfaceHierarchy then.

I think it would still be nice to avoid determining if anything changed when
setting pending state and instead do it just before Commit, that should still be
possible right?

Patch looks good in general otherwise % a few minor naming nits which I'll
comment on during the next review round.

[jbauman, 2016-06-09 21:46:18]

On 2016/06/09 18:48:49, reveman wrote:
> On 2016/06/09 at 18:25:31, jbauman wrote:
> > On 2016/06/09 14:19:29, reveman wrote:
> > >
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc
> > > File components/exo/surface.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2041193006/diff/1/components/exo/surface.cc#n...
> > > components/exo/surface.cc:272: modified_layer_info_ = true;
> > > Can we evaluate this as part of CommitSurfaceHierarchy() instead? Same
goes
> for
> > > the other functions that set pending state below. This way when more
> complicated
> > > clients make a large number of changes before they commit and some changes
> that
> > > cancel out other changes, tht will not result in unnecessary work at the
> time of
> > > commit().
> > 
> > We need to evaluate all this before CommitSurfaceHierarchy so it can deal
> properly with synchronized subtrees. For example if we have 
> >   A
> >  / \
> > B   C
> > |
> > D
> > 
> > (where A is root) and all are synchronized, then a change to the bounds of B
> needs to cause every surface to make a new SurfaceId. So C and A need to know
> that B's bounds changed before they do CommitSurfaceHierarchy so they can
commit
> their contents to a new surface.
> 
> 
> Makes sense that we need some temporary state and new function in addition to
> CommitSurfaceHierarchy then.
> 
> I think it would still be nice to avoid determining if anything changed when
> setting pending state and instead do it just before Commit, that should still
be
> possible right?
> 
> Patch looks good in general otherwise % a few minor naming nits which I'll
> comment on during the next review round.

Ok, I've modified this to keep two copies of most of the state and compare them
on commit to see if the layer is likely to be modified. I'm still invalidating
the state immediately if sub surfaces are moved around, because keeping around
two copies of that state is a bit more difficult.

[reveman, 2016-06-10 03:42:21]

lgtm with a bunch of naming nits

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:430: if (has_pending_contents_ &&
can you split this into multiple if-statements to make it a bit easier to read?

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:645: void Surface::CheckForCommittingToNewSurface() {
I feel like the correct name for this function is
"SetChildrenCommitToNewSurfaceIfChildLayersChanged" which is not much of a
simplification from just using HaveChildLayersChanged and
SetChildrenCommitToNewSurface directly instead of having this function. Your
call. Just make sure the name aligns with the choice of name for
SetChildrenCommitToNewSurface. If you go with the name suggestion I make below
then it would be CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces.

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:655: alpha(1.0f) {}
feel free to move these to the deceleration if you want. I'd like to do that for
all of exo/ at some point but as you're doing some of that already in this patch
there's no reason to hold back with more

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:659: bool Surface::State::ChangesLayerState(const
State& other) {
"Changes" is a bit weird as it implies something specific usage of this class.
invert it and name it LayerStateIsEqual? or is this just not the != operator?

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:667: bool Surface::HaveChildLayersChanged() {
This is not only dealing with child layers but also the layer itself. Can we use
the same pattern as CommitSurfaceHierarchy and make that less confusing by using
the term "Hierarchy" when we mean the surface itself and all its sub surfaces. 

HasLayerHierarchyChanged()?

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:671: for (auto& surf : pending_sub_surfaces_) {
nit: const auto&? and s/surf/sub_surface_entry/

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:678: void Surface::SetChildrenCommitToNewSurface() {
nit: SetSurfaceHierarchyNeedsCommitToNewSurfaces()

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.cc:680: for (auto& surf : pending_sub_surfaces_) {
nit: s/surf/sub_surface_entry/

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.h:252: bool HaveChildLayersChanged();
Can this be const? name seem to suggest it

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.h:282: bool modified_layer_info_ = true;
nit: can this be named "has_pending_layer_changes_" to be somewhat consistent
with "has_pending_contents_"?

https://codereview.chromium.org/2041193006/diff/20001/components/exo/surface....
components/exo/surface.h:287: bool commit_to_new_surface_ = true;
nit: needs_commit_to_new_surface_

[jbauman, 2016-06-11 00:53:05]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-06-11 00:53:06]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2041193006/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-06-11 00:53:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041193006/40001

[commit-bot: I haz the power, 2016-06-11 00:57:43]

Description was changed from

==========
exo: Only create new SurfaceLayer if layer properties change.

This allows us to avoid an unnecessary browser compositor commit and draw if
only the contents of the buffer change, which saves CPU.

BUG=610086
==========

to

==========
exo: Only create new SurfaceLayer if layer properties change.

This allows us to avoid an unnecessary browser compositor commit and draw if
only the contents of the buffer change, which saves CPU.

BUG=610086
==========

[commit-bot: I haz the power, 2016-06-11 00:57:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-11 00:57:59]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-11 01:00:29]

Description was changed from

==========
exo: Only create new SurfaceLayer if layer properties change.

This allows us to avoid an unnecessary browser compositor commit and draw if
only the contents of the buffer change, which saves CPU.

BUG=610086
==========

to

==========
exo: Only create new SurfaceLayer if layer properties change.

This allows us to avoid an unnecessary browser compositor commit and draw if
only the contents of the buffer change, which saves CPU.

BUG=610086

Committed: https://crrev.com/f4c3f293f20d87a6383da5c8d2df4763d89f2999
Cr-Commit-Position: refs/heads/master@{#399339}
==========

[commit-bot: I haz the power, 2016-06-11 01:00:31]

Patchset 3 (id:??) landed as
https://crrev.com/f4c3f293f20d87a6383da5c8d2df4763d89f2999
Cr-Commit-Position: refs/heads/master@{#399339}