New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 755351 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Add a new flag to indicate layer_property_changed_from_animation_

Project Member Reported by wutao@chromium.org, Aug 14 2017

Issue description

We would like to use render surface cache during layer animation.

One changeling today is that layer_property_changed_ in layer_impl.cc can be flagged through NoteLayerPropertyChanged in two ways:

1) inside layer_impl.cc, called by Set*(), e.g. SetBounds() [1].
[1] https://cs.chromium.org/chromium/src/cc/layers/layer_impl.cc?l=511&rcl=c57207227e8a945f168575553cf82233979803ec

2) inside layer_tree_impl.cc, called by ayerTreeImpl::MoveChangeTrackingToLayers [2].
In the second call, it will check
    if (layer->LayerPropertyChanged())
      layer->NoteLayerPropertyChanged();
[2] https://cs.chromium.org/chromium/src/cc/trees/layer_tree_impl.cc?l=503&rcl=b8e90b623d45c0588550935f29dc2c0e080798aa


LayerImpl::LayerPropertyChanged() will include PropertyTree damages, such as transform_changed and effect_changed [3].
[3] https://cs.chromium.org/chromium/src/cc/layers/layer_impl.cc?l=419&rcl=c57207227e8a945f168575553cf82233979803ec


The problem is that transform_changed and effect_changed can be flagged during animation. This caused that we can not reused the cache.

If we can add a separated flag layer_property_changed_from_animation_, then we could reuse the cache.
 

Comment 1 by wutao@chromium.org, Aug 14 2017

Tested locally, by commenting out the follow in MoveChangeTrackingToLayers for quick test:
if (layer->LayerPropertyChanged())
      layer->NoteLayerPropertyChanged();

I can get:
1. ~30% improvement in Close Window animation (hide animation)
2. ~50% improvement in Maximizing window animation (cross fade animation)


I can put out a cl for review soon.

Comment 2 by wutao@chromium.org, Aug 15 2017

Labels: -M-16 M-61
Uploaded a cl to:
https://chromium-review.googlesource.com/c/615023

Comment 3 by wutao@chromium.org, Aug 15 2017

#1 are tested on linux build.

On LINK, it is about 20% improvement in Maximizing window animation (cross fade animation)

Comment 4 by wutao@chromium.org, Aug 15 2017

Seem no improvement at KEVIN, although the caching is working.

Comment 5 by wutao@chromium.org, Aug 16 2017

Labels: -M-61 M-62
When use 10s duration for animation, there is about 15% improvement in Maximizing window animation (cross fade animation).

In normal duration, 15% is about 1-2 frames. Because the variations of the frame rate is a little big, so the difference is not obvious.

Comment 6 by wutao@chromium.org, Aug 16 2017

Added a follow up bug to have a better mechanism to further separate the layer damage sources:
https://bugs.chromium.org/p/chromium/issues/detail?id=755828

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f

commit bb1af5a4c94ddeab5c3984303ef22d31f7881b4f
Author: wutao <wutao@chromium.org>
Date: Wed Aug 16 17:35:35 2017

Add a new flag to indicate layer_property_changed_from_property_trees.

We would like to use render surface cache during layer animation. One
changeling is that layer_property_changed_ in layer_impl.cc can be
flagged through NoteLayerPropertyChanged while there is an animation.
Adding a new flag layer_property_changed_from_property_trees_ can allow
us to distinguish the sources of the layer property changes.

Changes:
1. Add the new flag layer_property_changed_from_property_trees_.
2. Only set the new flag in LayerTreeImpl::MoveChangeTrackingToLayers.
3. Add test expectations for new flag.

Bug:  755351 
Test: with caching render surface and test new flag in cc_unittests.
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ie081acae9d7ca88aba56826cc3089ea78d1003cf
Reviewed-on: https://chromium-review.googlesource.com/615023
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494842}
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/layers/layer_impl.cc
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/layers/layer_impl.h
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/layers/layer_impl_unittest.cc
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/layers/layer_unittest.cc
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/trees/damage_tracker_unittest.cc
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/bb1af5a4c94ddeab5c3984303ef22d31f7881b4f/cc/trees/tree_synchronizer_unittest.cc

Comment 8 by wutao@chromium.org, Aug 18 2017

Status: Fixed (was: Available)

Sign in to add a comment