stylo: We don't update direction or play-state immediately in test_animations_omta.html

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
24 days ago
3 hours ago

People

(Reporter: boris, Assigned: hiro)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

24 days ago
There are some test failures in test_animations_omta.html [1] while applying a new animation direction or animation play state. I dumped the log and it seems we didn't call AddAnimationsAndTransitionsToLayer() for these cases, so the animation-direction or animation-play-state on the compositor thread wasn't updated to the new value.

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta.html#1008-1011
(Reporter)

Updated

24 days ago
Priority: -- → P2
(Assignee)

Comment 1

24 days ago
One thing I am concerned is that we really flush styles by getBoundingClientRect() [1] on stylo when we are in test refresh mode.

[1] https://hg.mozilla.org/mozilla-central/file/4a6a71f4aa22/testing/mochitest/tests/SimpleTest/paint_listener.js#l53
(Reporter)

Updated

23 days ago
Blocks: 1321197
(Assignee)

Comment 2

7 days ago
Does this still fail?
(Assignee)

Updated

5 days ago
Blocks: 1366657
(Assignee)

Comment 3

5 days ago
Bumping up priority since this currently will cause mismatching of failure numbers for test_animations_omta.html.

Boris, I think this is more important than bug 1361663. As for bug 1361663, I think we can just tune tolerance value for now.
Priority: P2 → P1
(Reporter)

Comment 4

5 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Bumping up priority since this currently will cause mismatching of failure
> numbers for test_animations_omta.html.
> 
> Boris, I think this is more important than bug 1361663. As for bug 1361663,
> I think we can just tune tolerance value for now.

OK, I see. I still don't know the root cause of this bug. Need more time to debug it.
(Reporter)

Comment 5

5 days ago
In the tests of animation-direction, it seems this bug only happens when "we advance time and change the direction together" [1] because we pass incorrect direction into compositor (e.g. still pass alternative-reverse, after we change the direction to normal).

[1] http://searchfox.org/mozilla-central/source/layout/style/test/test_animations_omta.html#1020-1021,1037-1038,1054-1055,1071-1072,1088-1089
(Assignee)

Comment 6

5 days ago
(In reply to Boris Chiou [:boris] from comment #5)
> In the tests of animation-direction, it seems this bug only happens when "we
> advance time and change the direction together" [1] because we pass
> incorrect direction into compositor (e.g. still pass alternative-reverse,
> after we change the direction to normal).

Nice find! So, we are re-sending the target animation to compositor when we call waitForPaintsFlushed() without updating the animation style?
(Reporter)

Comment 7

5 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Boris Chiou [:boris] from comment #5)
> > In the tests of animation-direction, it seems this bug only happens when "we
> > advance time and change the direction together" [1] because we pass
> > incorrect direction into compositor (e.g. still pass alternative-reverse,
> > after we change the direction to normal).
> 
> Nice find! So, we are re-sending the target animation to compositor when we
> call waitForPaintsFlushed() without updating the animation style?

Yes. Looks like we get the old compositorAnimations from EffectCompositor::GetAnimationsForCompositor() in AddAnimationsAndTransitionsToLayer() at these moments. Need to check what happened.
(Reporter)

Comment 8

5 days ago
(In reply to Boris Chiou [:boris] from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Boris Chiou [:boris] from comment #5)
> > > In the tests of animation-direction, it seems this bug only happens when "we
> > > advance time and change the direction together" [1] because we pass
> > > incorrect direction into compositor (e.g. still pass alternative-reverse,
> > > after we change the direction to normal).
> > 
> > Nice find! So, we are re-sending the target animation to compositor when we
> > call waitForPaintsFlushed() without updating the animation style?
> 
> Yes. Looks like we get the old compositorAnimations from
> EffectCompositor::GetAnimationsForCompositor() in
> AddAnimationsAndTransitionsToLayer() at these moments. Need to check what
> happened.

At these moments, not only main thread but also compositor thread are using the "old direction". However, we indeed set the correct direction on normal restyle.
(Reporter)

Comment 9

5 days ago
Looks like we didn't call UpdateOldAnimationPropertiesWithNew() after changing the direction to "Normal" in the normal restyle. Why does this only happen after we advance the clock?
Comment hidden (typo)
(Reporter)

Comment 11

5 days ago
OK. Let me explain the possible root cause:

After advancing 2000ms and change animationDirection to "Normal", we trigger an animation restyle, which includes:
a. doing interpolation
b. setting the new animation direction  (Is this a bug?),
   so the animation direction becomes "Normal", from "Alternative-Reverse".

After this animation-only restyle, we trigger a normal restyle, which including:
c. setting the new animation direction, and we notice that there is no change because "Normal" == "Normal",
   so we don't update css animation properties.

That is why we didn't update animation direction value in KeyframeEffect at the moment.

e.g.
For animation 10s linear, opacity [0, 1]:

1) When changing the animation direction to "Alternate_reverse" without advancing 2000ms:

GECKO(78969) | ----- normal traversal ------
GECKO(78969) | set_animation_direction: Alternate_reverse
GECKO(78969) | call UpdateOldAnimationPropertiesWithNew
GECKO(78969) | ----- normal traversal finish ------

GECKO(78969) | ----- animation-only traversal ------
GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 1 => Opacity(1)
GECKO(78969) | set_animation_direction: Alternate_reverse
GECKO(78969) | ----- animation-only traversal finish ------
GECKO(78969) | ----- normal traversal ------
GECKO(78969) | ----- normal traversal finish ------


2) When changing the animation direction to "Normal" after advancing 2000ms:

