Closed
Bug 1341987
Opened 8 years ago
Closed 8 years ago
stylo: Don't update animation rule refresh time when we compose styles
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
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?
Assignee | ||
Comment 1•8 years 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•8 years 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•8 years 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•8 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
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•8 years ago
|
||
Ah, right. This way spoiled offscreen throttling for transform.
Assignee | ||
Updated•8 years ago
|
Attachment #8840391 -
Flags: review?(bbirtles)
Attachment #8840392 -
Flags: review?(bbirtles)
Attachment #8840393 -
Flags: review?(bbirtles)
Attachment #8840394 -
Flags: review?(bbirtles)
Comment 10•8 years ago
|
||
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•8 years 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•8 years 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•8 years ago
|
Attachment #8840391 -
Flags: review?(bbirtles)
Attachment #8840392 -
Flags: review?(bbirtles)
Attachment #8840393 -
Flags: review?(bbirtles)
Attachment #8840394 -
Flags: review?(bbirtles)
Comment 13•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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
Closed: 8 years 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.
Description
•