1409446 - Rewrite the ScrollingLayersHelper code to use the explicit parent APIs

Status: RESOLVED FIXED

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

Description

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

Splitting off this bug as an intermediate step to fixing bug 1405359. Right now when defining a clip or a scroll layer, we are required to define it only after pushing the appropriate ancestor onto the stack. In servo/webrender#1796 (later modified with #1849 and #1857) I added an API that allows us to explicitly pass the desired ancestor id when defining a clip or scroll layer. This allows us to skip the pushing/popping of the ancestor.

This bug is to update ScrollingLayersHelper to use this new API and save on some of the pushing/popping that happens.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cb12ed4a37aeb1b06be6d09adecf687d89b5ed2

Comment 2

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

Attachment: Bug 1409446 - Wire up the DefineClip and DefineScrollLayer APIs to allow specifying ancestry.

The APIs now allow providing the parent clip or scroll info explicitly
instead of having to push it on the stack. For now we just pass
Nothing() to preserve the existing behaviour, so this change is a
functinoal no-op.

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

Comment 3

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

Attachment: Bug 1409446 - Remove unused function.

We don't use the code to track the parents of each scroll id, so we can
dump it and just keep a set of scroll ids that we've defined to avoid
redefining them.

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

Comment 4

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

Attachment: Bug 1409446 - Unify the clip stack in WebRenderAPI.

By using a variant we can keep a single stack of both clips and
scrollframes, rather than two separate stacks. This is important because
we will want to know how the things are interleaved (e.g. if the last
thing that was pushed was a clip or a scrollframe).

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

Comment 5

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

Attachment: Bug 1409446 - Write the new recursive method to define clip chains.

This code is more straightforward in its recursion than the old code,
and provides a relatively clean way to explicitly pass the desired
parent when defining a new clip or scroll layer.

Refer to the documentation in the patch for more details.

Note that this patch provides the basic recursive algorithm to define
the clips and scroll layers, although it omits some of the complicating
edge cases which will be added in later patches. The new code is not
invoked from anywhere until all the edge case handling has been done.

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

Comment 6

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

Attachment: Bug 1409446 - Modify the extra-clip flag to instead track more useful information.

Instead of just keeping a count of how many "extra clips" (aka
out-of-band clips) we have pushed, track more complex information for
each clip. In particular, track the display item's normal clip chain, as
well as the clip id of the extra clip that was pushed. This will be
needed to override clip cache information in the next patch.

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

Comment 7

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

Attachment: Bug 1409446 - Properly handle out-of-band clips in the new ScrollingLayersHelper code.

When display items (such as mask items) push an out-of-band clip, we
can't use clip ids from, or update clip ids into, the cache. We also
need to ensure we take these out-of-band clips into account when
determining the parent for a new clip we are going to define.

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

Comment 8

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

Attachment: Bug 1409446 - Deal with scenario of two interchangeable DisplayItemClipChain objects causing a cache miss.

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

Comment 9

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

Attachment: Bug 1409446 - Work around the dual-ancestor case that WR doesn't handle yet.

Bug 1409442 is tracking a change that will allow scroll layers to have
multiple ancestors. Without that, there are cases we cannot properly
handle, and so we need to ignore a clip in those scenarios. This patch
makes sure we do that instead of crashing.

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

Comment 10

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

Attachment: Bug 1409446 - Handle nested display item scenarios.

This handles some cases where a nested display item's clip chain
implicitly extends from the wrapper item's clip chain.

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

Comment 11

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

Attachment: Bug 1409446 - Treat sticky clips as out-of-band clips.

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

Comment 12

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

Attachment: Bug 1409446 - Switch over to the new code.

One of the sticky-pos tests was only passing because of two wrongs that
cancelled each other out in the old code. Specifically, instead of
defining a nested clip with the sticky clip as an ancestor, the clip was being
defined with the root ASR as an ancestor. Both resulted in the nested
clip not scrolling with the actual scrolling scrollframe and so the test
was passing.

The new code changes things so that the nested clip is defined with the
actual scrolling scrollframe as the ancestor, causing the reftest to fail.
Fixing the clip ancestry is not hard but it reveals other problems so
so I'm deferring that to a follow-up bug.

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

Comment 13

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

Attachment: Bug 1409446 - Remove old code that is now unused.

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

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8919427 [details]
Bug 1409446 - Wire up the DefineClip and DefineScrollLayer APIs to allow specifying ancestry.

https://reviewboard.mozilla.org/r/190254/#review195592

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8919428 [details]
Bug 1409446 - Remove unused function.

https://reviewboard.mozilla.org/r/190256/#review195596

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8919429 [details]
Bug 1409446 - Unify the clip stack in WebRenderAPI.

https://reviewboard.mozilla.org/r/190258/#review195598

Comment 17

[Ethan Lin[:ethlin]]

Comment on attachment 8919431 [details]
Bug 1409446 - Modify the extra-clip flag to instead track more useful information.

https://reviewboard.mozilla.org/r/190262/#review195754

Comment 18

[Ethan Lin[:ethlin]]

Comment on attachment 8919436 [details]
Bug 1409446 - Treat sticky clips as out-of-band clips.

https://reviewboard.mozilla.org/r/190272/#review195756

Comment 19

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

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

Comment 20

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

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

I also deleted the TODO at [1] in the "Treat sticky clips as out-of-band clips." patch, because that patch addresses the TODO.

[1] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/layout/painting/nsDisplayList.cpp#7088

Comment 21

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

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

Comment 22

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

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

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8919430 [details]
Bug 1409446 - Write the new recursive method to define clip chains.

https://reviewboard.mozilla.org/r/190260/#review196920

::: gfx/layers/wr/ScrollingLayersHelper.cpp:102
(Diff revision 1)
> +  // aChain->mASR == aAsr             => recurse(aAsr, aChain->mParent),
> +  //                                     then define aChain
> +  // aChain->mASR != aAsr             => recurse(aAsr->mParent, aChain),
> +  //                                     then define aAsr
> +  //
> +  // These basically be collapsed down into two codepaths; one that recurses

I think this is missing the word "can"

::: gfx/layers/wr/ScrollingLayersHelper.cpp:110
(Diff revision 1)
> +  if (aChain && aChain->mASR == aAsr) {
> +    return RecurseAndDefineClip(aItem, aAsr, aChain, aAppUnitsPerDevPixel, aStackingContext, aCache);
> +  }
> +  if (aAsr) {
> +    return RecurseAndDefineAsr(aItem, aAsr, aChain, aAppUnitsPerDevPixel, aStackingContext, aCache);
> +  }
> +
> +  MOZ_ASSERT(!aChain && !aAsr);

Especially looking at the assert below, the if statements seem a bit verbose. It seems the "return RecourseAndDefineClip(...)" could just happen inside a single if (aChain || aAsr) {...}. Or did you do it the way you did it to stay closer to the comment above?

Comment 24

[Markus Stange [:mstange]]

Comment on attachment 8919431 [details]
Bug 1409446 - Modify the extra-clip flag to instead track more useful information.

https://reviewboard.mozilla.org/r/190262/#review197736

I'm a bit unhappy about the layering violation where renderers now need to know about nsDisplayItem, but I don't have any good ideas for how to avoid that.

::: gfx/webrender_bindings/WebRenderAPI.h:432
(Diff revision 1)
> -  uint32_t mExtraClipCount;
> +  // clips that are generated by display items but that ScrollingLayersHelper
> +  // doesn't know about. These are called "cache overrides" because while we're
> +  // inside one of these clips, the WR clip stack is different from what
> +  // ScrollingLayersHelper thinks it actually is (because of the out-of-band
> +  // clip that was pushed onto the stack) and so ScrollingLayersHelper cannot
> +  // use it's clip cache as-is. Instead, any time ScrollingLayersHelper wants

it's -> its

Comment 25

[Markus Stange [:mstange]]

Comment on attachment 8919432 [details]
Bug 1409446 - Properly handle out-of-band clips in the new ScrollingLayersHelper code.

https://reviewboard.mozilla.org/r/190264/#review197742

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8919433 [details]
Bug 1409446 - Deal with scenario of two interchangeable DisplayItemClipChain objects causing a cache miss.

https://reviewboard.mozilla.org/r/190266/#review197744

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8919434 [details]
Bug 1409446 - Work around the dual-ancestor case that WR doesn't handle yet.

https://reviewboard.mozilla.org/r/190268/#review197746

Comment 28

[Markus Stange [:mstange]]

Comment on attachment 8919435 [details]
Bug 1409446 - Handle nested display item scenarios.

https://reviewboard.mozilla.org/r/190270/#review197748

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 8919436 [details]
Bug 1409446 - Treat sticky clips as out-of-band clips.

https://reviewboard.mozilla.org/r/190272/#review197750

Comment 30

[Markus Stange [:mstange]]

Comment on attachment 8919437 [details]
Bug 1409446 - Switch over to the new code.

https://reviewboard.mozilla.org/r/190274/#review197756

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8919438 [details]
Bug 1409446 - Remove old code that is now unused.

https://reviewboard.mozilla.org/r/190276/#review197758

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8919430 [details]
Bug 1409446 - Write the new recursive method to define clip chains.

https://reviewboard.mozilla.org/r/190260/#review197760

::: gfx/layers/wr/ScrollingLayersHelper.cpp:110
(Diff revision 1)
> +  if (aChain && aChain->mASR == aAsr) {
> +    return RecurseAndDefineClip(aItem, aAsr, aChain, aAppUnitsPerDevPixel, aStackingContext, aCache);
> +  }
> +  if (aAsr) {
> +    return RecurseAndDefineAsr(aItem, aAsr, aChain, aAppUnitsPerDevPixel, aStackingContext, aCache);
> +  }
> +
> +  MOZ_ASSERT(!aChain && !aAsr);

Oops, I missed that this was calling different functions.

Comment 33

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

