Closed
Bug 1165185
Opened 9 years ago
Closed 6 years ago
Invalidating transformed elements during scrolling
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: mstange, Assigned: mstange)
References
Details
Attachments
(3 files, 4 obsolete files)
863 bytes,
text/html
|
Details | |
6.01 KB,
text/plain
|
Details | |
3.87 KB,
patch
|
Details | Diff | Splinter Review |
In this testcase, the SVG squares are invalidated when scrolling up around the 100 pixel scroll position. This was reduced from bug 950946. In the testcase, the invalidation is coming from the FuzzyEqual transform check of the inactive container layer for the transform. We're comparing these values: -4030.64648 = -4177.64648 + 147 = -4000.64673 + -177 + 147 -4030.64673 = -4030.64673 + 0 = -4000.64673 + -30 -4000.64673 is the original SVG transform, 30 and 177 are the two scroll positions, and 147 is the offset between them. FuzzyEqual uses a fixed epsilon of 1e-5, so we fail the check and invalidate.
Assignee | ||
Comment 1•9 years ago
|
||
/r/8811 - Bug 1165185 - Avoid invalidations due to precision loss when shifting transforms. r=roc Pull down this commit: hg pull -r 65adfa0d47016e95567c47b4e74adb7b9cf22106 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 2•9 years ago
|
||
This patch does some unnecessary work by comparing the non-translation matrix elements twice. I'll add a method to Matrix4x4 instead that avoids that duplicated work.
Assignee | ||
Comment 3•9 years ago
|
||
Actually, we can't avoid the work because the matrix could have perspective and so PostTranslate wouldn't just affect the translation components.
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1165185 - Avoid invalidations due to precision loss when shifting transforms. r=roc
Attachment #8612668 -
Flags: review?(roc)
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc https://reviewboard.mozilla.org/r/8811/#review9157 ::: layout/reftests/invalidation/fractional-transform-1.html:18 (Diff revision 1) > + <rect x="100" y="2300" height="50" width="50" fill="grey" class="reftest-no-paint"/> Remove tab character ::: gfx/layers/LayerTreeInvalidation.cpp:106 (Diff revision 1) > + * -4177.64648, which is exactly aMatrixA._42. I don't understand this. -4030.64673-147 is -4177.64673 on my computer :-)
Attachment #8612668 -
Flags: review?(roc)
Assignee | ||
Comment 6•9 years ago
|
||
https://reviewboard.mozilla.org/r/8811/#review9243 > I don't understand this. -4030.64673-147 is -4177.64673 on my computer :-) My bad - the matrix elements are floats, so I'll change all those numbers to include an "f" at the end. printf("%f\n", -4030.64673f - 147); prints "-4177.646484" for me.
Assignee | ||
Updated•9 years ago
|
Attachment #8612668 -
Flags: review?(roc)
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc Bug 1165185 - Avoid invalidations due to precision loss when shifting transforms. r=roc
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc https://reviewboard.mozilla.org/r/8811/#review9273 ::: gfx/layers/LayerTreeInvalidation.cpp:121 (Diff revision 2) > + aMatrixA.FuzzyEqual(Matrix4x4(aMatrixB).PostTranslate(-aOffset.x, -aOffset.y, 0)); I wonder if it would make more sense to simply update our definition of fuzziness to check directly for imprecision in the last few bits of the mantissa. E.g. change gfx::FuzzyEqual to return true when fabs(a - b) <= max(abs(a), abs(b))*0.001 or something more efficient than that...
Attachment #8612668 -
Flags: review?(roc)
Assignee | ||
Comment 9•9 years ago
|
||
Yeah, let's just do that. It turns out that there are cases in which my patch doesn't work. Here's an example of where it doesn't work: CSS transform translation 112.15299, old scroll offset -709, new scroll offset -617: comparing one direction: -504.846985f = -596.846985f + 92 = 112.152992f + -709 + 92 -504.847015f = -504.847015f + 0 = 112.152992f + -617 comparing the other direction: -596.846985f = -596.846985f + 0 = 112.152992f + -709 -596.847046f = -504.847015f - 92 = 112.152992f + -617 - 92
Assignee | ||
Comment 10•9 years ago
|
||
Here's a test program that uses nextafterf to run through all possible float values, and lets you try different implementations for each part of the transform comparison process. I haven't found a way to avoid invalidations in the presence of NudgeToInteger, so I propose we remove it.
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc
Attachment #8612668 -
Attachment description: MozReview Request: Bug 1165185 - Avoid invalidations due to precision loss when shifting transforms. r=roc → MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc
Attachment #8612668 -
Flags: review?(roc)
Assignee | ||
Comment 12•9 years ago
|
||
Roc, do you think the removal of NudgeToIntegers will cause problems? I created a new FuzzyEqual function in response to Jeff's feedback. His opinion is that FuzzyEqual is really a terrible function, and every user should be forced to use a very long function name so they'll think twice about using it, and so they'd be forced to decide what behavior they want. I can rename the existing FuzzyEqual to FuzzyEqualWithFixedEpsilon, if you'd like.
Assignee | ||
Comment 13•9 years ago
|
||
Oops, the setTimeout in the second reftest obviously needs to be changed to MozReftestInvalidate.
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc https://reviewboard.mozilla.org/r/8811/#review9349 ::: gfx/2d/Matrix.h:28 (Diff revision 3) > + float maxabs = std::max(fabs(aV1), fabs(aV2)); I fear this is going to be super slow. How about we special case aV1 == aV2? ::: layout/base/FrameLayerBuilder.cpp (Diff revision 3) > - transform.NudgeToIntegersFixedEpsilon(); Why are you removing this?
Attachment #8612668 -
Flags: review?(roc)
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > Comment on attachment 8612668 [details] > MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling > transformed elements. r=roc > > https://reviewboard.mozilla.org/r/8811/#review9349 > > ::: gfx/2d/Matrix.h:28 > (Diff revision 3) > > + float maxabs = std::max(fabs(aV1), fabs(aV2)); > > I fear this is going to be super slow. > > How about we special case aV1 == aV2? Good idea. > ::: layout/base/FrameLayerBuilder.cpp > (Diff revision 3) > > - transform.NudgeToIntegersFixedEpsilon(); > > Why are you removing this? Because it'll cause FuzzyEqualWithProportionalEpsilon to return false in many cases. Example Raw translation: 0.999948799610138f Old scroll offset: 11 New scroll offset: 51 0.999948799610138f + 51 = 51.999950408935547f will be nudged to 52.0f, but 0.999948799610138f + 11 = 11.999948501586914f won't be nudged, and then in the end we're comparing 51.999946594238281f to 52.0f and return false.
Flags: needinfo?(roc)
Comment 16•9 years ago
|
||
I haven't looked at the patch but there are additive and multiplicative fuzzy-equals functions with suitably long names in mfbt.
(In reply to Markus Stange [:mstange] from comment #15) > > ::: layout/base/FrameLayerBuilder.cpp > > (Diff revision 3) > > > - transform.NudgeToIntegersFixedEpsilon(); > > > > Why are you removing this? > > Because it'll cause FuzzyEqualWithProportionalEpsilon to return false in > many cases. > > Example > Raw translation: 0.999948799610138f > Old scroll offset: 11 > New scroll offset: 51 > > 0.999948799610138f + 51 = 51.999950408935547f will be nudged to 52.0f, but > 0.999948799610138f + 11 = 11.999948501586914f won't be nudged, and then in > the end we're comparing 51.999946594238281f to 52.0f and return false. NudgeToIntegers was introduced to avoid problems with CanDraw2D and Is2D returning false when values like Z are close to zero. I guess you can preserve that behavior by nudging just for the input to CanDraw2D and Is2D?
Flags: needinfo?(roc)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8606106 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc
Attachment #8612668 -
Flags: review?(roc)
Assignee | ||
Comment 21•9 years ago
|
||
I'm really sorry for the spam, there was another bug in there...
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc
Assignee | ||
Comment 23•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=616cb13a1640
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc https://reviewboard.mozilla.org/r/8811/#review9573 Ship It!
Attachment #8612668 -
Flags: review?(roc) → review+
Assignee | ||
Comment 25•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38db29ffd291
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc
Attachment #8612668 -
Flags: review+
Assignee | ||
Comment 27•9 years ago
|
||
Bug 1165185 - Nudge layer prescale to integers. r=roc
Attachment #8623286 -
Flags: review?(roc)
Assignee | ||
Comment 28•9 years ago
|
||
https://reviewboard.mozilla.org/r/11479/#review9899 ::: layout/base/FrameLayerBuilder.cpp:4781 (Diff revision 1) > - // XXX Do we need to move nearly-integer values to integers here? > + scale = NudgedToIntegerSize(scale); The change from NudeToIntegerFixedEpsilon() to NudgeTo2D() means that we're no longer nudging the transform's _22 value, which factors into the layer's prescale. So with just the previous patch, there is one test failure in layout/reftests/transform-3d/preserve3d-1a.html where we have a _22 value of 0.99999988079071044922 which ends up in the layer's prescale and makes the text render differently. Nudging the scale fixes the failure. ::: layout/base/FrameLayerBuilder.cpp:4656 (Diff revision 1) > + float width = aSize.width; > + float height = aSize.height; > + gfx::NudgeToInteger(&width); > + gfx::NudgeToInteger(&height); This is surprisingly ugly. Should I add an overload of gfx::NudgeToInteger that accepts a pointer to double, so that I don't need this temporary conversion to float?
https://reviewboard.mozilla.org/r/11479/#review9957 > This is surprisingly ugly. Should I add an overload of gfx::NudgeToInteger that accepts a pointer to double, so that I don't need this temporary conversion to float? No, this is OK.
Comment on attachment 8612668 [details] MozReview Request: Bug 1165185 - Try to avoid invalidations when scrolling transformed elements. r=roc https://reviewboard.mozilla.org/r/8811/#review9959 Ship It!
Attachment #8612668 -
Flags: review+
Attachment #8623286 -
Flags: review?(roc) → review+
Comment on attachment 8623286 [details] MozReview Request: Bug 1165185 - Nudge layer prescale to integers. r=roc https://reviewboard.mozilla.org/r/11479/#review9961 Ship It!
Assignee | ||
Comment 32•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f025568ad34 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5c445f7e20
Comment 33•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3f025568ad34 https://hg.mozilla.org/mozilla-central/rev/5f5c445f7e20
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 34•8 years ago
|
||
This got backed out in 48 for causing bug 1209100.
Status: RESOLVED → REOPENED
status-firefox45:
--- → fixed
status-firefox46:
--- → fixed
status-firefox47:
--- → fixed
status-firefox48:
--- → affected
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8612668 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8623286 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8969856 -
Attachment is obsolete: true
Assignee | ||
Comment 36•6 years ago
|
||
Comment 37•6 years ago
|
||
Pushed by jmuizelaar@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/88d41ddd11be Add a test for not invalidating transformed elements inside SVG during scrolling. r=roc
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/88d41ddd11be
Status: REOPENED → RESOLVED
Closed: 9 years ago → 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•