Closed Bug 1165185 Opened 9 years ago Closed 6 years ago

Invalidating transformed elements during scrolling

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox41 --- fixed
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- wontfix
firefox62 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached file testcase
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.
/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/
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.
Actually, we can't avoid the work because the matrix could have perspective and so PostTranslate wouldn't just affect the translation components.
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)
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.
Attachment #8612668 - Flags: review?(roc)
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)
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
Attached file test program
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.
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)
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.
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)
(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)
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)
Attachment #8606106 - Attachment is obsolete: true
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
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)
I'm really sorry for the spam, there was another bug in there...
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
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+
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+
Bug 1165185 - Nudge layer prescale to integers. r=roc
Attachment #8623286 - Flags: review?(roc)
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+
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
Depends on: 1209100
Depends on: 1202714
Depends on: 1255962
This got backed out in 48 for causing bug 1209100.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 866415
Attachment #8612668 - Attachment is obsolete: true
Attachment #8623286 - Attachment is obsolete: true
Depends on: 1458968
Attachment #8969856 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/88d41ddd11be
Status: REOPENED → RESOLVED
Closed: 9 years ago6 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: mozilla41 → mozilla62
You need to log in before you can comment on or make changes to this bug.