Closed Bug 1359242 Opened 7 years ago Closed 7 years ago

Enable WR nsDisplayBackgroundImage image type by default

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: mchang, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp])

Attachments

(2 files, 2 obsolete files)

Bug 1359242 - Enable background-image layers by default in webrender.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1359242 - Update reftest annotations.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
+++ 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.
Depends on: 1359320
Depends on: 1359360
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
Depends on: 1360112
Depends on: 1360127
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.
(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.
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 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
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
I filed bug 1361356 and bug 1361357 for the two clipping issues above. We can discuss on those bugs.
Depends on: 1362967
Depends on: 1364830
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.
Depends on: 1366984
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?
Flags: needinfo?(mchang)
Depends on: 1387353
(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!
Assignee: mchang → ethlin
Flags: needinfo?(mchang)
Depends on: 1388634
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.
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.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
Assignee: ethlin → kechen
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.
(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.
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
Depends on: 1401898
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.
A lot of unexpected-pass results which is great. Most of the failures seem to be fuzzable differences in gradient rendering.
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
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
Assignee: kechen → bugmail
Attachment #8902185 - Attachment is obsolete: true
Attachment #8902186 - Attachment is obsolete: true
Comment on attachment 8913205 [details]
Bug 1359242 - Update reftest annotations.

https://reviewboard.mozilla.org/r/184608/#review189776
Attachment #8913205 - Flags: review?(jmuizelaar) → review+
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 on attachment 8913204 [details]
Bug 1359242 - Enable background-image layers by default in webrender.

https://reviewboard.mozilla.org/r/184606/#review189806
Attachment #8913204 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: