1390804 - Fix mask reftest failures for layers-free mode

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Ethan Lin[:ethlin]]

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

Comment 1

[Ethan Lin[:ethlin]]

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 2

[Ethan Lin[:ethlin]]

Attachment: Bug 1390804 - Apply WR mask to ScrollingLayersHelper for layers-free.

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

Comment 3

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

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 4

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

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.

Comment 5

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

Attachment: Bug 1390804 - When pushing a mask clip, don't record it in DisplayListBuilder's clip stack.

Recording mask clips in the clip stack changes the value of TopmostClipId()
which confuses the code in ScrollingLayersHelper. The mask clip can be
thought of as an "out-of-band" clip that ScrollingLayersHelper doesn't
need to know about. This patch adds a mechanism for pushing such
"out-of-band" clips without touching the clip stack.

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

Comment 6

[Ethan Lin[:ethlin]]

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!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ceb5ec58738
When pushing a mask clip, don't record it in DisplayListBuilder's clip stack. r=ethlin

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/2ceb5ec58738