1453935 - Make layout/reftests/bugs/{1291528.html,289480}.html pass again (repeat snapping)

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

A lot of this is being addressed on the webrender side with work separated into a number of pull requests:
 - https://github.com/servo/webrender/pull/2621
 - https://github.com/servo/webrender/pull/2644
 - https://github.com/servo/webrender/pull/2572

This work is affected by how we handle the pixel snapping of repeated primitives. The current plan is to simplify the problem described here: https://github.com/servo/webrender/pull/2644#issuecomment-380807766 by snapping both the primitive rect and the repeated rect. This is a fairly big change for repeated backgrounds with fractional repeated sizes.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Reftests annotations for the webrender update containing PR #2644

cf. https://github.com/servo/webrender/pull/2644

There is also a bit of fuzziness to add to the ACID reftest:

> REFTEST TEST-UNEXPECTED-FAIL | http://localhost:46135/1523467925296/136/289480.html#top == http://localhost:46135/1523467925296/136/289480-ref.html | image comparison, max difference: 12, number of differing pixels: 1534

but I haven't found where this is specified yet.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8967699 [details] [diff] [review]
Reftests annotations for the webrender update containing PR #2644

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

::: layout/reftests/css-gradients/reftest.list
@@ +46,5 @@
>  fails-if(gtkWidget&&/x86_64-/.test(xulRuntime.XPCOMABI)) fuzzy(1,1622) fuzzy-if(cocoaWidget,2,41281) fuzzy-if(Android,8,1091) fuzzy-if(skiaContent,2,500) == radial-shape-farthest-corner-1b.html radial-shape-farthest-corner-1-ref.html
>  fuzzy-if(Android,17,13320) == radial-shape-farthest-side-1a.html radial-shape-farthest-side-1-ref.html
>  fuzzy-if(Android,17,13320) == radial-shape-farthest-side-1b.html radial-shape-farthest-side-1-ref.html
> +fuzzy-if(webrender, 1, 4) == radial-size-1a.html radial-size-1-ref.html
> +fuzzy-if(webrender, 1, 4) == radial-size-1b.html radial-size-1-ref.html

(a) spaces are not allowed in annotations
(b) please use tight ranges like 1-1,4-4

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Nicolas Silva [:nical] from comment #1)
> > REFTEST TEST-UNEXPECTED-FAIL | http://localhost:46135/1523467925296/136/289480.html#top == http://localhost:46135/1523467925296/136/289480-ref.html | image comparison, max difference: 12, number of differing pixels: 1534
> 
> but I haven't found where this is specified yet.

https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/layout/reftests/bugs/reftest.list#308

Comment 4

[Nicolas Silva [:nical]]

Attachment: Reftests annotations for the webrender update containing PR #2644

Updated patch. I ran it locally to make sure the manifest is parsed properly.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8967708 [details] [diff] [review]
Reftests annotations for the webrender update containing PR #2644

I'll be landing this patch as part of bug 1457241 which will contain #2644, so I'll obsolete the patch here.

One thing for future reference, the change you made for 1291528.html doesn't actually work because the last clause that evaluates to true is the one that is applied, so the fuzzy(8,1900) overrides the fails-if(webrender). The webrender clause needs to be after the fuzzy to take effect. I'll make that change locally.

Also, I'll repurpose this bug to track the fix that will remove the fails-if.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Expanding this bug to also cover the tests that I'm disabling in bug 1416772, since they also started failing with high frequency from servo/webrender#2644.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Do you think this is important enough to fix for the MVP? Are we going to miss regressions with these tests disabled?

Comment 8

[Nicolas Silva [:nical]]

As far as getting these tests to pass, I think that we have a lot of more important items for the MVP.
There's one thing that I think is worth fixing before going to release is snapping repeated sizes before repeating, to make sure we avoid some cases where a line or a column of pixels get wrapped on the wrong side with certain border styles and non-integer sizes.

Comment 9

[Maire Reavy [:mreavy]]

(In reply to Nicolas Silva [:nical] from comment #8)
> As far as getting these tests to pass, I think that we have a lot of more
> important items for the MVP.
> There's one thing that I think is worth fixing before going to release is
> snapping repeated sizes before repeating, to make sure we avoid some cases
> where a line or a column of pixels get wrapped on the wrong side with
> certain border styles and non-integer sizes.

Thanks, Nical.  I agree with you.  Updating this bug to block riding to Release, not Nightly or Beta.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

At this moment, I think only layout/reftests/bug/1291528.html is failing. layout/reftests/bugs/289480.html can be re-enabled with some fuzz.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2990526f571cefcc8ed443b2864b36688d6789aa&selectedJob=220009794

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1453935 - Ensure that we snap within the visible rect with WebRender.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Added CPU side changes to mirror what is done on the GPU side (it was also incorrect using the pic clip rect instead of the prim rect, but lining them up didn't solve my original snapping problems, so... ;)).

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

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Oddly enough, it still fails one of the tests, despite it passing locally for me (with and without my usual 200% screen scaling).

try (linux): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddeb1ee67e9d27e7c9d762e4fa7c0b17d8bfa66a
try (windows, osx): https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f011077f56cb4f780f9800642caf3626f5f980

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ad4b91e29f
Ensure that we snap within the visible rect with WebRender. r=kvark

Comment 15

[Andreea Pavel [:apavel]]

Backed out for failing reftests

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=226950830&searchStr=os%2Cx%2C10.10%2Cquantumrender%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-macosx64-qr%2Fdebug-reftest-e10s-2%2Cr-e10s%28r2%29&revision=87ad4b91e29fb9025d90ceb89a62b5a4eb7d584a

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226950830&repo=mozilla-inbound&lineNumber=113416

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd03ab546fa01aa3705192b2445d7311d7ed997

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e32dd08ec5db
Ensure that we snap within the visible rect with WebRender. r=kvark

Comment 17

[Oana Pop-Rus]

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

Comment 20

[Rares Doghi, Desktop QA]

Hi, This issue seems to still occur on https://www.samsung.com/ro/ in Nightly 67.0a1 (2019-02-10), unlike before this does not happen when you move the mouse cursor on top of the watches thumbnail so they come into view, but rather when you move the mouse cursor away from the thumbnail and the watches are going back to their initial state, thats when the images are shaking a bit, this issue does not occur with webrender off.

Please check the attachment for more info.

Comment 21

[Rares Doghi, Desktop QA]

Attachment: 2019-02-11_13h44_34.mp4

Comment 22

[Rares Doghi, Desktop QA]

Please note that this issue occurs on this website as well https://news.err.ee/ as stated in the last comment of Bug 1526298

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Rares, can you file a separate bug for the issues you're still seeing.

Comment 24

[Rares Doghi, Desktop QA]

Hi Jeff, I already added a separate issue Bug 1526298 but it's been marked as Duplicate for this bug.