1367987 - Turn on image layer by default

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Ethan Lin[:ethlin]]

Based on the try result[1], most of the reftest failures are about rounding, snapping issue. We could set fuzz for some of the failures.  

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=70306b9f7df45c1f1713c1824678ef549e0c0c34&selectedJob=100832979

Comment 1

[Ethan Lin[:ethlin]]

New try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce6d4194e192989b6f43b91258739ffb58124066&selectedJob=104793306
Looks like we only need to fix downscale problem in R1 and it's should be about snapping issue in webrender though the snap changes were just be reverted[1].

[1] https://github.com/servo/webrender/commit/b2614e4eb58f9dee08b8c38f96bc3bac834c837b

Comment 2

[Ethan Lin[:ethlin]]

After investigating, the downscale issue is because for some images, we shouldn't convert them to image layers. We already have some checks in CanOptimizeToImageLayer, and I should reuse the function.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Enable image layer by default

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Check 'CanOptimizeToImageLayer'

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Add fuzzy-if

There are some intermittent fuzzy-if so I set their range as '0-xxx'. And it looks we may get different fuzz numbers in different runs. For instance, I got different differing pixels in Ru3[1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e69db08b8af908eed703351218bca7122f623c13&selectedJob=105368880

Comment 6

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

Comment on attachment 8875593 [details] [diff] [review]
Check 'CanOptimizeToImageLayer'

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

Passing this review to Markus, I'm not that familiar with this code. It seems like it runs in non-webrender codepaths too.

Comment 7

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

(In reply to Ethan Lin[:ethlin] from comment #5)
> There are some intermittent fuzzy-if so I set their range as '0-xxx'. And it
> looks we may get different fuzz numbers in different runs. For instance, I
> got different differing pixels in Ru3[1].
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e69db08b8af908eed703351218bca7122f623c13&selectedJob=1
> 05368880

This seems bad - the results are nondeterministic which means we must have some sort of race condition going on. Do we know why this is happening?

/cc Andrew as well in case he has run into this problem, it might be something in imagelib?

Comment 8

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

Comment on attachment 8875602 [details] [diff] [review]
Add fuzzy-if

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

I'd like to spend a little bit of time first trying to figure out why the results are nondeterministic. If we can't get to the bottom of it in a couple of days then I'll probably r+ anyway but we should at least try a little.

Comment 9

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Comment on attachment 8875602 [details] [diff] [review]
> Add fuzzy-if
> 
> Review of attachment 8875602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to spend a little bit of time first trying to figure out why the
> results are nondeterministic. If we can't get to the bottom of it in a
> couple of days then I'll probably r+ anyway but we should at least try a
> little.

I think it may be related to Webrender tiling, though I'm not familiar with that part. We can see the reftest-analyzer result[1]. If we select 'Circle differences', it seems the different pixels are on the edge of tiles. We pass the whole image to Webrender from Gecko, so the race condition probably happens when WebRender processes the tiling data in parallel. 

[1] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/FWcitgCiTTu9HsChAVaMTQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 10

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

Dzmitry, is it possible that WR's tiling code produces nondeterministic results? See previous comment.

Comment 11

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

Hm actually looking at [1] we can see the differences are on pixel rows 86, 197, 313, 424, 581, 692, 808, and 919. That doesn't really seem like tile boundaries to me, although it might be rounding errors in the gradient calculations? Also the Ru3 failures (385823-2a.html and 385823-2c.html) in the try run from comment 5 don't seem to have the same "tile" pattern, that seems more random.

[1] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/OWv7efx7TFODjBAOVTu8oQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 12

[Dzmitry Malyshau [:kvark]]

If snapping was involved, does the issue persist if you take WR after the snapping code got reverted (https://github.com/servo/webrender/pull/1339) ?

Comment 13

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

(In reply to Ethan Lin[:ethlin] from comment #2)
> After investigating, the downscale issue is because for some images, we
> shouldn't convert them to image layers. We already have some checks in
> CanOptimizeToImageLayer, and I should reuse the function.

FYI, I will be fixing any downscale related issues in bug 1366097 and bug 1368776. The attached patches are in working order if needed for testing purposes.

Comment 14

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

(In reply to Dzmitry Malyshau [:kvark] from comment #12)
> If snapping was involved, does the issue persist if you take WR after the
> snapping code got reverted (https://github.com/servo/webrender/pull/1339) ?

The try push in question is using WR cset b2614e4eb58f9dee08b8c38f96bc3bac834c837b which has the snapping code reverted. I don't know if the results are different with the snapping patches applied (also not sure if those are going to reland)

Comment 15

[Dzmitry Malyshau [:kvark]]

> Dzmitry, is it possible that WR's tiling code produces nondeterministic results? See previous comment.

I don't think there is any non-determinism on the CPU code side, so here is what we do on the GPU side.
When tiling an image, we draw the chunks as separate image instances. They share snapping region, so snapping shouldn't be affected by tiling. They do, however, clamp they UV coordinates separately. So, if you have any sort of transform, even a basic downscale, then automatic tiling of that image would produce different rendering result depending on the tile edges, because the renderer will not be able to interpolate across the tiles. If there is no transform, and the image is 1x1 on screen, there should be no determinism, in theory.

Nicolas can double-check me here, since he implemented a lot of tiling code of WR.

Comment 16

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

(In reply to Dzmitry Malyshau [:kvark] from comment #15)
> > Dzmitry, is it possible that WR's tiling code produces nondeterministic results? See previous comment.
> 
> I don't think there is any non-determinism on the CPU code side, so here is
> what we do on the GPU side.
> When tiling an image, we draw the chunks as separate image instances. They
> share snapping region, so snapping shouldn't be affected by tiling. They do,
> however, clamp they UV coordinates separately. So, if you have any sort of
> transform, even a basic downscale, then automatic tiling of that image would
> produce different rendering result depending on the tile edges, because the
> renderer will not be able to interpolate across the tiles. If there is no
> transform, and the image is 1x1 on screen, there should be no determinism,
> in theory.
> 
> Nicolas can double-check me here, since he implemented a lot of tiling code
> of WR.

Hm, good to know! I dismissed downscale-on-decode as being related because both sides were being done by WR, but if the above is true and the root cause, then the reftests should pass if you apply the patches from bug 1368776 and bug 1183378 (and bug 1366097 if there are SVG-related failures).

Comment 17

[Nicolas Silva [:nical]]

Dzmitry summed it up well, the tiling only depends on the resolution of the image being tiled (and a tile size if specified, which in our case is always the default 512x512).

Comment 18

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

FWIW this "pixel differences aligned with tile-edges" type problem is already happening intermittently on layout/reftests/bugs/1242172-2.html, and is tracked by bug 1370908. Here's a sample reftest analyzer link: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/XTKBc9wARHO-bdSiopegPg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1 from m-c revision 981da978f1f686ad024fa958c9d27d2f8acc5ad0.

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8875593 [details] [diff] [review]
Check 'CanOptimizeToImageLayer'

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

Seems reasonable. Which check in CanOptimizeToImageLayer are you interested in, here? Many of the checks aren't that relevant because for example nsImageFrame doesn't support repeated images. But it definitely makes sense to share the code.

Comment 20

[Ethan Lin[:ethlin]]

(In reply to Markus Stange [:mstange] from comment #19)
> Comment on attachment 8875593 [details] [diff] [review]
> Check 'CanOptimizeToImageLayer'
> 
> Review of attachment 8875593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems reasonable. Which check in CanOptimizeToImageLayer are you interested
> in, here? Many of the checks aren't that relevant because for example
> nsImageFrame doesn't support repeated images. But it definitely makes sense
> to share the code.

It seems to be the check of scale width/height[1] to let it return false.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#4057

Comment 21

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> FWIW this "pixel differences aligned with tile-edges" type problem is
> already happening intermittently on layout/reftests/bugs/1242172-2.html, and
> is tracked by bug 1370908. Here's a sample reftest analyzer link:
> https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/
> XTKBc9wARHO-bdSiopegPg/runs/0/artifacts/public/logs/live_backing.
> log&only_show_unexpected=1 from m-c revision
> 981da978f1f686ad024fa958c9d27d2f8acc5ad0.

All platforms in bug 1370908 seem to be test-linux64-qr[1]. I guess it's a regression from bug 1354463 since after creating the layer of color/canvas-bg-color, nsDisplayImage is easier to be optimized as an image layer. Morris is working on this issue.

[1] https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1370908&startday=2017-06-05&endday=2017-06-11&tree=all

Comment 22

[Ethan Lin[:ethlin]]

Attachment: Add fuzzy-if

The random different number problem was identified in bug 1370908. After rebase, it seems that we only need some fixed fuzzy values.
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=346095c97402f189e11054f677ac8e5c6a654aff&selectedJob=108439007

Comment 23

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

Comment on attachment 8879467 [details] [diff] [review]
Add fuzzy-if

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

LGTM, thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e38bcd123b
Enable image layer by default when webrender is enabled. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe074cf9ebf
Check 'CanOptimizeToImageLayer' to determine whether we should create image layer. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e6fcbd8642
Add fuzzy-if to tests for webrender to make the try result passed. r=kats

Comment 25

[Carsten Book [:Tomcat]]

sorry had to back this out, seems this cause https://treeherder.mozilla.org/logviewer.html#?job_id=108737869&repo=mozilla-inbound

Comment 26

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e00859e67c6
Backed out changeset 64e6fcbd8642 
https://hg.mozilla.org/integration/mozilla-inbound/rev/83fc0ce7e848
Backed out changeset 8fe074cf9ebf 
https://hg.mozilla.org/integration/mozilla-inbound/rev/15360254c12d
Backed out changeset 08e38bcd123b for causing new intermittent failure in reftests/image/image-seam-2.html

Comment 27

[Ethan Lin[:ethlin]]

It should be similar problem to bug 1370908. kats, should I add a fuzzy-if for this reftest or wait for WR fix?

Comment 28

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

fuzzy-if is fine, r=me

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4c5ebd2966
Enable image layer by default when webrender is enabled. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c690a90bce2
Check 'CanOptimizeToImageLayer' to determine whether we should create image layer. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa10d4a66cfc
Add fuzzy-if to tests for webrender to make the try result passed. r=kats

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de4c5ebd2966
https://hg.mozilla.org/mozilla-central/rev/7c690a90bce2
https://hg.mozilla.org/mozilla-central/rev/fa10d4a66cfc