1359242 - Enable WR nsDisplayBackgroundImage image type by default

Status: RESOLVED FIXED

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

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1351242 +++

The slowest display item type from local profiling is nsDisplayBackground with the image type. This usually goes through the fast path at [1] as well.

[1] http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6809

Let's get this turned on by default in WR.

Comment 1

[Ethan Lin[:ethlin]]

I sent a try[1] with the pref on for image type. There are many image disappeared cases and I cannot reproduce on my local device. I think one reason is that we don't handle draw result correctly (bug 1359360). With the patch in bug 1359360, I still got this kind failures[2], but seems to be fewer. For other failures, it looks like there are many clipping/position/transform problems.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=182fe9e1046d0ee4612ad4a3df3e579b35de2dbc&selectedJob=93906660
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=14515bb83ceb990a7e6ec3baa693d721d2a76d6f&selectedJob=94011582

Comment 2

[Ethan Lin[:ethlin]]

This is a new try result with the patches in bug 1360112 and bug 1360127. Please ignore the assertion in debug build.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea02c1b9f9b75d3b0b30ccb4224b9cdf8bea2d67&selectedJob=94767023

The try result is much better than the result in comment 1, though still lots of failures. So far as I know, we still have problems about masking, transform, position and image loading. To avoid duplicated work, I suggest that we could create a bug first when looking into an issue.

About masking, I fixed mask layer position for WebRenderXXXLayer in bug 1359314 and bug 1354464. But it looks like the position is still wrong for WebRenderDisplayItemLayer since the stacking context is slightly different from WebRenderXXXLayer's.

Comment 3

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

