802321 - Repeated, unnecessary invalidation of transformed element as it scrolls

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: testcase

This is another case of the issue that was fixed in bug 800198. It seems like we are still needlessly invalidating transformed elements as we scroll. In this case the over-invalidation seems to be triggered when a rotation is involved.

Comment 1

[Jonathan Watt [:jwatt]]

Ugh. In this testcase, when scrolling the div down very slowly we also seem to needlessly invalidate the entire content area over and over again. Curiously that doesn't seem to happen when we scroll down a bit faster, and doesn't seem to happen at all when scrolling up (well, with the exception of when we hit bug 801918).

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: SVG version of testcase

This SVG version has slightly different behavior.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Use FuzzyEquals to compare matrices in LayerTreeInvalidation

Rounding bugs are fun.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 672026 [details] [diff] [review]
Use FuzzyEquals to compare matrices in LayerTreeInvalidation

Review of attachment 672026 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfx3DMatrix.cpp
@@ +131,5 @@
> +  return ::FuzzyEqual(_11, o._11) && ::FuzzyEqual(_12, o._12) && ::FuzzyEqual(_13, o._13) && ::FuzzyEqual(_14, o._14) &&
> +         ::FuzzyEqual(_21, o._21) && ::FuzzyEqual(_22, o._22) && ::FuzzyEqual(_23, o._23) && ::FuzzyEqual(_24, o._24) &&
> +         ::FuzzyEqual(_31, o._31) && ::FuzzyEqual(_32, o._32) && ::FuzzyEqual(_33, o._33) && ::FuzzyEqual(_34, o._34) &&
> +         ::FuzzyEqual(_41, o._41) && ::FuzzyEqual(_42, o._42) && ::FuzzyEqual(_43, o._43) && ::FuzzyEqual(_44, o._44);
> +}

Use mozilla::gfx::FuzzyEqual from Tools.h.

Is 1e-4 actually needed here or can we go lower?

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: alternate testcase

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: alternate testcase 2

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: alternate testcase 3

These three alternate testcases (created along the way while I was reducing http://www.jasondavies.com/factorisation-diagrams/ to the first two testcase I attached) don't seem to be fixed by your patch, Matt. :(

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Comment on attachment 672026 [details] [diff] [review]
> Use FuzzyEquals to compare matrices in LayerTreeInvalidation
> 
> Review of attachment 672026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfx3DMatrix.cpp
> @@ +131,5 @@
> > +  return ::FuzzyEqual(_11, o._11) && ::FuzzyEqual(_12, o._12) && ::FuzzyEqual(_13, o._13) && ::FuzzyEqual(_14, o._14) &&
> > +         ::FuzzyEqual(_21, o._21) && ::FuzzyEqual(_22, o._22) && ::FuzzyEqual(_23, o._23) && ::FuzzyEqual(_24, o._24) &&
> > +         ::FuzzyEqual(_31, o._31) && ::FuzzyEqual(_32, o._32) && ::FuzzyEqual(_33, o._33) && ::FuzzyEqual(_34, o._34) &&
> > +         ::FuzzyEqual(_41, o._41) && ::FuzzyEqual(_42, o._42) && ::FuzzyEqual(_43, o._43) && ::FuzzyEqual(_44, o._44);
> > +}
> 
> Use mozilla::gfx::FuzzyEqual from Tools.h.

Ok.

> 
> Is 1e-4 actually needed here or can we go lower?

The error margin I recorded when debugging it was 0.00000096, and it didn't work using 1e-6, so I'm guessing that it must get slightly higher at times.

Maybe 1e-5 if it's important?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Use FuzzyEquals to compare matrices in LayerTreeInvalidation v2

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't compare geometry objects for inactive layers

The invalidations were due to the bounds of the nsDisplayTransform changing when we scrolled past a certain point. Rounding bugs yet again.

Lets just not compare geometries for inactive layers at all, since we still have LayerTreeInvalidation happening.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 672087 [details] [diff] [review]
Use FuzzyEquals to compare matrices in LayerTreeInvalidation v2

Review of attachment 672087 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfx3DMatrix.cpp
@@ +129,5 @@
> +         gfx::FuzzyEqual(_13, o._13, 1e-4) && gfx::FuzzyEqual(_14, o._14, 1e-4) &&
> +         gfx::FuzzyEqual(_21, o._21, 1e-4) && gfx::FuzzyEqual(_22, o._22, 1e-4) && 
> +         gfx::FuzzyEqual(_23, o._23, 1e-4) && gfx::FuzzyEqual(_24, o._24, 1e-4) &&
> +         gfx::FuzzyEqual(_31, o._31, 1e-4) && gfx::FuzzyEqual(_32, o._32, 1e-4) && 
> +         gfx::FuzzyEqual(_33, o._33, 1e-4) && gfx::FuzzyEqual(_34, o._34, 1e04) &&

Nice typo. I suggest replacing 1e-4 with a static const float declared right here.

Comment 12

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a69dfa3ed05
https://hg.mozilla.org/integration/mozilla-inbound/rev/48aff95d2d29

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4a69dfa3ed05
https://hg.mozilla.org/mozilla-central/rev/48aff95d2d29