1389138 - Create scroll layers in the WR display list for layers-free mode

Status: RESOLVED FIXED

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

Description

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

In layers mode, the ScrollingLayersHelper makes calls to PushScrollLayer [1] to create "scroll layers" in WR. This allows APZ to do async scrolling using the scroll ids. We need to hook this up in layers-free mode as well.

[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/gfx/layers/wr/ScrollingLayersHelper.cpp#73

Comment 1

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

Attachment: Bug 1389138 - Extract a helper PushScrollLayer method.

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

Comment 2

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

Attachment: Bug 1389138 - Introduce a variant type to allow unifying the clip and scroll ids.

This allows us to keep a single stack where we track both clip ids and
scroll ids with the correct interleaving order that they were pushed in.

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

Comment 3

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

Attachment: Bug 1389138 - Push scroll layers in layers-free WR.

This patch takes the existing code (which allows recursively pushing the necessary
part of a clip chain) and folds it into another recursive function that
operates on the ASR chain. Now the ASR recursion is the primary one, and
clips are pushed for each ASR as needed.

This code was partly modelled after the code in
ContainerState::SetupScrollingMetadata, which also iterates over the
ASRs and clips.

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

Comment 4

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

Attachment: Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

When dealing with fixed-pos items we need to make sure they have right
combination of scroll layer and clip.

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

Comment 5

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

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

Comment 6

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

Comment on attachment 8896391 [details]
Bug 1389138 - Extract a helper PushScrollLayer method.

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

Comment 7

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

Comment on attachment 8896392 [details]
Bug 1389138 - Introduce a variant type to allow unifying the clip and scroll ids.

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

Comment 8

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

Comment on attachment 8896393 [details]
Bug 1389138 - Push scroll layers in layers-free WR.

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

Comment 9

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

Comment on attachment 8896394 [details]
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

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

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8896391 [details]
Bug 1389138 - Extract a helper PushScrollLayer method.

https://reviewboard.mozilla.org/r/167644/#review174588

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8896392 [details]
Bug 1389138 - Introduce a variant type to allow unifying the clip and scroll ids.

https://reviewboard.mozilla.org/r/167646/#review174590

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8896393 [details]
Bug 1389138 - Push scroll layers in layers-free WR.

https://reviewboard.mozilla.org/r/167648/#review174594

::: gfx/layers/wr/ScrollingLayersHelper.cpp:100
(Diff revision 2)
>  {
> +  int32_t auPerDevPixel = aItem->Frame()->PresContext()->AppUnitsPerDevPixel();
> +
> +  // Push all the ASRs from whatever was last pushed down to the item's
> +  // enclosing ASR, as well all the clips that affect those ASRs.
> +  DefineAndPushScrollLayers(aItem, aItem->GetActiveScrolledRoot(),

I'm uncertain whether passing aItem->GetActiveScrolledRoot() here is the right thing to do vs passing aItem->GetClipChain()->mASR. At the moment, this would only make a difference for fixed items, so the next patch is probably going to handle it somehow.

::: gfx/layers/wr/ScrollingLayersHelper.cpp:136
(Diff revision 2)
> +  // Find the first clip up the chain that's "outside" aAsr. Any clips
> +  // that are "inside" aAsr (i.e. that are scrolled by aAsr) will need to be
> +  // pushed onto the stack after aAsr has been pushed. On the recursive call
> +  // we need to skip up the clip chain past these clips.
> +  const DisplayItemClipChain* asrClippedBy = aChain;
> +  while (asrClippedBy && asrClippedBy->mASR == aAsr) {

I think this condition needs to be
(asrClippedBy && ActiveScrolledRoot::PickAncestor(asrClippedBy->mASR, aAsr) == aAsr).

Otherwise, in the case where the item's clip is scrolled by a child ASR of the item's ASR, asrClippedBy will be set to the item's scrolled clip because the loop will never be run.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8896394 [details]
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

https://reviewboard.mozilla.org/r/167650/#review174600

I would be happier if fixed items weren't handled specially and the if the information about what to fix it to came from the ASR instead of from the FrameMetrics, but maybe this is fine for now. It's closer to what FrameLayerBuilder does anyway.

Comment 14

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

(In reply to Markus Stange [:mstange] from comment #12)
> I'm uncertain whether passing aItem->GetActiveScrolledRoot() here is the
> right thing to do vs passing aItem->GetClipChain()->mASR. At the moment,
> this would only make a difference for fixed items, so the next patch is
> probably going to handle it somehow.

I gave this some more thought and realized that in the case where the item's clip is scrolled by a child ASR of the item's ASR (same case you mentioned below) we do want aItem->GetClipChain()->mASR here. For maximum generic-ness I rewrote this code to use PickDescendant on the two ASRs so that we always call this function with whichever ASR is more "leafmost" in the tree.

> ::: gfx/layers/wr/ScrollingLayersHelper.cpp:136
> I think this condition needs to be
> (asrClippedBy && ActiveScrolledRoot::PickAncestor(asrClippedBy->mASR, aAsr)
> == aAsr).
> 
> Otherwise, in the case where the item's clip is scrolled by a child ASR of
> the item's ASR, asrClippedBy will be set to the item's scrolled clip because
> the loop will never be run.

Yes, you're right. I didn't properly handle this scenario of the item's clip being scrolled by an ASR that is a descendant of the item's ASR. Updated patch will have this fixed.

(In reply to Markus Stange [:mstange] from comment #13)
> I would be happier if fixed items weren't handled specially and the if the
> information about what to fix it to came from the ASR instead of from the
> FrameMetrics, but maybe this is fine for now. It's closer to what
> FrameLayerBuilder does anyway.

Agreed, I updated this as well so that it doesn't hard-code the position-fixed item but instead checks to see if the topmost scroll id on the stack is the one we want.

Apart from all this, I realized I was missing code to update the scroll id stack on calls to PushClipAndScrollInfo, so I fixed that as well. The new patches I'm pushing are like so:

Part 1 - same as before
Part 2 - same as before
Part 3 - new, contains the fix to the scroll id stack, and I also extracted the addition of the TopmostScrollId() function which was in the old part 3
Part 4 - old part 3 combined with old part 4, plus the fixes for the issues you pointed out.

I'm using the same m-c base rev and commit IDs so hopefully the interdiff is useful.

Comment 15

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

Comment on attachment 8896391 [details]
Bug 1389138 - Extract a helper PushScrollLayer method.

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

Comment 16

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

Comment on attachment 8896392 [details]
Bug 1389138 - Introduce a variant type to allow unifying the clip and scroll ids.

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

Comment 17

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

Comment on attachment 8896394 [details]
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

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

Comment 18

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

Comment on attachment 8896393 [details]
Bug 1389138 - Push scroll layers in layers-free WR.

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

Comment 19

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

Comment on attachment 8896394 [details]
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

Well that almost worked, except it didn't recognize the new part 3 as new. Setting the flags manually for re-review on part 3 and 4.

Comment 20

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> Well that almost worked, except it didn't recognize the new part 3 as new.

You need to remove the MozReview-Commit-ID line from the extended commit message if you want mozreview to treat the patch as new.

Comment 21

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

(In reply to Markus Stange [:mstange] from comment #20)
> You need to remove the MozReview-Commit-ID line from the extended commit
> message if you want mozreview to treat the patch as new.

I did in my local patch, but as soon as I qpush the patch into my mercurial queue one of the hg extensions adds a new one. I thought the requirement was that the commit id needs to be new, not that it shouldn't be present at all. It looks like MozReview somehow decided the new part 3 corresponds to the old part 4 even though the commit IDs are different.

Comment 22

[Markus Stange [:mstange]]

Oh, huh, then I don't understand what happened.

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8896394 [details]
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

https://reviewboard.mozilla.org/r/167650/#review174966

Comment 24

[Markus Stange [:mstange]]

Comment on attachment 8896393 [details]
Bug 1389138 - Push scroll layers in layers-free WR.

https://reviewboard.mozilla.org/r/167648/#review174974

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20c649cb4a21
Extract a helper PushScrollLayer method. r=mstange
https://hg.mozilla.org/integration/autoland/rev/442b037713d6
Introduce a variant type to allow unifying the clip and scroll ids. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d45e64ac4452
Add helper to track the topmost scroll id on the WR stack. r=mstange
https://hg.mozilla.org/integration/autoland/rev/321176b458a5
Push scroll layers in layers-free WR. r=mstange

Comment 26

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

Backed out for bustage at dist/include/mozilla/layers/ScrollingLayersHelper.h:22: class 'FrameMetrics' was previously declared as a struct:

https://hg.mozilla.org/integration/autoland/rev/c72b111355887e0a23073fbede374d3634f0908a
https://hg.mozilla.org/integration/autoland/rev/8dff08e46838d5ccb9c70ccb8eb56b7280d27b1d
https://hg.mozilla.org/integration/autoland/rev/bda67429b9c9b4d071e275378718efa3ff48f4a3
https://hg.mozilla.org/integration/autoland/rev/20e02a7df9377dbeb1dbdf2d330c7c0a621ab0b0

Recent push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=211697317f6b0669ac8c4e55e84fd05dd9615fea&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123928053&repo=autoland

[task 2017-08-17T18:59:40.465435Z] 18:59:40     INFO -  In file included from /home/worker/workspace/build/src/gfx/layers/wr/WebRenderCanvasLayer.cpp:15:
[task 2017-08-17T18:59:40.465530Z] 18:59:40     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/ScrollingLayersHelper.h:22:1: error: class 'FrameMetrics' was previously declared as a struct [-Werror,-Wmismatched-tags]
[task 2017-08-17T18:59:40.465574Z] 18:59:40     INFO -  class FrameMetrics;
[task 2017-08-17T18:59:40.465606Z] 18:59:40     INFO -  ^
[task 2017-08-17T18:59:40.465818Z] 18:59:40     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/FrameMetrics.h:45:8: note: previous use is here
[task 2017-08-17T18:59:40.465995Z] 18:59:40     INFO -  struct FrameMetrics {
[task 2017-08-17T18:59:40.466178Z] 18:59:40     INFO -         ^
[task 2017-08-17T18:59:40.466407Z] 18:59:40     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/ScrollingLayersHelper.h:22:1: note: did you mean struct here?
[task 2017-08-17T18:59:40.466572Z] 18:59:40     INFO -  class FrameMetrics;
[task 2017-08-17T18:59:40.466760Z] 18:59:40     INFO -  ^~~~~
[task 2017-08-17T18:59:40.466947Z] 18:59:40     INFO -  struct
[task 2017-08-17T18:59:40.467287Z] 18:59:40     INFO -  1 error generated.

Comment 27

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

Will fix and reland.

Comment 28

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

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

Comment 29

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

Comment on attachment 8896391 [details]
Bug 1389138 - Extract a helper PushScrollLayer method.

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

Comment 30

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

Comment on attachment 8896392 [details]
Bug 1389138 - Introduce a variant type to allow unifying the clip and scroll ids.

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

Comment 31

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

Comment on attachment 8896394 [details]
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.

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

Comment 32

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

Comment on attachment 8896393 [details]
Bug 1389138 - Push scroll layers in layers-free WR.

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ed1e355fd10
Extract a helper PushScrollLayer method. r=mstange
https://hg.mozilla.org/integration/autoland/rev/cdf220527d0c
Introduce a variant type to allow unifying the clip and scroll ids. r=mstange
https://hg.mozilla.org/integration/autoland/rev/00554875a00a
Add helper to track the topmost scroll id on the WR stack. r=mstange
https://hg.mozilla.org/integration/autoland/rev/5d7460dd02f9
Push scroll layers in layers-free WR. r=mstange

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/9ed1e355fd10
https://hg.mozilla.org/mozilla-central/rev/cdf220527d0c
https://hg.mozilla.org/mozilla-central/rev/00554875a00a
https://hg.mozilla.org/mozilla-central/rev/5d7460dd02f9