Fix mask reftest failures for layers-free mode

RESOLVED FIXED in Firefox 57

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

5 months ago
The wr mask doesn't work in certain mask tests and svg mask tests[1] if layers-free mode is enabled. I haven't identify the exact problem. But it looks like the color format is wrong. I will investigate this issue.


[1] layout/reftests/border-radius/corner-joins-1.xhtml
    layout/reftests/invalidation/mask-invalidation-1a.html
    layout/reftests/css-animations/mask-position-after-finish-1a.html
(Assignee)

Comment 1

5 months ago
After some study, it's not color format problem. It's a regression of bug 1386483. I should pass the WR mask to the ScrollingLayersHelper. I'll have a patch for it.
Comment hidden (mozreview-request)
I experimented a little with the mask clip in webrender/examples/basic.rs, and it looks like the result should be the same if the clip is pushed as a separate mask or as part of the nested clips. So I feel like this patch only works by accident and doesn't fix the root of the problem. (And even then, it doesn't seem to fix corner-joins-1.xhtml, at least for me locally - it fixes the other two tests you listed).

I think the root of the problem is that when we push the mask clip in nsDisplayMask, we change the topmost clip id in the mClipIdStack [1]. Later, when we check the TopmostClipId() at [2] we don't find the clip we should be finding, and so that causes us to push a whole new stack of clips which screws things up.

I think a better fix is to just not update mClipIdStack when we're pushing the mask clip from nsDisplayMask, as it can be considered an "out of band" clip that's pushed and that ScrollingLayersHelper has no knowledge about. Alternatively instead of just checking the TopmostClipId() at [2] we can check the entire stack of pushed clips to see if the clip is contained anywhere. I'm leaning towards the first solution. I checked and it seems to give me the same results as your patch.

[1] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/gfx/webrender_bindings/WebRenderAPI.h#326
[2] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/gfx/layers/wr/ScrollingLayersHelper.cpp#136
Comment on attachment 8898223 [details]
Bug 1390804 - Apply WR mask to ScrollingLayersHelper for layers-free.

Although this patch works, I think it unnecessarily propagates the mask to multiple clips. In other words, if a child display item of the nsDisplayMask has a long clip chain, then all of those clips will end up defined with the mask which seems inefficient. Also if those same DisplayItemClipChain items are later re-used elsewhere in the display list they will still have the mask which might be undesirable. I have another patch which I think solves this problem better.
Attachment #8898223 - Flags: review?(bugmail) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
mozreview-review
Comment on attachment 8898388 [details]
Bug 1390804 - When pushing a mask clip, don't record it in DisplayListBuilder's clip stack.

https://reviewboard.mozilla.org/r/169782/#review175136

I didn't know the mechanism of clip chain well. I review the whole process again and I think I understand how it works now. Thanks for your explanation and the patch!
Attachment #8898388 - Flags: review?(ethlin) → review+

Comment 7

5 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ceb5ec58738
When pushing a mask clip, don't record it in DisplayListBuilder's clip stack. r=ethlin
(Assignee)

Updated

5 months ago
Blocks: 1391541

Comment 8

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ceb5ec58738
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.