(In reply to Markus Stange [:mstange] from comment #23)
> > +  // These basically be collapsed down into two codepaths; one that recurses
> 
> I think this is missing the word "can"

Fixed

> Especially looking at the assert below, the if statements seem a bit
> verbose. It seems the "return RecourseAndDefineClip(...)" could just happen
> inside a single if (aChain || aAsr) {...}. Or did you do it the way you did
> it to stay closer to the comment above?

Different functions, but also yes, I wanted to keep the structure of the code closer to the comment so that it would be easier to follow.

(In reply to Markus Stange [:mstange] from comment #24) 
> I'm a bit unhappy about the layering violation where renderers now need to
> know about nsDisplayItem, but I don't have any good ideas for how to avoid
> that.

Ah, I didn't even realize it was a layering violation, thanks for pointing that out. Technically the only thing I need is the clipchain which I could pass as a void* if we really want to avoid abuse of the nsDisplayItem*. I can't think of any other ways to avoid this either.

> ::: gfx/webrender_bindings/WebRenderAPI.h:432
> > +  // use it's clip cache as-is. Instead, any time ScrollingLayersHelper wants
> 
> it's -> its

Fixed.

Comment 34

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

Comment on attachment 8919427 [details]
Bug 1409446 - Wire up the DefineClip and DefineScrollLayer APIs to allow specifying ancestry.

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

Comment 35

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

Comment on attachment 8919428 [details]
Bug 1409446 - Remove unused function.

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

Comment 36

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

Comment on attachment 8919429 [details]
Bug 1409446 - Unify the clip stack in WebRenderAPI.

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

Comment 37

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

Comment on attachment 8919430 [details]
Bug 1409446 - Write the new recursive method to define clip chains.

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

Comment 38

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

Comment on attachment 8919431 [details]
Bug 1409446 - Modify the extra-clip flag to instead track more useful information.

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

Comment 39

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

Comment on attachment 8919432 [details]
Bug 1409446 - Properly handle out-of-band clips in the new ScrollingLayersHelper code.

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

Comment 40

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

Comment on attachment 8919433 [details]
Bug 1409446 - Deal with scenario of two interchangeable DisplayItemClipChain objects causing a cache miss.

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

Comment 41

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

Comment on attachment 8919434 [details]
Bug 1409446 - Work around the dual-ancestor case that WR doesn't handle yet.

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

Comment 42

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

Comment on attachment 8919435 [details]
Bug 1409446 - Handle nested display item scenarios.

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

Comment 43

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

Comment on attachment 8919436 [details]
Bug 1409446 - Treat sticky clips as out-of-band clips.

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

Comment 44

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

Comment on attachment 8919437 [details]
Bug 1409446 - Switch over to the new code.

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

Comment 45

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

Comment on attachment 8919438 [details]
Bug 1409446 - Remove old code that is now unused.

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

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/793acbd73126
Wire up the DefineClip and DefineScrollLayer APIs to allow specifying ancestry. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/4ae5fc22aecf
Remove unused function. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/b1465f5c3f35
Unify the clip stack in WebRenderAPI. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/408acda8e5e5
Write the new recursive method to define clip chains. r=mstange
https://hg.mozilla.org/integration/autoland/rev/75e3a4777a7f
Modify the extra-clip flag to instead track more useful information. r=ethlin,mstange
https://hg.mozilla.org/integration/autoland/rev/15f491515ba6
Properly handle out-of-band clips in the new ScrollingLayersHelper code. r=mstange
https://hg.mozilla.org/integration/autoland/rev/40c84480b4d4
Deal with scenario of two interchangeable DisplayItemClipChain objects causing a cache miss. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4a1434cdcae3
Work around the dual-ancestor case that WR doesn't handle yet. r=mstange
https://hg.mozilla.org/integration/autoland/rev/debf2683f01f
Handle nested display item scenarios. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4d8523e88330
Treat sticky clips as out-of-band clips. r=ethlin,mstange
https://hg.mozilla.org/integration/autoland/rev/53894a33da8c
Switch over to the new code. r=mstange
https://hg.mozilla.org/integration/autoland/rev/e036a5afa854
Remove old code that is now unused. r=mstange

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/793acbd73126
https://hg.mozilla.org/mozilla-central/rev/4ae5fc22aecf
https://hg.mozilla.org/mozilla-central/rev/b1465f5c3f35
https://hg.mozilla.org/mozilla-central/rev/408acda8e5e5
https://hg.mozilla.org/mozilla-central/rev/75e3a4777a7f
https://hg.mozilla.org/mozilla-central/rev/15f491515ba6
https://hg.mozilla.org/mozilla-central/rev/40c84480b4d4
https://hg.mozilla.org/mozilla-central/rev/4a1434cdcae3
https://hg.mozilla.org/mozilla-central/rev/debf2683f01f
https://hg.mozilla.org/mozilla-central/rev/4d8523e88330
https://hg.mozilla.org/mozilla-central/rev/53894a33da8c
https://hg.mozilla.org/mozilla-central/rev/e036a5afa854