1824242 - WPT failure for svg-rotate-angle-45-* tests, with fuzzy pixels along edges

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: screenshot showing the barely fuzzy pixels in one of these tests

Three Firefox failures here:
https://wpt.fyi/results/css/css-transforms/rotate?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2021-transforms%20svg-rotate-angle-45

As noted in https://github.com/web-platform-tests/wpt/issues/35945 , the test has a red diamond shape, precisely covered by a rotated rect. We've got some barely-fuzzy pixels (maxDifference=1) along two of the edges.

See attached screenshot of the compare view for https://wpt.fyi/results/css/css-transforms/rotate/svg-rotate-angle-45-022.html

We should either annotate the failures, or make the red diamond a tad smaller to avoid any antialiased fringe getting composited together. [EDIT: making the red diamond smaller doesn't help; see comment 6.]

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(These tests were improved somewhat in https://github.com/web-platform-tests/wpt/issues/35945 to avoid a similar issue where red fringe was leaking off the top of the green square, BTW.)

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Side note: when I remove our .ini files for these tests, they all still seem to pass locally for me (on Linux), in headless as well as non-headless mode. I wonder if our .ini files are still needed, and if something additional is needed to trigger the failure that WPT.fyi is reporting...

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Here's a try run to see if we can remove our own annotations:
https://treeherder.mozilla.org/jobs?repo=try&revision=f7c2f4e9d1813d76d9cf0ac0616ce927304f2cff

On WPT.fyi dashboard, the differing pixels are all RGB(0, 127, 0) in the testcase vs. RGB(0, 128, 0) in the reference case. So it's not exactly that red pixels are showing through, but rather that we're picking up a slight lack-of-color when compositing together this antialiased fringe with the (green) pixels below it.

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #3)

On WPT.fyi dashboard, the differing pixels are all RGB(0, 127, 0) in the testcase vs. RGB(0, 128, 0) in the reference case. So it's not exactly that red pixels are showing through, but rather that we're picking up a slight lack-of-color when compositing together this antialiased fringe with the (green) pixels below it.

(Or maybe this is a pixel from the outer edge of the red diamond's antialiasing fringe that doesn't entirely get covered up, where we're like almost entirely green, and the teensy amount of redness gets rounded down to 0 in the red color channel, but has enough of an impact to also round down the green color channel from 128 to 127.)

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Looks like my Try run does hit the same fuzzy-failure that wpt.fyi is reporting.

Here's another try run with the borders of the red diamond shifted inwards by 1px; let's see if that helps:
https://treeherder.mozilla.org/jobs?repo=try&revision=62042aa59c0b31e92d03c45075976d1321890a13

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Shifting inwards did not help. More interestingly: hiding the red shape entirely does not help (!!) Here's a Try run where I did that, changing the red shape to have fill="none" so that it doesn't render:
https://treeherder.mozilla.org/jobs?repo=try&revision=a65dbf81d8934b1753d6480c16038aeee720f56b

So this seems to be a WebRender/compositing bug of some sort, where we're painting two green objects (one of which is transformed to be entirely inside the bounds of the other) and we produce a barely-lighter color (color channel 127 instead of 128) at the antialiased line-of-overlap.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Aha, one thing I hadn't noticed -- software webrender is fine. We have no failures for these tests in the W-swr tasks in my above-linked try runs.

So this is a bug in our GPU WebRender backend.

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Here's another try run where I've edited svg-rotate-angle-45-001.html to remove the red shape entirely; and I added a bunch of rotated green squares for good measure (all of them over the solid green background, without any holes that need covering-up or anything fancy):
https://treeherder.mozilla.org/jobs?repo=try&revision=de5b3c887c4a9410e9925baa91fc499420ba9460

Here, everything is styled as "green" i.e. rgb(0,128,0). Somehow this produces a whole bunch of pixels with +/- 1 in the green color channel at the edges of the rotated shapes, as shown in the failure logs. So there's some compositing brokenness happening here.

Glenn, is this a known issue with our GPU WebRender backend, and is there anything we can do about it? (We could just annotate these tests as fuzzy, but it'd be nice to fix the bug and avoid curating the various fuzzy annotations for green-SVG-shapes-over-green-backgrounds, if possible.)

Comment 9

[Glenn Watson [:gw]]

(below is speculation, I'd need to spend some significant time on this to conclusively answer)

If it renders correctly in sw-wr but is off by 1/255 in hw-wr, it's unlikely to be a simple fix. It's possibly either a hardware accuracy issue, or an accuracy / rounding issue somewhere in the clip mask / AA WR code. Looking at the ref test, it's also possible that one of the tests goes down the blob rendering path, while one hits native WR primitives, I'm not sure.

I think we're best to mark these as fuzzy, for now, it's unlikely I'll have time to look in to this in depth this half (though, having said that, I am working on changes to the clip-mask and tiled rendering code this half, it's possible I may fix or need to investigate this issue as part of finalizing those changes).

Comment 10

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Appreciate the insights - thanks!

(In reply to Glenn Watson [:gw] from comment #9)

If it renders correctly in sw-wr but is off by 1/255 in hw-wr> [...]

It does / it is, yeah.

[...] it's unlikely to be a simple fix.

OK, good to know.

I think we're best to mark these as fuzzy, for now, it's unlikely I'll have time to look in to this in depth this half

Cool. Yeah, this is visually imperceptible, so it's hard to make a case for making this a priority to fix, other than from a "that seems really-unexpected, computationally" sort of perspective.

I'm happy to mark these as fuzzy for the time being, though (when time allows). A maxDifference=1 mismatch along the edge of a rotated shape seems pretty trivially fine from a fuzzy-annotation perspective, and unrelated to what the test is trying to test for (which is does the transform work at all; and it does work.)

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I've spun off bug 1840511 for the actual WebRender bug, and I intend to co-opt this bug to land some fuzzy annotations.

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Tentative "part 1" to add a reftest that fails, based on the WPT test:
https://treeherder.mozilla.org/jobs?repo=try&revision=79d8ecce57f9eac9ba7107d75a7588bb3272c5d4

It passes on some platforms (like on my local machine, see comment 2); I think anywhere with software-webrender, possibly other configs too. I'm using the Try run to determine what the fails-if annotation should be.

Comment 13

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: Bug 1824242 part 1: Add reftest variants of some known-fuzzy WPTs, with strict fuzzy annotations, so we can find out if our underlying bug gets fixed in the future. r?#layout-reviewers

The known failure is tracked in followup bug 1840511 (which I'm using as the
name of the reftest).

I'm doing this because the WPTs in question are getting a fuzzy allowance baked
into them in the next patch in this series -- and since the fuzziness seems to
be reliable, we can hopefully use this expected-to-fail variant to detect if
that issue ever goes away (at which point we could theoretically remove the
fuzzy annotations from the WPTs themselves).

Comment 14

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: Bug 1824242 part 2: Annotate some SVG WPTs as having some barely-fuzzy (maxDifference=1) pixels. r?#layout-reviewers

The barely-fuzzy pixels are visually imperceptible; they're just off-by-one in
a single color channel, along an antialiased edge of a transformed shape.

We have https://bugzilla.mozilla.org/show_bug.cgi?id=1840511 filed to track the
underlying behavior here. Since it's imperceptible and isn't what these tests
are trying to test for, let's adjust the fuzzy tolerance to avoid a spurious
failure.

Depends on D182294

Comment 15

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Green Try run (with patches for this and bug 1819467):
https://treeherder.mozilla.org/jobs?repo=try&revision=93c6cacea756aabe7cdf34be62f7d6a21f4ad33a

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e6c87c74998
part 1: Add two reftest variants of some known-fuzzy WPTs, with strict fuzzy annotations, so we can find out if our underlying bug gets fixed in the future. r=layout-reviewers,jfkthame
https://hg.mozilla.org/integration/autoland/rev/4c8a3d74c0b7
part 2: Annotate some SVG WPTs as having some barely-fuzzy (maxDifference=1) pixels. r=layout-reviewers,jfkthame

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40802 for changes under testing/web-platform/tests

Comment 18

[Serban Stanca [:SerbanS]]

Backed out for causing reftests failures in 1840747-1.html.

• Backout link
• Push with failures
• Failure Log
• Failure line: REFTEST TEST-UNEXPECTED-PASS | layout/reftests/bugs/1840747-1.html == about:blank | image comparison, max difference: 0, number of differing pixels: 0

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 20

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Thanks! It seems the newly-added 1840747-1.html doesn't reproduce its expected-fuzziness with our useDrawSnapshot backend. I just need to add an annotation for that one to exclude that configuration.

(My green try run from comment 15 didn't include this test -- I added it later, and ran it through Try with a targeted ./mach try fuzzy layout/reftests/bugs/reftest.list, but apparently that runs the wrong set of tests! The reftest-snapshot test run for that try run seems to have executed a completely different set of reftests. I'll file a bug on that.)

Also, unrelatedly: jgraham, see comment 17 and comment 19 -- it looks like wpt sync bot landed the pull request here even though the patch was backed out from autoland before it ever hit central. I think that's unexpected? (In this case it's fine that the merge happened; but in other cases I could imagine it being problematic, particularly in cases where e.g. the backout might be due to issues in included WPT tests or test-changes.)

Comment 21

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #20)

Also, unrelatedly: jgraham, see comment 17 and comment 19 -- it looks like wpt sync bot landed the pull request here even though the patch was backed out from autoland before it ever hit central. I think that's unexpected? (In this case it's fine that the merge happened; but in other cases I could imagine it being problematic, particularly in cases where e.g. the backout might be due to issues in included WPT tests or test-changes.)

Ah, it looks like the wpt-sync-bot did end up backing out:
https://github.com/web-platform-tests/wpt/commit/b0329d69f0d3c43017287e586a00413d0cc187c3

So the bot did clean up after itself, but it does seem odd that it went ahead with the merge before the commits ever hit mozilla-central. (I thought "made it to mozilla-central without being backed out" was supposed to be a blocker for wpt-sync-bot merges; but maybe I misunderstood or something changed.)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b24df9298001
part 1: Add reftest variants of some known-fuzzy WPTs, with strict fuzzy annotations, so we can find out if our underlying bug gets fixed in the future. r=layout-reviewers,jfkthame
https://hg.mozilla.org/integration/autoland/rev/3b9f8dfc5772
part 2: Annotate some SVG WPTs as having some barely-fuzzy (maxDifference=1) pixels. r=layout-reviewers,jfkthame

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40842 for changes under testing/web-platform/tests

Comment 24

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/b24df9298001
https://hg.mozilla.org/mozilla-central/rev/3b9f8dfc5772

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 26

[James Graham [:jgraham]]

Also, unrelatedly: jgraham, see comment 17 and comment 19 -- it looks like wpt sync bot landed the pull request here even though the patch was backed out from autoland before it ever hit central.

I suspect this is a case of the sync bot being bad at handling backouts that touch multiple bugs. In this case it looks like the backout got applied to the PR for #1819467, and the bot didn't notice that it also applied to this bug. I've filed https://github.com/mozilla/wpt-sync/issues/1904 on what seems to be the underlying issue here.