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

Issue 753224 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 756693
issue 756695
issue 756696
issue 764994



Sign in to add a comment

Cache render surface in animations in CrOS.

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

Issue description

Create this master issue to add cache render surface in various animations in CrOS.


 

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

Cc: abodenha@chromium.org ajuma@chromium.org reve...@chromium.org
To use the cache render surface in animation, we need to layer->SetCacheRenderSurface(true) before animation, and layer->SetCacheRenderSurface(false) after animation.

One method we can add animation observer to reset the flag after animation is finished. But this requires adding observer for each animations, such as cross fade, hide/show animations.

Is there a way we can add this code once and reset the flag to old values when animation finished.
Tried on ScopedLayerAnimationSettings, in the dtor, SetCacheRenderSurface(old_value). But this does not work because ScopedLayerAnimationSettings is out of scope before animation finishes.

+reveman@ and ajuma@ for suggestions.

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

Cc: osh...@chromium.org
Maybe we should integrate this with ui::LayerAnimator?

Some background; We don't want to force render surface caching for all UI animations but for a large set we'd like to use this. Anything that animates opacity typically wants this but there are cases where we animate opacity and should avoid it (one layer in the animation) and there are cases where we're not animating opacity but would still like to enable this.

Comment 4 by ajuma@chromium.org, Aug 8 2017

Cc: vollick@chromium.org flackr@chromium.org
Integrating with ui::LayerAnimator sounds like a good approach. Given that the decision to enable/disable isn't strictly a function of the animation type, is this something that would need to be added as a flag on LayerAnimationSequence?

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

LayerAnimationSequence sounds good.


Looks like we have the following changes:
1. Because the SetCacheRenderSurface flag is set on ui::Layer, we need to modify the LayerAnimationDelegate to have SetCacheRenderSurfaceFromAnimation(bool).


2. Add a flag needs_cache_render_surface in LayerAnimationSequence, this flag can be set from ScopedLayerAnimationSettings or LayerAnimationSequence, similar to LayerAnimationSequence::SetAnimationMetricsReporter [1].
[1] https://cs.chromium.org/chromium/src/ui/compositor/layer_animation_sequence.cc?l=247&rcl=299a73a1f9756946d1df44ce03aa281647c9fc56


3. In LayerAnimationSequence, modify OnStart, OnProgress, ProgressToEnd, OnAbort to set the layer cache flag accordingly to new and old cache flag values.



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

Seems the plan in #3 and #5 may not work.

One layer (animator) might have multiple sequences, when one sequence start/finish, it will change the cc_layer_->cache_render_surface flag. This will affect how other sequences to get and set this flag.

So this flag still need to stay at LayerAnimator or ui::Layer level, and only be cleared after all the animating sequences finished, which can only be done by adding an observer.

I will try to add this internal observer for this flag clear up.

Comment 7 by wutao@chromium.org, Aug 10 2017

I am considering to add an internal observer to ScopedLayerAnimationSettings, and a function: ScopedLayerAnimationSettings::CacheRenderSurfaceForLayer(Layer* layer).

void ScopedLayerAnimationSettings::CacheRenderSurfaceForLayer(
    ui::Layer* layer) {
  AddObserver(new CleanupCacheRenderSurfaceObserver(layer));
}

In the observer ctor, layer->SetCacheRenderSurface(true).
in the observer dtor, layer->SetCacheRenderSurface(old_true).

Will upload a cl soon.

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

Blockedon: 756693

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

Blockedon: 756695

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

Blockedon: 756696
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 18 2017

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

commit 52217d1a68d9a011960d8f901db0bf19592b6bf1
Author: wutao <wutao@chromium.org>
Date: Fri Aug 18 02:40:31 2017

Add CacheRenderSurfaceObserver to ScopedLayerAnimationSettings.

We want to force render surface caching for a large set of animations in
CrOS. We would like to set the cache flag on the animating layer and
unset the flag upon animation finish. Adding an observer to achieve this.

Changes:
1. A counter in ui::Layer to maintain the number of requests for render
surface caching.
2. New observer in ScopedLayerAnimationSettings observing both layer
destroyed and animation completion.
3. In the new observer increase the counter in the ctor and decrease the 
request counter when called.

using this new settings.

Bug:  753224 
Test: New LayerAnimator unittest and Checking cache in CrossFade animation
Change-Id: I08f1078acbd2e3fb40a90ad3397caef8ed1ebeba
Reviewed-on: https://chromium-review.googlesource.com/610939
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495437}
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ash/wallpaper/wallpaper_widget_controller.cc
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ash/wallpaper/wallpaper_widget_controller.h
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/layer.cc
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/layer.h
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/layer_animation_delegate.h
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/layer_animator_unittest.cc
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/layer_observer.h
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/layer_unittest.cc
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/scoped_layer_animation_settings.cc
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/scoped_layer_animation_settings.h
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/test/test_layer_animation_delegate.cc
[modify] https://crrev.com/52217d1a68d9a011960d8f901db0bf19592b6bf1/ui/compositor/test/test_layer_animation_delegate.h

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 18 2017

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

commit 265bdd924659ecc28f1dac34653f38ce1d2fd082
Author: wutao <wutao@chromium.org>
Date: Fri Aug 18 17:10:40 2017

CrOS: Cache render surface in cross fade animation.

Cache render surface in animation can improve the UI performance
significantly. We have built up fundamental blocks already. In this
cl, we are caching the render surface in cross fade animation.

Performance improvement:
Linux build: ~50%
LINK: ~20%
KEVIN: ~15%

Bug:  756695 ,  753224 
Test: Tested improvement with/out cache.
Change-Id: Iad42f124b4e77c3c171bd0d59ea7b6f2831ae7f3
Reviewed-on: https://chromium-review.googlesource.com/619931
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495591}
[modify] https://crrev.com/265bdd924659ecc28f1dac34653f38ce1d2fd082/ash/wm/window_animations.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Sep 7 2017

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

commit fb6613a035e615799d30f141cc4d140dc06d134f
Author: wutao <wutao@chromium.org>
Date: Thu Sep 07 21:43:36 2017

CrOS: Cache render surface in hide animation.

Cache render surface in animation can improve the UI performance
significantly. We have built up fundamental blocks already. In this
cl, we are caching the render surface in hide animation.

Performance improvement:
Linux build: ~30%
KEVIN: ~15%

Bug:  756693 ,  753224 
Test: Tested improvement with/out cache.
Change-Id: I539a8407ae9742593a4a44de8f2542fb5f2630f9
Reviewed-on: https://chromium-review.googlesource.com/620132
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500398}
[modify] https://crrev.com/fb6613a035e615799d30f141cc4d140dc06d134f/ui/wm/core/window_animations.cc

Comment 15 by wutao@chromium.org, Sep 13 2017

Blockedon: 764994
Components: OS>Kernel>Graphics
Status: Assigned (was: Available)
Status: Fixed (was: Assigned)
We do not have plan to apply this into more animations for now.
Close this bug. And will create new one if needed.

Sign in to add a comment