1219543 - KeyframeEffectReadOnly::mIsPropertyRunningOnCompositor should be a member of AnimationPropery

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

It will be more convenient.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP v1

This is a WIP patch.
With this patch a mochitest (checking in MotationObserver callback) fails at https://dxr.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23/dom/animation/test/chrome/test_running_on_compositor.html#209
I am now investigationg the failure reason.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8685667 [details] [diff] [review]
WIP v1

@@ -782,16 +782,17 @@ nsAnimationManager::BuildAnimations(nsSt
           keyframesWithProperty.AppendElement(kfIdx);
         }
         lastKey = kf.mKey;
       }
 
       AnimationProperty &propData = *destEffect->Properties().AppendElement();
       propData.mProperty = prop;
       propData.mWinsInCascade = true;
+      propData.mIsRunningOnCompositor = false;

The reason of the test failure is here.
The isRunningOnCompositor flag is set to false whenever aniamtion style is changed. I am now thinking how we can avoid this.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Comment on attachment 8685667 [details] [diff] [review]
> WIP v1
> 
> @@ -782,16 +782,17 @@ nsAnimationManager::BuildAnimations(nsSt
>            keyframesWithProperty.AppendElement(kfIdx);
>          }
>          lastKey = kf.mKey;
>        }
>  
>        AnimationProperty &propData =
> *destEffect->Properties().AppendElement();
>        propData.mProperty = prop;
>        propData.mWinsInCascade = true;
> +      propData.mIsRunningOnCompositor = false;
> 
> The reason of the test failure is here.
> The isRunningOnCompositor flag is set to false whenever aniamtion style is
> changed. I am now thinking how we can avoid this.

