Closed Bug 1377187 (clipping-rewrite) Opened 7 years ago Closed 6 years ago

Make layout/reftests/async-scrolling/fixed-pos-scrolled-clip-2.html pass on linux64-qr with APZ enabled

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox62 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1377187 - Simplify code to define clips and scroll layers.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1377187 - Rewrite the clipping code to use the new clipchain API.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
With WR+APZ enabled, the layout/reftests/async-scrolling/fixed-pos-scrolled-clip-2.html reftest fails. This bug is for the patches that make it pass, without regressing any "earlier" tests.
This case is currently not supported by WR, so we'll have to wait for some upstream changes before this can be fixed (see bug 1373802 for some discussion).
Assignee: bugmail → nobody
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22e70b0afd087dda6d9eab509b4d8081d067fcca

^ has my WIP patches to redo the gecko-side WR clipping glue to use the new ClipChain API that martin added in WR. It mostly works, there's still a few unexpected-failures that i need to debug. Also some new unexpected-passes which is gratifying to see.

One of the things that needs to be fixed on the WR side still is servo/webrender#2383 (which is why I disabled WR hit-testing in my try push).
Assignee: nobody → bugmail
Markus, sorry for the megapatch in part 2. I did try to split it a bit but it was getting really tricky to have incrementally compiling changes. If you want me to try harder to split it up into smaller patches let me know and I can do that. Part of the reason I renamed ScrollingLayersHelper to ClipManager is because the diff between the two is not really meaningful. Although some code was reused, it's better to just treat ClipManager as new "from scratch" code. My suggestion for reviewing that second patch would be to do this:

- Read the comments on data structures in ClipManager.h
- Read through ClipManager.cpp mostly ignoring the mASROverride stuff and get a handle on the basic flow of it
- Look at the changes to the rest of the files. They're a mix of different things that are mostly side-effects of the main change in ClipManager. If the main flow in ClipManager makes sense then these changes should be easier to follow. Note the places that the ASR override mechanism is used
- Read through ClipManager.cpp again to ensure the mASROverride stuff makes sense


More comprehensive try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59e6a61405a3ca62eb33eb5614399dc1884e8cba
Somehow the combination of these patches and bug 1451168 resulted in some reftests failing, in ways that look fuzzable. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4f4add1ada8f38458995ed3ea3896b8be0af4ea is the try push with the failures, I'll wait for it to complete and then update these patches as necessary (likely just fiddling with the fuzzy annotations).
The 836844-1.html fuzziness turned out to be nondeterministic (see try push above) so I had to use a wider range to cover it all. Patch updated with those changes as well as updated some code comments to be more correct.
Alias: clipping-rewrite
Rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5de82b0341e98599db3134168a8ea66766e033d

Showing a bunch of mask failures, presumably due to interference from bug 1435094. I'm not going to bother trying to fix it until these patches are reviewed though.
Comment on attachment 8972145 [details]
Bug 1377187 - Simplify code to define clips and scroll layers.

https://reviewboard.mozilla.org/r/240838/#review247998
Attachment #8972145 - Flags: review?(mstange) → review+
Comment on attachment 8972146 [details]
Bug 1377187 - Rewrite the clipping code to use the new clipchain API.

https://reviewboard.mozilla.org/r/240840/#review248034

This looks great!

I think it would be good to have some documentation for the general shape of the clip scroll tree that we're building.
E.g. "The ancestor of a scroll layer node is never a clip node. You can imagine the tree of scroll layer nodes to be a subtree off to the side in the clip scroll tree whose shape mirrors the ASR tree's shape." I think that's accurate?

::: gfx/layers/wr/ClipManager.cpp:200
(Diff revision 2)
> +    // it directly to |asr|. This can produce incorrect results. Using the clip
> +    // instead of the ASR avoids this and ensures the item stays "inside" the
> +    // stacking context.

But only if the clip has been defined inside the stacking context, I expect? Can we be sure that the clip we have here has been defined inside the stacking context?

This seems like an odd thing to do for WR in general, though.
Attachment #8972146 - Flags: review?(mstange) → review+
Fixed the rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=930bfa577fc96f4c9caa40c6e728221435be46ac

(In reply to Markus Stange [:mstange] from comment #15)
> I think it would be good to have some documentation for the general shape of
> the clip scroll tree that we're building.
> E.g. "The ancestor of a scroll layer node is never a clip node. You can
> imagine the tree of scroll layer nodes to be a subtree off to the side in
> the clip scroll tree whose shape mirrors the ASR tree's shape." I think
> that's accurate?

Yeah, that's accurate. I'll add some documentation to that effect.

> ::: gfx/layers/wr/ClipManager.cpp:200
> (Diff revision 2)
> > +    // it directly to |asr|. This can produce incorrect results. Using the clip
> > +    // instead of the ASR avoids this and ensures the item stays "inside" the
> > +    // stacking context.
> 
> But only if the clip has been defined inside the stacking context, I expect?

Yes

> Can we be sure that the clip we have here has been defined inside the
> stacking context?

Hmm, good question. I think we can be sure of it in some cases, e.g. when the stacking context's mAffectsClipPositioning is true. It might always be true, though - and that might be the reason there are still some failing reftests. I'll think about this a bit more and see if I can construct a case that breaks this, but either way I don't think it needs to block these patches landing.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/976c2ea507b9
Simplify code to define clips and scroll layers. r=mstange
https://hg.mozilla.org/integration/autoland/rev/6a55c1fa635e
Rewrite the clipping code to use the new clipchain API. r=mstange
https://hg.mozilla.org/mozilla-central/rev/976c2ea507b9
https://hg.mozilla.org/mozilla-central/rev/6a55c1fa635e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
No longer blocks: 1508203
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: