1514383 - Filters and clips interact poorly with WR.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

For example, when you're clipped, webrender scales the drop-shadow filter to the clipped rect, which is not fine if it has an offset.

For example, in:

  https://github.com/servo/webrender/blob/fbcfd98c16d5bd100c02add8d671d0b39746c54b/wrench/reftests/filters/filter-drop-shadow-clip.png

The bottom left corner shouldn't be visible, and should be a solid line instead.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Emilio do you want to take this one or do you think there's someone else that would be good?

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1514383 - Only clip the outermost Picture in a stacking context, since we sometimes need the inner pixels for filter rendering. r?gw

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc1dccf4bf56
Only clip the outermost Picture in a stacking context, since we sometimes need the inner pixels for filter rendering. r=gw

Comment 4

[Natalia Csoregi [:nataliaCs]]

Backed out for wrench failure on filter-drop-shadow-clip.yaml

backout: https://hg.mozilla.org/integration/autoland/rev/0cfb403edfbdc23c0d0b3a532a0b5ff7cc2fc89f

push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Cquantumrender%2Copt%2Cwebrender%2Cstandalone%2Cwebrender-linux-release%2Cwr%28wrench%29&group_state=expanded&selectedJob=223980200&revision=dc1dccf4bf566f33c7bfae692064c4b73f493916

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223980200&repo=autoland&lineNumber=2734

[task 2019-01-25T09:07:20.620Z] REFTEST reftests/filters/filter-hue-rotate-1.yaml == reftests/filters/filter-hue-rotate-1-ref.yaml
[task 2019-01-25T09:07:20.685Z] REFTEST reftests/filters/filter-hue-rotate-alpha-1.yaml == reftests/filters/filter-hue-rotate-alpha-1-ref.yaml
[task 2019-01-25T09:07:20.760Z] REFTEST reftests/filters/filter-long-chain.yaml == reftests/filters/filter-long-chain.png
[task 2019-01-25T09:07:24.116Z] REFTEST reftests/filters/filter-drop-shadow.yaml == reftests/filters/filter-drop-shadow.png
[task 2019-01-25T09:07:24.895Z] REFTEST reftests/filters/filter-drop-shadow-on-viewport-edge.yaml == reftests/filters/filter-drop-shadow-on-viewport-edge.png
[task 2019-01-25T09:07:25.444Z] REFTEST reftests/filters/blend-clipped.yaml == reftests/filters/blend-clipped.png
[task 2019-01-25T09:07:30.110Z] REFTEST reftests/filters/filter-drop-shadow-clip.yaml == reftests/filters/filter-drop-shadow-clip.png
[task 2019-01-25T09:07:30.851Z] REFTEST TEST-UNEXPECTED-FAIL | reftests/filters/filter-drop-shadow-clip.yaml == reftests/filters/filter-drop-shadow-clip.png | image comparison, max difference: 2, number of differing pixels: 5555

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06704e4ba49c
Only clip the outermost Picture in a stacking context, since we sometimes need the inner pixels for filter rendering. r=gw

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/06704e4ba49c

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This doesn't look fixed? At least http://scratchpad.io/nasty-hydrant-1127 still renders wrong.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: scene-1-0.ron

Indeed, it is still broken for real content :(

I've attached the scene.ron for the relevant content.

You can see that we create a ClipChain inside the stacking context, and that it's parented to the existing ClipChain outside the stacking context. That's really not what we want, and we explicitly separated the gecko-side clips for that reason.

Looks like that happens here: https://searchfox.org/mozilla-central/source/gfx/layers/wr/ClipManager.cpp#361

I don't really understand the ClipChain format, it appears that we have an array of clips (with associated spatial), and then we form a chain of those.

What's the distinction between clips in the same chain element, vs new chain elements? Gecko just has a single clip per chain element, so it seems like there's something extra being expressed here.

Kats, any idea what the right code here is? I think we shouldn't be looking at parent items for more clips, and just using the gecko clip chain as-is.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: dl-dump.txt

Attached display list dump for the page showing the parented clip chains.

Gecko code that tries to prevent this: https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2961

Comment 10

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

I'd have to look at this a bit more tomorrow but I think the ClipManager code you linked to in comment 8 can probably be removed (i.e. just leave parentChainId as Nothing always) now that Dzmitry's rewrite has landed. I think WR probably has all the right information and most of the time that parentChainId will just result in redundant clips. In some cases (apparently this one) it might produce wrong behaviour now.

Comment 11

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)

I'd have to look at this a bit more tomorrow but I think the ClipManager code you linked to in comment 8 can probably be removed (i.e. just leave parentChainId as Nothing always) now that Dzmitry's rewrite has landed.

Doing this fixes the test case but breaks other things. I'm investigating more.

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=96115700480f96859c675208311dfaec5c538c89 is better, but still failing one reftest. It appears to be because the sticky frame API doesn't allow us to specify a clip id, and so the current implementation relies on stuff inside the sticky frame chaining together the clip chain from outside the sticky frame, and my patch stops that from happening. I'll see if updating the sticky frame API fixes this, or uncovers other things.

Comment 13

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

I created bug 1523776 to track this issue.