1437032 - Rounding error in layout/reftests/bugs/1291528.html

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Nicolas Silva [:nical]]

While building the WR display list we call into StackingContextHelper::ToRelativeLayoutRect in many places. The latter rounds the coordinates which should work if we are careful about rounding all parameters the same way and all scales and transforms are applied but we seem to be missing something which causes the gradient to repeat an extra row of pixels at the bottom.

Comment 1

[Nicolas Silva [:nical]]

CC Lee, the rumor has it that you are also dealing with rounding-related issues.

Comment 2

[Nicolas Silva [:nical]]

I think I finally understand what is really going on here:

This this test case the is a gradient of height 18.5px with a 2px border. For some reason which I am sure has to do with the wonders of CSS, we extend the gradient's rect by 2px on each side (I guess so that the it would show under a semi transparent border), but still want the origin of information like the gradient points to be offset as if we hadn't extend the gradient rect (basically we don't want to move the frame of reference along with the gradient rect's to-left corner).
To achieve this we pick an origin for the gradient that is offset by the size of the original bounds minus border with the intent that the second repetition of the gradient will start at the right spot: 2px after the gradient bounds. We then use a clip rect to make sure that the gadient doesn't actually extend up to its origin.

The problem is that the gradient tile size and offset haven't been computed with the snapping taken into account, which makes the second repetition of the gradient potentially land on the wrong spot if the size of the element isn't an integer amount.

Comment 3

[Markus Stange [:mstange]]

Good work!

(In reply to Nicolas Silva [:nical] from comment #2)
> For some reason which I am sure has to do with the wonders of CSS, we extend
> the gradient's rect by 2px on each side (I guess so that the it would show
> under a semi transparent border), but still want the origin of information
> like the gradient points to be offset as if we hadn't extend the gradient
> rect (basically we don't want to move the frame of reference along with the
> gradient rect's to-left corner).

This is because background-origin defaults to padding-box and background-clip defaults to border-box.

Comment 4

[Nicolas Silva [:nical]]

The trickiness here, is that the equivalent painting code manually snaps each tile of the gradient while the webrender dl building one snaps the parameter and repeats them (meaning the error accumulates with each repetition).

I am more and more erring on the side of "this isn't fixable on the gecko side and has to be addressed in webrender." I suppose tiled background images will have a similar problem.

Just removing the snapping on the gecko side fixes this reftest and makes another one fail (but fuzzable), so I think I'll go ahead and do that.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Don't snap gradients when building the WebRender display item.

In order to properly snap repeated gradients we need to snap each repetition individually, which is inefficient to do at the display list level. The current way snapping is done here is wrong so let's remove it. I suspect webrender's own pixel snapping is a tad naive but it's better than what we do here.

One of the big issues we have here is that either we adjust all gradient parameters to account for how much the rectangle was stretched by the snapping (in which case this adjustment accumulate with each repetition which is quite bad), or we can end up with an extra line/column of pixels of the wrong color depending on how things get snapped (what this test shows and the glitch is quite noticeable).


try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce16a5f988ebd003e135edeccabec4832743f5e8

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc150f21f7fa
Rely on WebRender to pixel snap gradients instead of doing it when building the display list. r=mstange

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/fc150f21f7fa