In case of animations, to fix this problem properly we need to preserve the mIsRunningOnCompositor flag of the previous one.
In current implementation the flag is replaced at [1].
[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#494
This is highly related to part 3 patch (attachment 8684183 [details] [diff] [review]) for bug 1166500. As Brian suggested in bug 1166500 comment 51, SwapProperties might be better because after the swap we still have the old properties so we can update the new flag with the old one.

In case of transitions there is only one property so we can handle the case easier than the above animation case.

So, I would like to fix the animation case property in a future bug.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

A try result [1] showed me that the failure test in comment 1 causes intermittent.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=16e6dfb69339

In the MutationObserver callback,

  mIsRunningOnCompositor == false if the micro task for the MutationObserver is processed before building display list.
  mIsRunningOnCompositor == true if there is no room to perform the micro task before building display list.

So I've decided to deal with the failure case in this bug without attachment 8684183 [details] [diff] [review] for bug 1166500.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a57aae610390

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: isRunningOnCompositor flag is now a member of AnimationProperty.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Clear isRunningOnCompositor flag only if the target effect has the property.

Without this patch two assertions in KeyframeEffectReadOnly::SetIsRunningOnCompositor
will be hit.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list.

Without this fix, mIsRunningOnCompositor will be unpredictable in
MutationObserver callbacks.

For example:

 mIsRunningOnCompositor will be false if the micro task for
 the MutationObserver is processed before building display list.

 mIsRunningOnCompositor will be true if there is no room to process
 the micro task before building display list

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: MutationObserver should disconnect when the test is finished.

This part is not actually related to this issue though.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Use the current isRunningOnCompositor flag when the style is changed for transitions.

This patch fixes the case that the flag was false in MutationObserver callbacks
when the transition style is changed even if the transition property has been
already running on the compositor.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Part 1 and 2 are built upon patches for bug 1226118.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00196df18208&selectedJob=14602282

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8694021 [details] [diff] [review]
Part 1: isRunningOnCompositor flag is now a member of AnimationProperty.

Review of attachment 8694021 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should add initializers to AnimationProperty in case we forget to set them. Then we can probably drop the lines that set mWinsInCascade and mIsRunningOnCompositor from nsAnimationManager.cpp and nsTransitionManager.cpp.

e.g.

 nsCSSProperty mProperty = eCSSProperty_UNKNOWN;
 bool mWinsInCascade = true;
 bool mIsRunningOnCompositor = false;

r=birtles with comments addressed

::: dom/animation/KeyframeEffect.cpp
@@ +495,5 @@
>               "compositor-animatable property");
> +  for (AnimationProperty& property : mProperties) {
> +    if (property.mProperty == aProperty) {
> +      MOZ_ASSERT(property.mWinsInCascade,
> +                 "The property should win in the CSS cascade");

I think this should be:

MOZ_ASSERT(property.mWinsInCascade || !aIsRunning,
           "Only animations that win in the cascade should run on the compositor");

::: dom/animation/KeyframeEffect.h
@@ +150,4 @@
>    InfallibleTArray<AnimationPropertySegment> mSegments;
>  
> +  // NOTE: This operator does *neither* compare the mWinsInCascade *nor* the
> +  // mIsRunningOnCompositor members.

Nit: This might be slightly easier to read as:

// NOTE: This operator does *not* compare the mWinsInCascade member *or*
// the mIsRunningCompositor member.

@@ +154,5 @@
>    // This is because AnimationProperty objects are compared when recreating
>    // CSS animations to determine if mutation observer change records need to
>    // be created or not. However, at the point when these objects are compared
> +  // neither the mWinsInCascade nor the mIsRunningOnCompositor will have been
> +  // set on the new objects so we ignore this member to avoid generating

"...so we ignore these members..."

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8694022 [details] [diff] [review]
Part 2: Clear isRunningOnCompositor flag only if the target effect has the property.

I guess this patch is to avoid hitting the assertion added in part 1. As discussed we can probably just drop the assertion from part 1 and avoid iterating over all the properties twice here.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8694023 [details] [diff] [review]
Part 3: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list.

Review of attachment 8694023 [details] [diff] [review]:
-----------------------------------------------------------------

Are we sure that mIsRunningOnCompositor will be cleared in the case where, after restyling, the element is no longer layerized or animated?

Also, this algorithm is O(n^2) where n = the number of properties being animated.

Perhaps we could, for example, only fix this for the case where the @keyframes rule has not changed. Alternatively we might have to set up a little hashmap to avoid iterating over one of the lists of properties all the time.

Clearing review for now because even if this is correct, we should find a way of doing this that isn't O(n^2).

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8694026 [details] [diff] [review]
Part 5: Use the current isRunningOnCompositor flag when the style is changed for transitions.

Review of attachment 8694026 [details] [diff] [review]:
-----------------------------------------------------------------

The test looks fine but I'm not really sure if this is correct. It seems odd to transfer this flag over to a new transition.

::: layout/style/nsTransitionManager.cpp
@@ +666,5 @@
>  
> +  bool isCurrentlyRunningOnCompositor = false;
> +  if (haveCurrentTransition &&
> +      aProperty == oldPT->Properties()[0].mProperty &&
> +      aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect()) {

We don't need to check aProperty == oldPT->Properties()[0].mProperty. We only set haveCurrentTransition when that is true. (And in the one case where we later clear oldPT, we do an early return before we get to this line.)

Also, we shouldn't need to check aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect() since we should only set mIsRunningOnCompositor when that is true.

However, I wonder if this is really correct. In this case we're generating a new transition, not updating an existing one. For example, if we reverse an existing transition should we really mark the new transition as already running on the compositor?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #16)
> Comment on attachment 8694026 [details] [diff] [review]
> Part 5: Use the current isRunningOnCompositor flag when the style is changed
> for transitions.
> 
> Review of attachment 8694026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The test looks fine but I'm not really sure if this is correct. It seems odd
> to transfer this flag over to a new transition.
> 
> ::: layout/style/nsTransitionManager.cpp
> @@ +666,5 @@
> >  
> > +  bool isCurrentlyRunningOnCompositor = false;
> > +  if (haveCurrentTransition &&
> > +      aProperty == oldPT->Properties()[0].mProperty &&
> > +      aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect()) {
> 
> We don't need to check aProperty == oldPT->Properties()[0].mProperty. We
> only set haveCurrentTransition when that is true. (And in the one case where
> we later clear oldPT, we do an early return before we get to this line.)
> 
> Also, we shouldn't need to check
> aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect() since we
> should only set mIsRunningOnCompositor when that is true.
> 
> However, I wonder if this is really correct. In this case we're generating a
> new transition, not updating an existing one. For example, if we reverse an
> existing transition should we really mark the new transition as already
> running on the compositor?

Though I don't remember exactly why I changed opacity property in the test case, I guess I could not MutationObserver callback when I changed tranditionDuration as well as animationDuration.
After discussion with Brian, I now understand changing transitionDuration does not cause MutationObserver callbacks. So, this patch is not necessary at all.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list v2

Previously this was actually part 3.
To avoid the O(n^2) cost this patch splits nested loops into two individual loop.

(In reply to Brian Birtles (:birtles) from comment #15)
> Comment on attachment 8694023 [details] [diff] [review]
> Part 3: Avoid the period that mIsRunningOnCompositor is false between
> restyling and building display list.
> 
> Review of attachment 8694023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are we sure that mIsRunningOnCompositor will be cleared in the case where,
> after restyling, the element is no longer layerized or animated?

I am not 100% sure. But this patch preserves previous behavior. I.e., without part 1 patch, the flag is not cleared there because the flag is not included in KeyframeEffectReadOnly::Properties().

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8699353 [details] [diff] [review]
Part 2: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list v2

Review of attachment 8699353 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with comments addressed.

::: layout/style/nsAnimationManager.cpp
@@ +388,5 @@
>  
> +void
> +nsAnimationManager::CopyIsRunningOnCompositor(
> +  KeyframeEffectReadOnly* aSourceEffect,
> +  KeyframeEffectReadOnly* aDestEffect)

Make these references and make aSourceEffect a const ref?

I wonder if we should even just pass in references to the properties arrays? It's up to you.

@@ +390,5 @@
> +nsAnimationManager::CopyIsRunningOnCompositor(
> +  KeyframeEffectReadOnly* aSourceEffect,
> +  KeyframeEffectReadOnly* aDestEffect)
> +{
> +  nsTHashtable<nsGenericHashKey<nsCSSProperty>> sourceProperties;

As discussed, we can probably use nsCSSPropertySet here.

@@ +517,5 @@
> +          // To preserve old mIsRunningOnCompositor we need to replace the new
> +          // one with the old one if the old one exists before all the other
> +          // properties are replaced. Without it mIsRunningOnCompositor stays
> +          // false until building display list is processed even if the
> +          // animation property is actually running on the compositor.

"To preserve the mIsRunningOnCompositor value on each property, we copy it from the old effect to the new effect since, in the following step, we will completely clobber the properties on the old effect with the values on the new effect."

?

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: isRunningOnCompositor flag is now a member of AnimationProperty v2

Addressed review comments and removed assertions as a result of discussion with Brian.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Avoid the period that mIsRunningOnCompositor is false between restyling and building display list v3

Addressed comments.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: MutationObserver should disconnect when the test is finished.

Changed the part number.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c27ce307fe

Thanks!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1254ce863bc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea91eabfc250
https://hg.mozilla.org/integration/mozilla-inbound/rev/d75875a50429

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1254ce863bc3
https://hg.mozilla.org/mozilla-central/rev/ea91eabfc250
https://hg.mozilla.org/mozilla-central/rev/d75875a50429