Closed
Bug 1454324
Opened 6 years ago
Closed 6 years ago
Skip calculating animation value on the compositor if animation progress value hasn't been changed since the last composition
Categories
(Core :: DOM: Animation, enhancement)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla61
| Tracking | Status | |
|---|---|---|
| firefox61 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
(Depends on 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
We've been doing such optimizations for animations on the main-thread. There is no reason we don't do the optimizations on the compositor. Actually front-end people expected it (bug 1448133).
|
Assignee | |
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e47d38ba7f2715d4876a96e80378550ebf7387
|
|
Assignee | |
Comment 2•6 years ago
|
||
test_missing-keyframe-on-compositor.html failed in the try. There are something I am missing.
|
|
Assignee | |
Comment 3•6 years ago
|
||
I was missing 'missing-keyframe' case. I am going to optimize single animation case here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d411b83ae19eba17a65dd309880fadd6ff338a6
|
|
Assignee | |
Comment 4•6 years ago
|
||
Also I am missing animation-timing-function and transition-timing-function are the default timing function for each keyframe. So, the approach in the try is not sufficient CSS animations/transitions. We have to check that the target segment is not changed and the position in the segment is not changed as well.
|
|
Assignee | |
Comment 5•6 years ago
|
||
There were more things that needed to be involved. When we skip composing new animation values we have to use the previous composed values in any way. That means that we can skip animation calculation but can't skip composing. But on WebRender, it seems we don't need to compose the previous values? https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd1eaee9b0dc1650116d05b2f9d24ef52e212e25
|
||
Comment 6•6 years ago
|
||
Thanks for looking at this, hiro. Based on what you've found here, what are the chances this could ride the trains on 61? I ask, because I'm hoping to drive a SHIELD study on reducing the tab throbber frame rate to see how it impacts page load time, and I think we'd need this.
Flags: needinfo?(hikezoe)
|
|
Assignee | |
Comment 7•6 years ago
|
||
Yep, I've been working on this to land this as soon as possible. Unfortunately, the approach in comment 5 didn't work in some cases [1]. So I had to figure out another way. The another way seems to work fine [2]. So it will be in time for 61. :) [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51fd88b667d35353d064cce47ef790dd16b961b&selectedJob=174266133 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a0e986d9b7d758e384f1806dd8534f9ac8d244
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
|
|
Assignee | |
Comment 8•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > [2] > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=52a0e986d9b7d758e384f1806dd8534f9ac8d244 Oops, forgot to mention about H3 failure in this try. it's bug 1454839. I have no idea why the bug number doesn't appear there.
|
|
Assignee | |
Comment 9•6 years ago
|
||
Also note that the approach what I am doing is less effective than the equivalent optimization on the main-thread. Since on the compositor, even if we skip calculating animation values, we've already started a new transaction at that moment.
Summary: Skip composing animation value on the compositor if animation progress value hasn't been changed since the last composition → Skip calculating animation value on the compositor if animation progress value hasn't been changed since the last composition
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 13•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969152 [details] Bug 1454324 - Cache animation TimingParams on the compositor. https://reviewboard.mozilla.org/r/237870/#review243604 ::: gfx/layers/AnimationHelper.cpp:462 (Diff revision 1) > + Move(AnimationUtils::TimingFunctionToComputedTimingFunction( > + animation.easingFunction())) (I actually wonder if we need the Move here -- I would have thought we already have an rvalue here -- but in the interests of keeping this patch just to moving code it's probably fine to leave this.)
Attachment #8969152 -
Flags: review?(bbirtles) → review+
|
|
||
Comment 14•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969154 [details] Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. https://reviewboard.mozilla.org/r/237874/#review243608 This looks good but I'm curious about the second optimization here. ::: commit-message-fec26:4 (Diff revision 1) > +Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. r?birtles,kats > + > +Note that we have to calculate animation values if there is another animation > +since the other animaiton might be 'accumulate' or 'add', or might have a s/animaiton/animation/ ::: dom/animation/KeyframeEffectReadOnly.cpp:1591 (Diff revision 1) > + const ComputedTiming& aComputedTiming, > + const Nullable<double>& aProgressOnLastCompose, > + IterationCompositeOperation aIterationComposite, > + uint64_t aCurrentIterationOnLastCompose) aProgressOnLastCompose and aCurrentIterationOnLastCompose should be together since, together, they represent the result of the timing model from the last time we composed. Perhaps the order could be: aComputedTiming, aIterationComposite, aProgressOnLastCompose, aCurrentIterationOnLastCompose ::: gfx/layers/AnimationHelper.h:29 (Diff revision 1) > struct AnimData { > InfallibleTArray<mozilla::AnimationValue> mStartValues; > InfallibleTArray<mozilla::AnimationValue> mEndValues; > InfallibleTArray<Maybe<mozilla::ComputedTimingFunction>> mFunctions; > TimingParams mTiming; > + // These two variables are the counter part of same name variables in s/counter part/counterpart/ or just: "These two variables correspond to the variables of the same names in KeyframeEffectReadOnly and are used for the same purpose: to skip composing animations whose progress has not changed." ::: gfx/layers/AnimationHelper.h:34 (Diff revision 1) > + // These two variables are used for a similar optimization above but work for > + // timing function in each keyframe. s/but work for/but are applied to the/ ::: gfx/layers/AnimationHelper.h:218 (Diff revision 1) > + * Returns SampleResult::None if the all animations haven't yet started (i.e. > + * in delay state), SampleResult::Skipped if the animation values haven't "Returns SampleResult::None if none of the animations are producing a result (e.g. they are in the delay phase with no backwards fill), ..." ::: gfx/layers/AnimationHelper.h:219 (Diff revision 1) > + * in delay state), SampleResult::Skipped if the animation values haven't > + * changed since the last call of this function, SmapleResult::Sampled if s/animation values haven't changed/animation output did not change/ ? ::: gfx/layers/AnimationHelper.h:220 (Diff revision 1) > + * changed since the last call of this function, SmapleResult::Sampled if > + * new animation values were sampled. s/SmapleResult/SampleResult/ s/if new animation values were sample/if the animation output was updated/ ? ::: gfx/layers/AnimationHelper.cpp:181 (Diff revision 1) > > + dom::IterationCompositeOperation iterCompositeOperation = > + static_cast<dom::IterationCompositeOperation>( > + animation.iterationComposite()); > + > + if (iEnd == 1 && This needs a comment about why we check for iEnd == 1 (basically what you have in the commit message). (It seems a shame we can only do this optimization in a very limited set of circumstances because it produces a kind of performance cliff. The animation an be performing well and you make it slightly more complicated and suddenly there is a significant change in performance. How hard is it to expand this optimization?) ::: gfx/layers/AnimationHelper.cpp:207 (Diff revision 1) > > double portion = > ComputedTimingFunction::GetPortion(animData.mFunctions[segmentIndex], > positionInSegment, > computedTiming.mBeforeFlag); > + if (iEnd == 1 && Likewise this too needs a comment. ::: gfx/layers/AnimationHelper.cpp:207 (Diff revision 1) > + if (iEnd == 1 && > + animData.mSegmentIndexOnLastCompose == segmentIndex && > + !animData.mPortionInSegmentOnLastCompose.IsNull() && > + animData.mPortionInSegmentOnLastCompose.Value() == portion) { > + return SampleResult::Skipped; > + } Why do we need this optimization? I don't think we do this on the main thread do we? ::: gfx/layers/composite/AsyncCompositionManager.cpp:675 (Diff revision 1) > - animationValue); > + animationValue); > - } else { > - // Set non-animation values in the case there are no in-effect > - // animations (i.e. all animations are in delay state or already > - // finished with non-forward fill modes). > + break; > + } > + case AnimationHelper::SampleResult::Skipped: > + // We don't need to update animation values for this layer since > + // the values haven't been changed. nit: s/haven't been changed/haven't changed/
|
|
||
Comment 15•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969154 [details] Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. https://reviewboard.mozilla.org/r/237874/#review243618 Discussed this second optimization with hiro. Apparently we don't need it on the main thread because we perform do a similar optimization as part of our regular style processing anyway. We do need this on the compositor, however, since we don't have the same optimization there and we hit this particular case whenever we use step timing functions (e.g. the tab throbber and any other frame-based animations). r=me with comments addressed (including, ideally, a comment explaining why we need this optimization).
Attachment #8969154 -
Flags: review?(bbirtles) → review+
|
||
Comment 16•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969153 [details] Bug 1454324 - Set non-animated values to the layer only if there is no running animation. https://reviewboard.mozilla.org/r/237872/#review243758 ::: gfx/layers/composite/AsyncCompositionManager.cpp:666 (Diff revision 1) > layer->GetAnimationData(), > animationValue, > hasInEffectAnimations); > if (hasInEffectAnimations) { > Animation& animation = animations.LastElement(); > ApplyAnimatedValue(layer, ApplyAnimatedValue seems like it will either set the transform properties, or the opacity properties, depending on what kind of animation is running. However we want to make sure both sets of shadow properties are set on the shadow layer. It looks like this patch leaves a gap because in the case where there is an opacity in-effect animation, the shadow transform properties don't get updated and vice-versa. Or am I missing something?
Attachment #8969153 -
Flags: review?(bugmail)
|
|
||
Comment 17•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969154 [details] Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. https://reviewboard.mozilla.org/r/237874/#review243780 I don't really know too much about animation specifics to comment on most of this patch. The high-level changes look reasonable enough, although I think my comment on the previous patch still applies - there are cases where we were assigning the shadow properties before which this patch removes. Either that's a bug and need fixing, or it needs documentation as to why it's ok. ::: gfx/layers/composite/AsyncCompositionManager.cpp:674 (Diff revision 1) > - // finished with non-forward fill modes). > + // We don't need to update animation values for this layer since > + // the values haven't been changed. It might be worth adding an #ifdef DEBUG block here that asserts the animation values haven't changed.
Attachment #8969154 -
Flags: review?(bugmail) → review+
|
|
Assignee | |
Comment 18•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16) > Comment on attachment 8969153 [details] > Bug 1454324 - Set non-animated values to the layer only if there is no > running animation. > > https://reviewboard.mozilla.org/r/237872/#review243758 > > ::: gfx/layers/composite/AsyncCompositionManager.cpp:666 > (Diff revision 1) > > layer->GetAnimationData(), > > animationValue, > > hasInEffectAnimations); > > if (hasInEffectAnimations) { > > Animation& animation = animations.LastElement(); > > ApplyAnimatedValue(layer, > > ApplyAnimatedValue seems like it will either set the transform properties, > or the opacity properties, depending on what kind of animation is running. > However we want to make sure both sets of shadow properties are set on the > shadow layer. It looks like this patch leaves a gap because in the case > where there is an opacity in-effect animation, the shadow transform > properties don't get updated and vice-versa. Or am I missing something? Oh, I thought that the layer for opacity animation doesn't apply any transform and the layer for transform animation doesn't apply opacity either. I will set the non-animating opacity or transform respectively in ApplyAnimatedValue(). Thanks!
|
|
Assignee | |
Comment 19•6 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8969154 [details] Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. https://reviewboard.mozilla.org/r/237874/#review243608 > This needs a comment about why we check for iEnd == 1 (basically what you have in the commit message). > > (It seems a shame we can only do this optimization in a very limited set of circumstances because it produces a kind of performance cliff. The animation an be performing well and you make it slightly more complicated and suddenly there is a significant change in performance. How hard is it to expand this optimization?) I think it shouldn't be so hard. We can do the further optimization later (when we ship composite operation?). I will file a bug for that.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 22•6 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8969154 [details] Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. https://reviewboard.mozilla.org/r/237874/#review243780 Though I did post a new patch, I am still thinking how to do this assertion.
|
|
Assignee | |
Comment 23•6 years ago
|
||
The assertion is hit in some cases. It seems to me that there remains animations in layer even after corresponding CompositorAnimationStorage is cleared. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5616dbf3184330cba644c517a2ffda299e9a847&selectedJob=174705958
|
|
Assignee | |
Comment 24•6 years ago
|
||
There were two bugs. 1) We didn't reset mCompositorAnimationsId when we clear all animation data in AnimationInfo. 2) We didn't clear all animation data in AnimationInfo when LayerTransactionParent receives 'ReleaseLayer' Though actually those bugs are unrelated to this bug (just un-wallpaper them by the assertion), hope the assertion works as expected (i.e. not being hit) in this try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfdd3c41ac13947197619dd9885b0c449ffd20f
|
|
Assignee | |
Comment 25•6 years ago
|
||
Gah, more oranges, but can't reproduce locally...
|
|
||
Comment 26•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969153 [details] Bug 1454324 - Set non-animated values to the layer only if there is no running animation. https://reviewboard.mozilla.org/r/237872/#review244194
Attachment #8969153 -
Flags: review?(bugmail) → review+
|
|
||
Comment 27•6 years ago
|
||
https://hg.mozilla.org/try/rev/05338b60ee11142a7025ab5034961a94fdd8446b#l1.12 doesn't seem right. The CompositorAnimationStorage is shared between the "root" LayerTransactionParent (for the UI process layer tree) and the cross-process LayerTransactionParents (for the attached content layer trees). So if you clear the whole thing inside a LayerTransactionParent you're going to be clearing it for the other process too.
|
|
Assignee | |
Comment 28•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > https://hg.mozilla.org/try/rev/05338b60ee11142a7025ab5034961a94fdd8446b#l1. > 12 doesn't seem right. The CompositorAnimationStorage is shared between the > "root" LayerTransactionParent (for the UI process layer tree) and the > cross-process LayerTransactionParents (for the attached content layer > trees). So if you clear the whole thing inside a LayerTransactionParent > you're going to be clearing it for the other process too. Thanks! I did read your comment about it somewhere else, but totally forgot it! A new try without the commit looks fine! https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8c0b6ecc2d408c35b1792d519c97599e259324f
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 34•6 years ago
|
||
Note that attachment 8969852 [details] fixes below test failures on WebRender test_animations_effect_timing_enddelay.html test_animations_iterationstart.html test_animations_pausing.html test_animations_playbackrate.html Also a bunch of test cases in test_animations_omta.html are fixed by the patch. There are still 6 test failures in test_animations_omta.html though. Anyway I am going to enable those test in bug 1455999.
|
|
Assignee | |
Comment 35•6 years ago
|
||
Comment on attachment 8969852 [details] Bug 1454324 - Reset mCompositorAnimationsId when we remove animations. Hmm, attachment 8969852 [details] seems to break the machinery in nsDisplayOpacity::CreateWebRenderCommands and nsDisplayTransform::CreateWebRenderCommands. We should reset the id after calling AddCompositorAnimationsIdForDiscard().
Attachment #8969852 -
Flags: review?(bugmail)
|
|
Assignee | |
Comment 36•6 years ago
|
||
Another try without attachment 8969852 [details]. It turns out that the attachment didn't need here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b9121536e7def8a8e5115f549d9f099e78bed26
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•6 years ago
|
Attachment #8969852 -
Attachment is obsolete: true
|
|
||
Comment 41•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8969853 [details] Bug 1454324 - Clear all animation data when we release layer. https://reviewboard.mozilla.org/r/238700/#review244590 This seems straightforward enough.
Attachment #8969853 -
Flags: review?(bugmail) → review+
|
|
||
Comment 42•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #34) > Note that attachment 8969852 [details] fixes below test failures on WebRender FYI I'm less sure about this patch that resets the id. IIRC the id was reused intentionally to avoid churn of some sort, I'd have to dig through the code history to remember more.
|
|
Assignee | |
Comment 43•6 years ago
|
||
Thanks for the review! (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #34) > > Note that attachment 8969852 [details] fixes below test failures on WebRender > > FYI I'm less sure about this patch that resets the id. IIRC the id was > reused intentionally to avoid churn of some sort, I'd have to dig through > the code history to remember more. I noticed that the reason why the id remains on the compositor is that, on testing refresh mode, we proceeds test code before we receive DeleteCompositorAnimations on the compositor. Anyway it should be fixed in bug 1455999, I don't yet have any good idea to solve it though.
|
|
Assignee | |
Comment 44•6 years ago
|
||
A final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bff66ee3ce695702c5d5012a9819b19421e7c05
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 49•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a31d1d7403d7 Cache animation TimingParams on the compositor. r=birtles https://hg.mozilla.org/integration/autoland/rev/ddd9936f539d Clear all animation data when we release layer. r=kats https://hg.mozilla.org/integration/autoland/rev/6ba049b00c55 Set non-animated values to the layer only if there is no running animation. r=kats https://hg.mozilla.org/integration/autoland/rev/b63e4bff951e Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. r=birtles,kats
|
||
Comment 50•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a31d1d7403d7 https://hg.mozilla.org/mozilla-central/rev/ddd9936f539d https://hg.mozilla.org/mozilla-central/rev/6ba049b00c55 https://hg.mozilla.org/mozilla-central/rev/b63e4bff951e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
|
|
||
Comment 51•6 years ago
|
||
Hey hiro, thanks for landing this! Here's a profile with a tab throbber throbbing at 20fps: https://perfht.ml/2qUC5GI I notice that there's still a "composite" label in the Compositor thread every 16ms, though the pattern appears to be: two short composites (~0.15ms), followed by a longer composite (~0.75ms). Is this expected?
Flags: needinfo?(hikezoe)
|
|
||
Comment 52•6 years ago
|
||
After looking through the code a little more, I'm now worried that the change in the last patch might have introduced a WR regression, although I'm not totally sure how to go about testing it. Basically, the way we do animations in WR is that when we build the display list, we set a PropertyBindingKey [1] on the stacking context. This is like a placeholder that says "this stacking context has a transform, but we will tell you the value of the transform at render time". And then when we want do a render/composite, we provide the corresponding PropertyBindingValue via the UpdateDynamicProperties call at [2]. However, I think what your patch is going to do, is if the sampling step produces SampleResult::Skipped because the value hasn't changed, then it will omit setting the PropertyBindingValue entirely, because the sample animation step will skip setting the transform into `transformArray` at [3]. When this happens, if WR goes to render a frame, it won't find the PropertyBindingValue and it will fall back to some default thing, which for transforms is the identity transform. So the animation might glitch because for a frame in the middle the transform might reset to the identity transform. I tried to make a test case with a really slow animation to see if it would trigger this but I was unable to. So maybe my test page isn't good enough, or maybe I'm not understanding something. Either way if you have any thoughts on this let me know. [1] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/webrender_bindings/src/bindings.rs#1626 [2] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent.cpp#1282 [3] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent.cpp#1276
|
|
Assignee | |
Comment 53•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #51) > Hey hiro, thanks for landing this! > > Here's a profile with a tab throbber throbbing at 20fps: > > https://perfht.ml/2qUC5GI > > I notice that there's still a "composite" label in the Compositor thread > every 16ms, though the pattern appears to be: two short composites > (~0.15ms), followed by a longer composite (~0.75ms). Is this expected? Yes, that's what I mentioned in comment 9. This optimization can skip calculation for animations but not skip composition itself. I have an idea to skip the composition in the case where there needs composition only for unchanged animations and if the composition isn't triggered by non-animation. But it would get more stuff involved.
Flags: needinfo?(hikezoe)
|
|
Assignee | |
Comment 54•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #52) > After looking through the code a little more, I'm now worried that the > change in the last patch might have introduced a WR regression, although I'm > not totally sure how to go about testing it. Basically, the way we do > animations in WR is that when we build the display list, we set a > PropertyBindingKey [1] on the stacking context. This is like a placeholder > that says "this stacking context has a transform, but we will tell you the > value of the transform at render time". And then when we want do a > render/composite, we provide the corresponding PropertyBindingValue via the > UpdateDynamicProperties call at [2]. > > However, I think what your patch is going to do, is if the sampling step > produces SampleResult::Skipped because the value hasn't changed, then it > will omit setting the PropertyBindingValue entirely, because the sample > animation step will skip setting the transform into `transformArray` at [3]. > When this happens, if WR goes to render a frame, it won't find the > PropertyBindingValue and it will fall back to some default thing, which for > transforms is the identity transform. So the animation might glitch because > for a frame in the middle the transform might reset to the identity > transform. > > I tried to make a test case with a really slow animation to see if it would > trigger this but I was unable to. So maybe my test page isn't good enough, > or maybe I'm not understanding something. Either way if you have any > thoughts on this let me know. > > [1] > https://searchfox.org/mozilla-central/rev/ > 36dec78aecc40539ecc8d78e91612e38810f963c/gfx/webrender_bindings/src/bindings. > rs#1626 > [2] > https://searchfox.org/mozilla-central/rev/ > 36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent. > cpp#1282 > [3] > https://searchfox.org/mozilla-central/rev/ > 36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent. > cpp#1276 Thank you, kats! This explanation really helps to understand WebRender stuff for me. It sounds like bug 1456679 might introduce the regression, but at first glance, it won't since we just skip *updating* the previous animation values and the previous values are preserved and used actually. I am going to check the code from now on.
|
||
Comment 55•6 years ago
|
||
Sorry didn't want to create a new bug since not sure what's going on here can you guys look into this profiles 20/30 FPS throbbers freaking out and slow performance of page loads but still better than the default ones Even when loading 10-20 pages which is very common Bad https://perfht.ml/2r2GE0L Worse https://perfht.ml/2r0qi8I Worst with WR https://perfht.ml/2r39qOI Something is very wrong with the new svg throbbers circling ones were better
Flags: needinfo?(hikezoe)
Flags: needinfo?(bugmail)
|
|
||
Comment 56•6 years ago
|
||
https://perfht.ml/2r5T3kK HW accleration off https://perfht.ml/2r0u2ag
|
|
||
Comment 57•6 years ago
|
||
(In reply to Jason Mechelynck from comment #55) > 20/30 FPS throbbers freaking out and slow performance of page loads but > still better than the default ones Can you explain what you mean a little more clearly? How are you measuring the FPS? Is the page load slower than it was before (i.e. a regression)? How did you decide this patch/bug was responsible?
Flags: needinfo?(bugmail) → needinfo?(pevar)
|
|
Assignee | |
Comment 58•6 years ago
|
||
I think the FPS means the preference names [1] for throbbers on tabs. But I don't see any suspicious on the compositor side in the profiles. [1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/browser/app/profile/firefox.js#457
Flags: needinfo?(hikezoe)
|
|
||
Comment 59•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #58) > I think the FPS means the preference names [1] for throbbers on tabs. But I > don't see any suspicious on the compositor side in the profiles. > > [1] > https://searchfox.org/mozilla-central/rev/ > 78dbe34925f04975f16cb9a5d4938be714d41897/browser/app/profile/firefox.js#457 Yes this
Flags: needinfo?(pevar)
|
||
Comment 60•6 years ago
|
||
I got following error when visit any instagram page. Assertion failure: transform->mTransform.mTransformInDevSpace.FuzzyEqualsMultiplicative( (layer->AsHostLayer()->GetShadowBaseTransform())), at /home/thinker/progm/mozilla-central/gfx/layers/composite/AsyncCompositionManager.cpp:696 For example, https://www.instagram.com/explore/locations/49695104/brooklyn-bridge/ The process crashed when I scrolled down the page.
|
|
Assignee | |
Comment 61•6 years ago
|
||
Hi Thinker! (In reply to Thinker Li [:sinker] from comment #60) > I got following error when visit any instagram page. > > Assertion failure: > transform->mTransform.mTransformInDevSpace.FuzzyEqualsMultiplicative( > (layer->AsHostLayer()->GetShadowBaseTransform())), at > /home/thinker/progm/mozilla-central/gfx/layers/composite/ > AsyncCompositionManager.cpp:696 That's bug 1457249. I will land patches for the bug soon.
You need to log in
before you can comment on or make changes to this bug.
Description
•