Closed Bug 1524385 Opened 7 months ago Closed 7 months ago

Set the current clip chain on the stacking context item

Categories

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

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: kats, Assigned: gw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

Attached patch hack (obsolete) — Splinter Review

The backstory here as I know it starts with bug 1514383 comment 8. In the testcase there is an element inside a stacking context and the clip chain for that element has a parent link to a clip chain outside the stacking context. This happens on the gecko side in ClipManager.cpp for historical reasons but is not in line with Gecko semantics.

Really what we want to be doing is dropping all those parent links in the clip chains, and instead ensuring that any enclosing stacking context has the clip chain that is relevant for that stacking context. This effectively means that every stacking context will have a clip chain. However doing so will trigger the isolation code path in WR which will produce perf regressions and possibly has correctness problems.

I'm attaching a patch Emilio tried yesterday where he saw correctness problems with picture caching enabled (about:home was rendering with a vertical mirror effect, among other things), although he said they mostly went away with picture caching disabled.

(Note that the hack patch applies without the patches from bug 1523776 - those patches are in the process of getting backed out from central)

With this patch bug 1523080 also seems to render correctly. I was able to reproduce severe artifacts with picture caching enabled that went away with picture caching disabled. I also did a try push with this patch and picture caching disabled, to see what else might need fixing up on the gecko side. That's at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c3aa2d67cb3513ed8651a81251bd7184414285f2

I added a WIP patch to this bug of roughly what I expect the WR changes to be.

In normal browsing, I haven't noticed any problems. I think this also fixes the bug in https://bugzilla.mozilla.org/show_bug.cgi?id=1523080, but I didn't look really closely at it - I might be missing something?

I did a try run [1] with the Gecko patch above + the WR changes. It does fail a few tests (the same tests on each platform, so there's not as many failures as it looks).

I will start looking at the reftest failures in the morning. I figured I'd post the WIP patch here just in case you have time to look at this overnight (I'm not sure if the failures are an issue with the input DL from Gecko, or a WR bug in the patch above yet).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9bc0c86812cebfd350dc75204ddf983b80518ce

Flags: needinfo?(kats)

Thanks! I'm looking into the failures.

So far, the mask-layer* ones are a problem on the gecko side. The nsDisplayOwnLayer::CreateWebRenderCommands should always create a SC instead of just for scrollthumb layers. I have a local patch to fix that.

The 906199-1.html failure I think is on the WR side. The SC in question has both preserve-3d and a clip, so maybe the BlitReason bitmask is dropping one of the flags somewhere, or maybe there's a semantics mismatch between WR and Gecko.

Looking into the others.

Flags: needinfo?(kats)
Attached patch Fix nsDisplayOwnLayer (obsolete) — Splinter Review

This is the local patch I mentioned in the previous comment. It also fixes the fixed-pos-scrolled-clip-* failures. The only other UNEXPECTED-FAIL is 1081185-1.html which appears to be a change in fuzz numbers so it's not a real failure.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87715ef08893ea6f40e02d7b56ff9956b9c0d32d

Assignee: nobody → kats

Argh, didn't mean to assign this to myself.

Assignee: kats → nobody

Fixed the bug related to 906199-1.html and rebased the patch.

Kicked off a pending try with the rebased patch + the Gecko fix from kats above:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d157a2948cd9c15440a18d2865c06bd2fd9679a

Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2853480ed90d
Set the current clip chain on the stacking context item. r=kvark
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee: nobody → gwatson
Comment on attachment 9040542 [details] [diff] [review]
hack

Obsoleting this WIP patch
Attachment #9040542 - Attachment is obsolete: true

Making this a P3 since it is a fix for bug 1523776 which is a P3. So this probably warrants an uplift to 66.

Priority: -- → P3
Depends on: 1525740

Would you like to request uplift to beta?

Flags: needinfo?(kats)
Flags: needinfo?(gwatson)

And if so would that also need the work in bug 1523776 and bug 1525740?

I'm hesitant now since it's pretty late in beta and the patch seems pretty big to me. I'm leaning towards not uplifting but Glenn probably has a better sense of how risky the patch is. And yes, if we do upload this we'd probably want to upload bug 1525740 and bug 1523776 as well.

Flags: needinfo?(kats)

I agree it does introduce some risk, I'm also not sure there's a massive benefit to having this in beta. I'd lean towards letting it settle in nightly and go into the next beta, I think. Jeff may have an opinion also.

Flags: needinfo?(gwatson) → needinfo?(jmuizelaar)

I'm fine with that strategy

Flags: needinfo?(jmuizelaar)

Great thanks! That's what I like to hear about this time in the beta cycle :)

Depends on: 1531170
You need to log in before you can comment on or make changes to this bug.