GECKO(78969) | ==== advance 2000 and change to "Normal" ====
GECKO(78969) | ----- animation-only traversal ------
GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)   // we still use Alternate_reverse
GECKO(78969) | set_animation_direction: Normal
GECKO(78969) | ----- animation-only traversal finish ------
GECKO(78969) | ----- normal traversal ------
GECKO(78969) | set_animation_direction: Normal       // This bug. In normal traversal, we thought there is no change, so
                                                     // didn't call UpdateOldAnimationPropertiesWithNew()
GECKO(78969) | ----- normal traversal finish ------

GECKO(78969) | ----- animation-only traversal ------
GECKO(78969) | specified timing direction: 3   // still Alternative-Reverse
GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)
GECKO(78969) | set_animation_direction: Normal
GECKO(78969) | ----- animation-only traversal finish ------
GECKO(78969) | ----- normal traversal ------
GECKO(78969) | ----- normal traversal finish ------


Why is there an animation-only restyle after advancing 2000ms? When we change the style, we only trigger a normal restyle
in other cases. This animation-only restyle happens only after advancing 2000ms. (And looks like it is not a pending play/pause animation?)
(Reporter)

Comment 12

5 days ago
(In reply to Boris Chiou [:boris] from comment #11)
> 2) When changing the animation direction to "Normal" after advancing 2000ms:
> 
> GECKO(78969) | ==== advance 2000 and change to "Normal" ====
> GECKO(78969) | ----- animation-only traversal ------
> GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)  
> // we still use Alternate_reverse
> GECKO(78969) | set_animation_direction: Normal
> GECKO(78969) | ----- animation-only traversal finish ------
> GECKO(78969) | ----- normal traversal ------
> GECKO(78969) | set_animation_direction: Normal       // This bug. In normal traversal, we thought there is no change, so
>                                                      // didn't call UpdateOldAnimationPropertiesWithNew()
> GECKO(78969) | ----- normal traversal finish ------

Hiro, is the updating of direction/playback-state by set_animation_${ident} [1] in animation-only traversal expected?

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/gecko.mako.rs#1967
(Reporter)

Updated

5 days ago
Flags: needinfo?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #11)
> OK. Let me explain the possible root cause:
> 
> After advancing 2000ms and change animationDirection to "Normal", we trigger
> an animation restyle, which includes:
> a. doing interpolation
> b. setting the new animation direction  (Is this a bug?),

When you say "setting the new animation direction", do you mean on the CSSAnimation object? I guess not, right?
(Assignee)

Comment 14

5 days ago
(In reply to Boris Chiou [:boris] from comment #12)
> (In reply to Boris Chiou [:boris] from comment #11)
> > 2) When changing the animation direction to "Normal" after advancing 2000ms:
> > 
> > GECKO(78969) | ==== advance 2000 and change to "Normal" ====
> > GECKO(78969) | ----- animation-only traversal ------
> > GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)  
> > // we still use Alternate_reverse
> > GECKO(78969) | set_animation_direction: Normal
> > GECKO(78969) | ----- animation-only traversal finish ------
> > GECKO(78969) | ----- normal traversal ------
> > GECKO(78969) | set_animation_direction: Normal       // This bug. In normal traversal, we thought there is no change, so
> >                                                      // didn't call UpdateOldAnimationPropertiesWithNew()
> > GECKO(78969) | ----- normal traversal finish ------
> 
> Hiro, is the updating of direction/playback-state by set_animation_${ident}
> [1] in animation-only traversal expected?

Definitely not!  Leaving NI? since this seems a bug in animation-only restyle.
(Reporter)

Comment 15

