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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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).

Comment 2

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

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).

Comment 3

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

Rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ef24f56ee59ab71675ea4021e7385230fa37b29

Comment 4

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

Rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46c327d34e522f06dc94f923f3d733e6c5d58714

Looking good so far, got some unexpected-passes

Comment 5

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

Attachment: Bug 1377187 - Simplify code to define clips and scroll layers.

This is a refactoring that moves the "parent clip id" determination from
bindings.rs into WebRenderAPI.cpp. This should be functionally
identical.

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

Comment 6

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

Attachment: Bug 1377187 - Rewrite the clipping code to use the new clipchain API.

The clip chain API in webrender allows us to build the clip state in WR
so that it matches the gecko display list more closely. This patch throws
away ScrollingLayersHelper.* and introduces ClipManager.* which pushes
the clip state to WR using the new method. A quick summary of the new
method is below.

Each display item in gecko has a DisplayItemClipChain which is a chain
of individual clips. The individual clips are defined in WR, and the
clip ids for those clips are put into a WR clip chain using the new
define_clip_chain API. Furthermore, each clip chain can also have a
parent chain, which is used to link a DisplayItemClipChain to the parent
display item's DisplayItemClipChain. This allows the WR clip state to
closely match the structure of the gecko display list clip state,
resulting in more correct behaviour.

There are a few other major changes that are lumped into this patch and
that were tricky to separate into their own patches:
- The collapsing of WrScrollId and WrStickyId into WrClipId. On the WR
  side all the clip ids are treated the same anyway. Trying to preserve
  the arbitrary distinction on the gecko side was resulting in
  increasingly convoluted code, with different kinds of Variant<..>
  types in the method signatures. It was much simpler and resulted in a
  bunch of code deletion to just collapse the types.
- Moving the "override" mechanism from WebRenderAPI to ClipManager. The
  override mechanism (explained in ClipManager.h) was simplified by
  moving it into ClipManager, because it removed the need for tracking
  additional clip stack state in WebRenderAPI.

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

Comment 7

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

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

Comment 8

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

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).

Comment 9

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

Comment on attachment 8972145 [details]
Bug 1377187 - Simplify code to define clips and scroll layers.

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

Comment 10

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

Comment on attachment 8972146 [details]
Bug 1377187 - Rewrite the clipping code to use the new clipchain API.

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

Comment 11

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

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.

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10b1d575235e85dbc6c09cb2f8d48f8330b26bf3

Comment 13

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

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 14

[Markus Stange [:mstange]]

Comment on attachment 8972145 [details]
Bug 1377187 - Simplify code to define clips and scroll layers.

https://reviewboard.mozilla.org/r/240838/#review247998

Comment 15

[Markus Stange [:mstange]]

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.

Comment 16

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

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.

Comment 17

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

Comment on attachment 8972145 [details]
Bug 1377187 - Simplify code to define clips and scroll layers.

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

Comment 18

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

Comment on attachment 8972146 [details]
Bug 1377187 - Rewrite the clipping code to use the new clipchain API.

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/976c2ea507b9
https://hg.mozilla.org/mozilla-central/rev/6a55c1fa635e