Turn on image layer by default

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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
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
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.
Attachment #8875592 - Flags: review?(bugmail)
Attachment #8875593 - Flags: review?(bugmail)
Posted patch Add fuzzy-if (obsolete) — Splinter Review
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
Attachment #8875602 - Flags: review?(bugmail)
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.
Attachment #8875593 - Flags: review?(bugmail) → review?(mstange)
(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 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.
Attachment #8875602 - Flags: review?(bugmail) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Dzmitry, is it possible that WR's tiling code produces nondeterministic results? See previous comment.
Flags: needinfo?(kvark)
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
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) ?
Flags: needinfo?(bugmail)
See Also: → 1366097, 1368776
(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.
(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)
Flags: needinfo?(bugmail)
URL: 1183378
See Also: → 1183378
> 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.
Flags: needinfo?(kvark) → needinfo?(nical.bugzilla)
(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).
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).
Flags: needinfo?(nical.bugzilla)
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 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.
Attachment #8875593 - Flags: review?(mstange) → review+
(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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Posted patch Add fuzzy-ifSplinter Review
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
Attachment #8875602 - Attachment is obsolete: true
Attachment #8879467 - Flags: review?(bugmail)
Comment on attachment 8879467 [details] [diff] [review]
Add fuzzy-if

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

LGTM, thanks!
Attachment #8879467 - Flags: review?(bugmail) → review+
Pushed by ethlin@mozilla.com:
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
sorry had to back this out, seems this cause https://treeherder.mozilla.org/logviewer.html#?job_id=108737869&repo=mozilla-inbound
Flags: needinfo?(ethlin)
Backout by cbook@mozilla.com:
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
It should be similar problem to bug 1370908. kats, should I add a fuzzy-if for this reftest or wait for WR fix?
Flags: needinfo?(ethlin) → needinfo?(bugmail)
fuzzy-if is fine, r=me
Flags: needinfo?(bugmail)
Pushed by ethlin@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.