5 days ago
(In reply to Brian Birtles (:birtles) from comment #13)
> (In reply to Boris Chiou [:boris] from comment #11)
> > OK. Let me explain the possible root cause:
> > 
> > After advancing 2000ms and change animationDirection to "Normal", we trigger
> > an animation restyle, which includes:
> > a. doing interpolation
> > b. setting the new animation direction  (Is this a bug?),
> 
> When you say "setting the new animation direction", do you mean on the
> CSSAnimation object? I guess not, right?

Sorry, I mean set_animation_{$ident}() function call [1] (i.e. gecko style struct).

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/gecko.mako.rs#1967
(Reporter)

Comment 16

5 days ago
(In reply to Boris Chiou [:boris] from comment #15)
> > When you say "setting the new animation direction", do you mean on the
> > CSSAnimation object? I guess not, right?
> 
> Sorry, I mean set_animation_{$ident}() function call [1] (i.e. gecko style
> struct).
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/
> gecko.mako.rs#1967

Not sure how many values have this problem. All I know is: 'Direction', 'FillMode', 'PlayState' [2].

[2] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/gecko.mako.rs#2511-2516
(Reporter)

Comment 17

5 days ago
(In reply to Boris Chiou [:boris] from comment #16)
> Not sure how many values have this problem. All I know is: 'Direction',
> 'FillMode', 'PlayState' [2].
> 
> [2]
> http://searchfox.org/mozilla-central/rev/
> 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/
> gecko.mako.rs#2511-2516

In test_animations_omta.html, we use a different way to test fill mode, and it seems we don't combine the style change and advance clock to test fill mode, so now it looks good. Anyway, I think if there is any, after we fix the bug of direction and play-state, fill mode would be also fixed.
(Assignee)

Comment 18

4 days ago
(In reply to Boris Chiou [:boris] from comment #12)
> Hiro, is the updating of direction/playback-state by set_animation_${ident}
> [1] in animation-only traversal expected?

The setter was called in cascading process. What I don't still understand is why the new value appeared there.
I will investigate it further.

Filed a couple of issues in animation-only restyle I found as another bug (bug 1367225).
Assignee: nobody → hikezoe
Flags: needinfo?(hikezoe)
(Assignee)

Comment 19

4 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Boris Chiou [:boris] from comment #12)
> > Hiro, is the updating of direction/playback-state by set_animation_${ident}
> > [1] in animation-only traversal expected?
> 
> The setter was called in cascading process. What I don't still understand is
> why the new value appeared there.
> I will investigate it further.

Just a quick update.
The new value has been already set in primary rules before we replace animation rules in replace_rules(). So, where?
(Assignee)

Comment 20

2 days ago
I had been totally blinding about the fact that a PropertyDeclarationBlock is stored in StyleSource which is a member of RuleNode.
In my understandings, when we call setProperty (e.g. div.style.animationDirection = 'reverse'), the property value is stored in the PropetyDeclarationBlock in the StyleSource, and after that, it is set to gecko's style struct in the cascading process.  So if we have both of RESTYLE_STYLE_ATTRIBUTE and RESTYLE_CSS_ANIMATIONS hints, the PropertyDeclarationBlock which was set by setProperty() is also set to the gecko's struct during animation-only restyle unfortunately.

The only solution what I can think of is to skip the cascading process that if we have both of restyle hints, that means the cascading process is deferred to the time when we process RESTYLE_STYLE_ATTRIBUTE in normal traversal.

Here is a try with the solution:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af12659b21fc3ea1fbf67b9199894d6cfb1657b0

It seems to work well.
(Assignee)

Comment 21

2 days ago
Oops, I forgot to add Cameron in CC.
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 days ago
mozreview-review
Comment on attachment 8871475 [details]
Bug 1361938 - Do not cascade when we have both of RESTYLE_STYLE_ATTRIBUTE and animation RestyleReplacements.

https://reviewboard.mozilla.org/r/142936/#review146654

::: layout/style/test/mochitest.ini:62
(Diff revision 1)
>  [test_animations_omta.html]
> -skip-if = stylo # crashing bug 1367670, bug 1361938, bug 1361663

I will drop this change once bug 1367670 is fixed.
(Assignee)

Comment 24

2 days ago
mozreview-review
Comment on attachment 8871475 [details]
Bug 1361938 - Do not cascade when we have both of RESTYLE_STYLE_ATTRIBUTE and animation RestyleReplacements.

https://reviewboard.mozilla.org/r/142936/#review146718

Clearing review request since some tests failed on a try based on today's autoland.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bc7537770d49db5df59767585e858c6e1044d4a
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

21 hours ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=583539063c838efcbd53eb62866e49684e399a63
(Reporter)

Comment 29

3 hours ago
mozreview-review
Comment on attachment 8872032 [details]
Bug 1361938 - Disable test_transitions_replacement_on_busy_frame.html.

https://reviewboard.mozilla.org/r/143554/#review147334

::: commit-message-44873:8
(Diff revision 1)
> +This test had been passed accidentaly.
> +In the test initially a transition (transform: none to translateX(300px)) is
> +created.  After the transition was sent to the compositor, secondary transition
> +tries to be created, the after-change style of the secondary transitions is
> +translateX(0px).  As per the transition spec, the before-change style of the
> +secondary should be translateX(300px), but on current implementation it's

I thought the before-change style should be the previous style change event, *except with* any styles derived from declarative animations such as CSS Transitions, CSS Animations, and SMIL Animations updated to the current time. In other words, it should be the current computed values including up-to-date animation rules?

If this test is failed on non-e10s, it's ok to skip it. Thanks for checking this.
Attachment #8872032 - Flags: review?(boris.chiou) → review+
You need to log in before you can comment on or make changes to this bug.