Closed Bug 1008915 Opened 6 years ago Closed 6 years ago

Unnecessary invalidation during scrolling due to floating point precision loss

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(2 files)

Attached file testcase
STR:
 1. Load the testcase and enable paint flashing.
 2. Scroll up and down a bit.

We unnecessarily invalidate the rectangle.
Attached patch v1Splinter Review
The problem here is that the X translation component of the final matrix computed in nsDisplayTransform::GetTransform varies depending on the vertical scroll position.

Without this patch, the matrix is computed as follows:

 1. nsDisplayTransform::GetResultingTransformMatrixInternal arrives at this matrix before the final basis change:

>   0.000000,    1.210612,  0.000000, 0.000000
>   -2.537108,   0.000000,  0.000000, 0.000000
>   0.000000,    0.000000,  1.000000, 0.000000
>   2282.956055, 82.881538, 0.000000, 1.000000

 2. nsDisplayTransform::GetResultingTransformMatrixInternal calls nsLayoutUtils::ChangeMatrixBasis.
   - aProperties.mToTransformOrigin is -99.983330, -599.983337
   - roundedOrigin is 107.983330, 506.983337
                   or 107.983330, 507.983337
                                   ^^ depending on the scroll position

 3. nsDisplayTransform::GetTransform calls mTransform.Translate(rounded), with rounded being the same as roundedOrigin in the previous step.

With roundedOrigin.y == 506.983337, the final matrix is:

>   0.000000,   1.210612,   0.000000, 0.000000
>   -2.537108,  0.000000,   0.000000, 0.000000
>   0.000000,   0.000000,   1.000000, 0.000000
>   768.733521, 110.922562, 0.000000, 1.000000
           ^^^    ^

With roundedOrigin.y == 507.983337, the final matrix is:

>   0.000000,   1.210612,   0.000000, 0.000000
>   -2.537108,  0.000000,   0.000000, 0.000000
>   0.000000,   0.000000,   1.000000, 0.000000
>   768.733826, 111.922562, 0.000000, 1.000000
           ^^^    ^

Note the difference in the translation components: The 1px difference in the Y component is expected, because roundedOrigin has changed by 1px due to the scroll, but the difference in the X component is unexpected and causes the invalidation.

With this patch applied, the final matrices look like this:

roundedOrigin.y == 506.983337:
>   0.000000,   1.210612,   0.000000, 0.000000
>   -2.537108,  0.000000,   0.000000, 0.000000
>   0.000000,   0.000000,   1.000000, 0.000000
>   768.733704, 110.922562, 0.000000, 1.000000

roundedOrigin.y == 507.983337:
>   0.000000,   1.210612,   0.000000, 0.000000
>   -2.537108,  0.000000,   0.000000, 0.000000
>   0.000000,   0.000000,   1.000000, 0.000000
>   768.733704, 111.922562, 0.000000, 1.000000
Attachment #8421006 - Flags: review?(dbaron)
Blocks: 866415
Attachment #8421006 - Flags: review?(dbaron) → review?(matt.woodrow)
Comment on attachment 8421006 [details] [diff] [review]
v1

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

::: layout/base/nsDisplayList.h
@@ +3325,5 @@
>     *        for the frame's bounding rectangle. Otherwise, it will use the
>     *        value of aBoundsOverride.  This is mostly for internal use and in
>     *        most cases you will not need to specify a value.
> +   * @param aOffsetByOrigin If true, the resulting matrix will be translated
> +   *        by aOrigin.

The origin offset is applied *before* the CSS transform, the comment should make this clear.
Attachment #8421006 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/d7968acde595
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1020556
You need to log in before you can comment on or make changes to this bug.