Opacity value != 1.0f is handled at here:
https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?l=829&rcl=cc477ff2aa3bb185b2b959a562a96a128dcaf403

So in the proposed cl, make APIs to determine if the animation is animating different values or not. Currently I only support this API to Float curve for opacity.
https://chromium-review.googlesource.com/c/chromium/src/+/663535

Animation opacity from 1.0 to 1.0 should not be considered an opacity animation imo. We might have other properties animating at the same time but opacity is not animating. It's clearly the same.

I might need change the description. It is not animating, it is has potential animation of opacity.
Has potential animation of opacity from 1.0 to 1.0 is valid from the layer_animator pov, but it will not cause damage.

The reasons are:
1. by changing any LayerAnimator unit test, e.g. CacheRenderSurface [1], to make the initial and target value the same, the animation still get created, started, and finished.

2. ui layer_animator does not know or check if the initial_value == target_value. It will create animation element, and effect_node when there is other changes requires property tree rebuild. At this time, HasPotentiallyRunningOpacityAnimation(layer) [2] will return true to create render surface. This cl is trying to change this API to return false if the initial_value == target_value.

3. However it will not change in the cc::Layer::SetOpacity, because there is a guard to prevent set to the same opacity value [3].

[1] https://cs.chromium.org/chromium/src/ui/compositor/layer_animator_unittest.cc?l=1699&rcl=a63a88f29ba7037e3b47bd7c10e88b76fd9e9958
[2] https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?l=830&rcl=57ee54827e51012631c50bb15647dad8c910f078
[3] https://cs.chromium.org/chromium/src/cc/layers/layer.cc?l=502&rcl=57ee54827e51012631c50bb15647dad8c910f078

Having the animation still run makes sense but it doesn't seem like IsPotentiallyAnimatingProperty should be returning true in these cases. If the target value is the same as the current value then should the animation of the property not be considered finished?

If we somehow still need IsPotentiallyAnimatingProperty to return true when values are the same then maybe a better approach is just to not allow UI to create animations to the same value and DCHECK that this is not done? IsPotentiallyAnimatingProperty not returning true is the preferred solution imo.

+ajuma@ who is the owner and will review the code eventually.

1. Not allow UI to create animations to the same value is the easiest. But this could break some devs when they set opacity to 1.0 (same value) and expect to delay calling other functions in the animation observer when opacity animation finishes? We cannot prevent devs doing this by current code, so it might break in some cases we do not know and those animation could be subtle and it is hard to notice.

2. We cannot simply set IsPotentiallyAnimatingProperty not returning true if target value is the same as the current value. Because if we return false, it will not create the effect_node [1]. But later when cc animation tick, if will expect the effect_node exists [2] and then fail.

Apparently there are some places setting opacity with the same value, because I get this DCHECK [2] failure on start of the chrome in desktop build.

3. That is why I implemented the cl in #1 to create a separate API to return true if the target value is the same as the current value. If the name of the API is confusing, we can come up a better one. How about FloatAnimationCurve::HasSameValue() or AreInitialAndTargetValuesSame()?


[1] https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?l=944&rcl=215ac0369f32bc5ced186a01b0f2ba007b09116c
[2] https://cs.chromium.org/chromium/src/cc/trees/layer_tree_impl.cc?l=598&rcl=a9b51b33da012d575b69c1d535c85dd68a76c8ad

+vollick@ for opinions. 

vollick@, ajuma@, any suggestions for how to handle this in an ideal way?

Just checking that the start and end values are the same isn't sufficient, since a keyframe animation could have intermediate keyframes with different values (e.g. imagine an animation that starts at opacity 0, fades in to opacity 1, and then fades back out to opacity 0). UI doesn't seem create this kind of keyframe animation in cc, but Blink does.

I'd suggest modifying ui::ThreadedLayerAnimationElement so that it doesn't send the animation to cc when the start and end values are the same. It already does this sort of special casing for 0-duration animations (see ThreadedLayerAnimationElement::RequestEffectiveStart and ThreadedLayerAnimationElement::IsThreaded). Try changing ThreadedLayerAnimationElement::IsThreaded so that it also compares start/end values. The advantage of this approach is that the ui animation framework will still produce the usual animation events that other code might be expecting.

Thanks ajuma@.
I am about to upload a cl soon based on your suggestion.

There is one thing left to unblock.

There are another places to use IsThreaded (IsFirstElementThreaded [1]). In ThreadedOpacityTransition, the start_ is only set after OnStart is called [2]. But IsThreaded called in Animator::StartTogether [3]. At this point, start_ is not inited (delegate->GetOpacityForAnimation()), but with default value of 0.0f. If I set target_ to 0.0f in this case, when I call AreStartAndTargetValuesTheSame (new API), it will return false, no matter current opacity is what value.


[1] https://cs.chromium.org/chromium/src/ui/compositor/layer_animation_sequence.cc?l=200&rcl=918b4f882ef09498b7649b5f7b673f71f115fbfe

[2] https://cs.chromium.org/chromium/src/ui/compositor/layer_animation_element.cc?l=384&rcl=c3119ad1a6b260ed81e74cd0f3308939ca102b21

[3] https://cs.chromium.org/chromium/src/ui/compositor/layer_animator.cc?l=274&rcl=c3119ad1a6b260ed81e74cd0f3308939ca102b21

That's tricky. One thing you could do is to make IsThreaded check Started(). When true, you can use the value of start_, but otherwise you need to use the value provided by the delegate as the value that you compare to target_.

#10, delegate is passed to functions in layer_animation_element/sequence.

So for the LayerAnimationSequence::IsFirstElementThreaded() case, maybe I can check element->Started() first before I call element->IsThreaded(). Seems there are multiple places to modify in layer_animator.cc.

But if I modify LayerAnimationSequence::IsFirstElementThreaded() to take the delegate as parameter, maybe make it easier:
LayerAnimationSequence::IsFirstElementThreaded(LayerAnimationDelegate* delegate)

Can I assume delegate is non-nullptr during the lifetime of layer_animator? It seems only set to nullptr in layer dtor.

Adding the delegate as a parameter to IsFirstElementThreaded sounds good. You can assume the delegate is non-null when we're trying to run animations. It's set/cleared in Layer::SetAnimator.

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

commit 914b06e605549afd88b2964edc96f87bd4797cbb
Author: wutao <wutao@chromium.org>
Date: Thu Sep 28 17:41:15 2017

Avoid creating render surface if animating same value opacity.

We should avoid creating render surface if we are animating opacity with
the same value of 1.0f. The opacity value not equal to 1.0f has already
been handled in the code. Therefore, in this cl, we modify the IsThreaded
to not create cc::Animation if it is animating with same start and target
values.

Bug:  764575 
Test: local with flag ui-show-composited-layer-borders=renderpass
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3b6044382d395c831e6e1abf627ead570d9715c1
Reviewed-on: https://chromium-review.googlesource.com/663535
Reviewed-by: Ali Juma <ajuma@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505081}
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animation_element.cc
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animation_element.h
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animation_element_unittest.cc
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animation_sequence.cc
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animation_sequence.h
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animator.cc
[modify] https://crrev.com/914b06e605549afd88b2964edc96f87bd4797cbb/ui/compositor/layer_animator_unittest.cc