1398706 - Using mask and APZ may produce wrong clipping region

Status: RESOLVED FIXED

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

Description

[Ethan Lin[:ethlin]]

The testcase is "./layout//reftests/border-radius/corner-joins-1.xhtml".

Comment 1

[Ethan Lin[:ethlin]]

Disabling APZ or removing mask could make the test passed or correcter. kats, any idea?

Comment 2

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

Not off the top of my head. I can look into it once I'm done with position:sticky stuff if you want.

Comment 3

[Ethan Lin[:ethlin]]

I guess I probably know what happens. Let me try.

Comment 4

[Ethan Lin[:ethlin]]

I think the problem is that the mask item doesn't counted as a clip chain. For example:

C1 - C2  - ItemA
         \ Mask - ItemB

Item A and item B have the same clip chain. When we push item A, we will cache the C1 and C2. Before we push Item B, we will push mask clip first. But we will push the cached C2 for item B and the cached C2's parent is C1, not mask. I have a workaround that always create new clip id if the item is under a mask clip. I will upload my workaround first.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Bug 1398706 - Always create new clip ids in ScrollingLayersHelper if it's inside a mask.

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

Comment 6

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

Ah, interesting. This seems to be related to the problem I'm investigating in bug 1400982. I had an idea in mind to fix that one, I'll take a closer look at this as well to see if my solution for that will fix this or if it needs some more tweaking.

Comment 7

[Ethan Lin[:ethlin]]

"layout/reftests/svg/mask-extref-dataURI-01.svg" is another testcase which is related to this problem.

Comment 8

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

I looked at the corner-joins-1-ref.xhtml page to understand it better. Here's a quick summary:

The display list for the bottom-left blob looks like this:

Mask ... asr(A) clipChain(<R> [root asr]) // from #topclip
  ...
  nsDisplaySVGGeometry asr(A) clipChain(<C> [A], <R> [root asr])
Mask ... asr(A) clipChain(<R> [root asr]) // from #botclip
  ...
  nsDisplaySVGGeometry asr(A) clipChain(<C> [A], <R> [root asr])

So when we process the #topclip mask, we make the clip C a child of that mask. Later when we process the #botclip mask, we create a new clip for it, but then when we process the child nsDisplaySVGGeometry, we have the exact same clip chain item C, and so we push C onto the stack. However C is already defined as having a parent #topclip, so that's what WR ends up using, instead of #botclip.

Ethan's patch fixes this by ensuring that we don't cache the clip id for C the first time we create it (because we're inside a mask) and so we create a new clip id for the "second" C (which also will not get cached). This in effect creates separate clips inside WR, one inside #topclip and one inside #botclip, which solves the problem.

This appears to be a different problem than bug 1400982. At least I don't think there's a way to unify the solutions to both problems.

Comment 9

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

Ironically in https://bugzilla.mozilla.org/show_bug.cgi?id=1390804#c4 I had thought of this problem ("Also if those same DisplayItemClipChain items are later re-used elsewhere in the display list they will still have the mask") with your original patch but I failed to realize that my patch had a different variant of the same problem :p

I wonder if it might be better to roll back the aRecordInStack change from bug 1390804 and instead go with the other suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=1390804#c3. I'm trying to think if that will help but right now my brain is not co-operating so it might have to wait until tomorrow.

Comment 10

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

I kicked off a layers-free try push with your patch, just to see if it breaks or fixes other tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9e60ae8d370398b63f723e3a98f09e49541ed4

Comment 11

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

I also have a baseline push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3cc0fdba058273b5770432b7db8258072a04f35. 

Baseline: UNEXPECTED-FAIL: 294
          UNEXPECTED-PASS: 33
Modified: UNEXPECTED-FAIL: 249
          UNEXPECTED-PASS: 29

I think that's a pretty good win. It looks like there's a handful of tests that are failing when they weren't before but on the whole this fixes a lot of things.

I also spent some time thinking about different ways to fix this problem and I couldn't come up with anything that I thought would be strictly better than what you have now. So I'd be ok with landing what you have now and then if we come up with something later we can tweak it.

Comment 12

[Ethan Lin[:ethlin]]

Okay, I'll upload a patch for review.

Comment 13

[Ethan Lin[:ethlin]]

Comment on attachment 8909714 [details]
Bug 1398706 - Always create new clip ids in ScrollingLayersHelper if it's inside a mask.

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

Comment 14

[Ethan Lin[:ethlin]]

Comment on attachment 8909714 [details]
Bug 1398706 - Always create new clip ids in ScrollingLayersHelper if it's inside a mask.

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

Comment 15

[Ethan Lin[:ethlin]]

Comment on attachment 8909714 [details]
Bug 1398706 - Always create new clip ids in ScrollingLayersHelper if it's inside a mask.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181234/diff/3-4/

Comment 16

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

Comment on attachment 8909714 [details]
Bug 1398706 - Always create new clip ids in ScrollingLayersHelper if it's inside a mask.

https://reviewboard.mozilla.org/r/181234/#review187686

Comment 17

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

Feel free to land this if it has a green try push on current tip. It will affect the results for bug 1389000 but I think we need to sort out the nondeterminism there before we can land that anyway, so no point in waiting.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/804199553c13
Always create new clip ids in ScrollingLayersHelper if it's inside a mask. r=kats

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/804199553c13