Closed Bug 1389138 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Graphics: WebRender
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Bug 1389138 - Extract a helper PushScrollLayer method.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1389138 - Introduce a variant type to allow unifying the clip and scroll ids.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1389138 - Push scroll layers in layers-free WR.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1389138 - Add helper to track the topmost scroll id on the WR stack.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
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 on attachment 8896391 [details]
Bug 1389138 - Extract a helper PushScrollLayer method.

https://reviewboard.mozilla.org/r/167644/#review174588
Attachment #8896391 - Flags: review?(mstange) → review+
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
Attachment #8896392 - Flags: review?(mstange) → review+
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.
Attachment #8896393 - Flags: review?(mstange) → review+
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.
Attachment #8896394 - Flags: review?(mstange) → review+
(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 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.
Attachment #8896394 - Flags: review+ → review?(mstange)
Attachment #8896393 - Flags: review+ → review?(mstange)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
Oh, huh, then I don't understand what happened.
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
Attachment #8896394 - Flags: review?(mstange) → review+
Comment on attachment 8896393 [details]
Bug 1389138 - Push scroll layers in layers-free WR.

https://reviewboard.mozilla.org/r/167648/#review174974
Attachment #8896393 - Flags: review?(mstange) → review+
Pushed by kgupta@mozilla.com:
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
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.
Flags: needinfo?(bugmail)
Will fix and reland.
Flags: needinfo?(bugmail)
Pushed by kgupta@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: