stylo: Don't update animation rule refresh time when we compose styles

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Animation
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

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

Attachments

(5 attachments)

(Assignee)

Description

10 months ago
I just realized while I am filing this, we can set the time when we send transform animations to the compositor. Because the time is only for transform animations that are running on the compositor.

So, we can set the time just before we call SetIsRunningOnCompositor in AddAnimationsForProperty().

Brian, is this correct?
(Assignee)

Comment 1

10 months ago
Is there a case that refresh driver's most recent time on composing style is different from the time on building display list?
If there is a case, I think it's a bug.
(Assignee)

Comment 2

10 months ago
A try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43edfe78faf202c58092fdc7fee567babf807e9d

I know this try is not quite useful because we have only one test case for this case. But yeah, not nothing. Thanks the test!

[1] https://hg.mozilla.org/mozilla-central/file/32dcdde1fc64/dom/animation/test/chrome/test_running_on_compositor.html#l238

I am also inclined to drop cascade level from the animation rule refresh time because when we need to update a transform on an element for transition level, we end up updating another transform on the same element for animation level too.
(Assignee)

Comment 3

10 months ago
The try syntax was for stylo, so tests on most platforms did not run at all.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=87754589d42dfb5f8bb3d5fa2a4214d7fcb7145f
(Assignee)

Updated

10 months ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Created attachment 8840662 [details]
Offscreen test case

I was a bit concerned that this approach would mean we fail to update scrollbars for an offscreen animation but it looks like we already have a bug there. I guess this will make that bug harder to fix? What do you think?
(Assignee)

Comment 9

10 months ago
Ah, right.  This way spoiled offscreen throttling for transform.
(Assignee)

Updated

10 months ago
Attachment #8840391 - Flags: review?(bbirtles)
Attachment #8840392 - Flags: review?(bbirtles)
Attachment #8840393 - Flags: review?(bbirtles)
Attachment #8840394 - Flags: review?(bbirtles)
Actually I think offscreen throttling for transform is already not working quite right? On Windows, when the animation is offscreen I suppose the scrollbar should still update every 200ms? But it doesn't seem to?

Maybe that's ok? Maybe we should be adjusting the scrollbar for offscreen animations? It would probably limit what power-saving things we can do in future if we need to make that work and it seems of questionable value to the user?
(Assignee)

Comment 11

10 months ago
(In reply to Brian Birtles (:birtles) from comment #10)
> Actually I think offscreen throttling for transform is already not working
> quite right? On Windows, when the animation is offscreen I suppose the
> scrollbar should still update every 200ms? But it doesn't seem to?

Ah, you are right. Is seems already to be broken.  I did check the example with my patches here, and then I thought it's caused by the patches, but now I did check without the patches, yeah, it's broken...  The test case did not catch this regression.  I should have written another test case in test_restyles.html.

As for this bug, I will re-check offscreen case closely. I might be wrong, it might not be related to this bug because offscreen check is done earlier than transform unthrottling.
(Assignee)

Comment 12

10 months ago
Yay!  I was wrong.  The test case causes nsChangeHint_UpdatePostTransformOverflow, it's not throttled at all if the element is off-screen because the UpdatePostTransformOverflow is not the case of offscreen throttling.  https://hg.mozilla.org/mozilla-central/file/7d50c8e3086d/layout/base/nsChangeHint.h#l384

Yes, unfortunately (luckily for this bug) the offscreen transform animation (which causes frame overflow) runs on the main-thread, so the refresh time is not related at all.
(Assignee)

Updated

10 months ago
Attachment #8840391 - Flags: review?(bbirtles)
Attachment #8840392 - Flags: review?(bbirtles)
Attachment #8840393 - Flags: review?(bbirtles)
Attachment #8840394 - Flags: review?(bbirtles)
I think this is better, but do you think we need to do this? The reasons I'm not sure are:

1) There are a lot of other bits of internal state we need to work out how to update outside of the parallel
   traversal.

   Currently, I'm wondering if we can:

   a) Add a pre-traversal step for Servo, EffectCompositor::WillComposeAnimations, which traverses all the
      elements in mElementsToRestyle that have posted a restyle and calls 'WillComposeStyle' so they can
      update their internal state.

   b) Simply not cache ServoAnimationRules and generate them each time (but probably cache the
      mProgressOnLastCompose in (a) and use that at least).

   Supposing something like that works, it seems like it would cover this too?

2) Long-term, I think we're probably doing this kind of "scroll-bar update" as a document-level thing. That
   would avoid any odd effects if different transform animations do their "scrollbar update" at different
   times and mean we just do less animation flushes overall. If we did that then I think we wouldn't need this
   either.

That said, this still seems like an improvement so I'll go ahead and review it anyway, but I wonder what you
think about the above.
(Assignee)

Comment 14