(In reply to Ethan Lin[:ethlin] from comment #2)
> About masking, I fixed mask layer position for WebRenderXXXLayer in bug
> 1359314 and bug 1354464. But it looks like the position is still wrong for
> WebRenderDisplayItemLayer since the stacking context is slightly different
> from WebRenderXXXLayer's.

This is probably because WebRenderDisplayItemLayer doesn't actually push a stacking context while the other WebRenderXXXLayers do. I'm working on some cleanup in bug 1359842 which should make all this code a bit more easy to understand and spot what's going wrong here.

Comment 4

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

I think the code in BuildWrMaskLayer is wrong. I modified to what I think is more correct. The try push is green:

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

Feel free to try it out and see if it helps fix the issues you were seeing. Although if you're seeing problems in WebRenderDisplayItemLayer then maybe my patch won't help because it only changes the codepath where the "aUnapplyTransform" argument is true.

Comment 5

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

I think the clip computed in WebRenderDisplayItem might be wrong as well. Here's a try push where I think I corrected it:

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

Comment 6

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

I also kicked off a try push with your patches plus mine, to see what the results look like:

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

Comment 7

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> I think the code in BuildWrMaskLayer is wrong. I modified to what I think is
> more correct. The try push is green:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3eae8ab0778b436419178c39c664d8aa2bebf629
> 
> Feel free to try it out and see if it helps fix the issues you were seeing.
> Although if you're seeing problems in WebRenderDisplayItemLayer then maybe
> my patch won't help because it only changes the codepath where the
> "aUnapplyTransform" argument is true.

I tried the patch but it will cause the test[1] failed on MacOS, though looks like it works well on linux. We may also need to check some reftests with the color layer pref on, since we did some fix for those reftest. Maybe we could create another bug for mask layer and make everything correct there.

[1] layout/reftests/backgrounds/attachment-local-clipping-color-4.html

Comment 8

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

I filed bug 1361356 and bug 1361357 for the two clipping issues above. We can discuss on those bugs.

Comment 9

[Ethan Lin[:ethlin]]

Bug 1364830 will fix the image disappearing issue. The new try result is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30cc3fb4f2469e2b446390841213de93777b0a9e&selectedJob=99068440. Without the image disappearing issue, it should be easier to identify problems.

Comment 10

[Ethan Lin[:ethlin]]

Mason, are you still working on this issue? I'm going to turn on more prefs by default. Could I take this bug if you don't plan to do this soon?

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to Ethan Lin[:ethlin] from comment #10)
> Mason, are you still working on this issue? I'm going to turn on more prefs
> by default. Could I take this bug if you don't plan to do this soon?

Sure thing, thanks for taking this on!

Comment 12

[Ethan Lin[:ethlin]]

Update the try result with the fix in bug 1388634:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2508492ef29a985de1ecb226cf5bd81326bbf191&selectedJob=122512521

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Bug 1359242 - Enable background image layer by default.

Review commit: https://reviewboard.mozilla.org/r/173662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173662/

Comment 14

[Ethan Lin[:ethlin]]

Attachment: Bug 1359242 - Disable the support of gradient.

Review commit: https://reviewboard.mozilla.org/r/173664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173664/

Comment 15

[Ethan Lin[:ethlin]]

Some tests are passed on layers-free mode. For the first version, I think we should disable gradient support since border doesn't support gradient neither.

Comment 16

[Ethan Lin[:ethlin]]

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

I think we could add fuzzy for most of the failures. If other tests are passed in layers-free mode, we could set fails-if for now.

Comment 17

[Ethan Lin[:ethlin]]

Kevin, please check each test with layers-free before looking into or adding fuzz. For those tests passed with layers-free, please add "pref(gfx.webrender.layers-free,true) skip-if(!webrender)" annotation and set 'fails-if(webrender)' for the original tests.

Comment 18

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

(In reply to Ethan Lin[:ethlin] from comment #17)
> For those tests passed with layers-free, please add
> "pref(gfx.webrender.layers-free,true) skip-if(!webrender)" annotation and
> set 'fails-if(webrender)' for the original tests.

I think it's probably better to just change the test to only run in layers-free. i.e. just add the pref(gfx.webrender.layers-free,true) annotation. At this point we don't care if we lose test coverage for layers-full mode.

Comment 19

[Kevin Chen[:kechen] (UTC + 8)]

Some failures of the tests in comment 16 disappear by adding pref(gfx.webrender.layers-free,true) in from of them.

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

Comment 20

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

New try push since we haven't done one in a while: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce46bdc29a1bd8c46f0935bdf194eabb5c25ff8

This is based on current autoland tip so it has layers-free on by default as well as the reftest.list cleanups from bug 1403559.

Comment 21

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

A lot of unexpected-pass results which is great. Most of the failures seem to be fuzzable differences in gradient rendering.

Comment 22

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

I went through all the failures and sorted them into fuzzy vs fails. Most were fuzzy, but the R3/R4 ones in the bugs/ folder I classified as fails.

Here's a try push with the annotations in place: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7ef154930b86852b376feb8e5351759ad4e3f7

Comment 23

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

Whoops, had a typo in that one so one test didn't get annotated properly. And the R4 failure was already fixed on inbound. New try push:

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

Comment 24

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

Attachment: Bug 1359242 - Enable background-image layers by default in webrender.

Review commit: https://reviewboard.mozilla.org/r/184606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184606/

Comment 25

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

Attachment: Bug 1359242 - Update reftest annotations.

Review commit: https://reviewboard.mozilla.org/r/184608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184608/

Comment 26

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

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

^ rebased on autoland tip, which includes bug 1395501.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8913205 [details]
Bug 1359242 - Update reftest annotations.

https://reviewboard.mozilla.org/r/184608/#review189776

Comment 28

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

Try push in comment 26 is not useful here because bug 1395501 got backed out. I retriggered reftest jobs on the try push in comment 23 to make sure it doesn't have intermittents.

Comment 29

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

Comment on attachment 8913204 [details]
Bug 1359242 - Enable background-image layers by default in webrender.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184606/diff/1-2/

Comment 30

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

Comment on attachment 8913205 [details]
Bug 1359242 - Update reftest annotations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184608/diff/1-2/

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8913204 [details]
Bug 1359242 - Enable background-image layers by default in webrender.

https://reviewboard.mozilla.org/r/184606/#review189806

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62f5be86a33c
Enable background-image layers by default in webrender. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/0e83790a93a6
Update reftest annotations. r=jrmuizel

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62f5be86a33c
https://hg.mozilla.org/mozilla-central/rev/0e83790a93a6