Closed Bug 1341987 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(5 files)

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?
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.
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.
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: nobody → hikezoe
Status: NEW → ASSIGNED
Attached file 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?
Ah, right.  This way spoiled offscreen throttling for transform.
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?
(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.
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.
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.
(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 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 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+
(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 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 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+
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
You need to log in before you can comment on or make changes to this bug.