10 months ago
(In reply to Brian Birtles (:birtles) from comment #13)
> I think this is better, but do you think we need to do this? The reasons I'm
> not sure are:
> 
> 1) There are a lot of other bits of internal state we need to work out how
> to update outside of the parallel
>    traversal.
> 
>    Currently, I'm wondering if we can:
> 
>    a) Add a pre-traversal step for Servo,
> EffectCompositor::WillComposeAnimations, which traverses all the
>       elements in mElementsToRestyle that have posted a restyle and calls
> 'WillComposeStyle' so they can
>       update their internal state.
> 
>    b) Simply not cache ServoAnimationRules and generate them each time (but
> probably cache the
>       mProgressOnLastCompose in (a) and use that at least).
> 
>    Supposing something like that works, it seems like it would cover this
> too?

Yes, a) definitely covers this issue. But even so, I prefer to do update the time right before we send the transform animation to the compositor because in the WillComposeAnimations there are some cases that we don't actually need to update the time since we will know that the target transform animation cannot be run on the compositor later.

> 2) Long-term, I think we're probably doing this kind of "scroll-bar update"
> as a document-level thing. That
>    would avoid any odd effects if different transform animations do their
> "scrollbar update" at different
>    times and mean we just do less animation flushes overall. If we did that
> then I think we wouldn't need this
>    either.

I am not sure about this, but that will be really fantastic if we threw this hack away!

Comment 15

10 months ago
mozreview-review
Comment on attachment 8840391 [details]
Bug 1341987 - Part 1: Rename AnimationRuleRefreshTime to LastTransformSyncTime.

https://reviewboard.mozilla.org/r/114910/#review117028

If we only update this when we actually synchronize with the compositor, should it be called mLastTransformSyncTime ? mLastTransformUpdateTime ?

::: dom/animation/EffectSet.h:217
(Diff revision 1)
> -  // A parallel array to mAnimationRule that records the refresh driver
> -  // timestamp when the rule was last updated. This is used for certain
> -  // animations which are updated only periodically (e.g. transform animations
> -  // running on the compositor that affect the scrollable overflow region).
> +  // Timestamp when transform animations in the effect set was last updated on
> +  // the main-thread. This is used for the transform animations which need to be
> +  // updated only periodically (e.g. the transform animations running on the
> +  // compositor that affect the scrollable overflow region).

"Refresh driver timestamp from the moment when transform animations in this effect set were last updated and sent to the compositor. This is used for transform animations that run on the compositor but need to be updated on the main thread periodically (e.g. so scrollbars can be updated)."
Attachment #8840391 - Flags: review?(bbirtles) → review+

Comment 16

10 months ago
mozreview-review
Comment on attachment 8840392 [details]
Bug 1341987 - Part 2: Drop cascade level from last refresh time for transform.

https://reviewboard.mozilla.org/r/114912/#review117030
Attachment #8840392 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 17

10 months ago
(In reply to Brian Birtles (:birtles) from comment #15)
> Comment on attachment 8840391 [details]
> Bug 1341987 - Part 1: Rename AnimationRuleRefreshTime to
> LastRefreshTimeForTransform.
> 
> https://reviewboard.mozilla.org/r/114910/#review117028
> 
> If we only update this when we actually synchronize with the compositor,
> should it be called mLastTransformSyncTime ? mLastTransformUpdateTime ?

Thanks! mLastTransformSyncTime sounds a pretty fit name for this!

Comment 18

10 months ago
mozreview-review
Comment on attachment 8840393 [details]
Bug 1341987 - Part 3: Update the last refresh time for transform only when we send transform animations to the compositor.

https://reviewboard.mozilla.org/r/114914/#review117032

::: dom/animation/EffectCompositor.cpp:376
(Diff revision 1)
>    if (!mPresContext || !elementsToRestyle.Contains(key)) {
>      return;
>    }
>  

Can we drop the check for "!mPresContext" here?

::: layout/painting/nsDisplayList.cpp:600
(Diff revision 1)
>               "inconsistent property flags");
>  
> -  DebugOnly<EffectSet*> effects = EffectSet::GetEffectSet(aFrame);
> +  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
>    MOZ_ASSERT(effects);
>  
> +  bool wasSent = false;

Call this sentAnimation? Or didSendAnimation?
Attachment #8840393 - Flags: review?(bbirtles) → review+

Comment 19

10 months ago
mozreview-review
Comment on attachment 8840394 [details]
Bug 1341987 - Part 4: Use nsIFrame's nsPresContext instead of getting from element.

https://reviewboard.mozilla.org/r/114916/#review117036
Attachment #8840394 - Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

10 months ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a83f222b420
Part 1: Rename AnimationRuleRefreshTime to LastTransformSyncTime. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b2d04e22fa33
Part 2: Drop cascade level from last refresh time for transform. r=birtles
https://hg.mozilla.org/integration/autoland/rev/81cfa489cb87
Part 3: Update the last refresh time for transform only when we send transform animations to the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/64b46c83b0cd
Part 4: Use nsIFrame's nsPresContext instead of getting from element. r=birtles

Comment 25

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a83f222b420
https://hg.mozilla.org/mozilla-central/rev/b2d04e22fa33
https://hg.mozilla.org/mozilla-central/rev/81cfa489cb87
https://hg.mozilla.org/mozilla-central/rev/64b46c83b0cd
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.