1368496 - Make layout/reftests/async-scrolling/bg-fixed-child.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 a bunch of fixed-pos/sticky-pos/background-attachment:fixed type tests are failing. This bug is specifically about layout/reftests/async-scrolling/bg-fixed-child-ref.html which uses background-attachment:fixed.

Comment 1

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

Some WIPs are in this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4a651ef760e2175615c3f4230bf60079e44adda

Comment 2

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

Got the patches ready for review, try push to make sure they don't break anything with APZ still disabled:

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

Comment 3

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

Attachment: Bug 1368496 - Extract a helper function from WebRenderLayer::ClipRect().

No functional changes, but we'll want this helper in a future patch for
code reuse.

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

Comment 4

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

Attachment: Bug 1368496 - Propagate the layer's "scrolled clip" to WebRender.

If the layer has a "scrolled clip", that clip scrolls with the content
of the layers (it moves if any of the scrollable frames defined by that
layer are scrolled).

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

Comment 5

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

Attachment: Bug 1368496 - Track the pushed clip and scroll IDs.

Tracking the active clips and scroll layers is helpful for implementing
fixed-position type stuff, because we will need to answer questions like
"what is the ancestor scroll layer of this other scroll layer that is
somewhere on the stack" and "what is the currently active stack of
non-scrolling clips". This patch also adds a couple of methods to
WebRenderAPI that answers questions like these.

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

Comment 6

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

Attachment: Bug 1368496 - Expose the push_clip_and_scroll_info API in WR via WebRenderAPI.

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

Comment 7

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

Attachment: Bug 1368496 - Add support for fixed-positioning with APZ.

This implements at least enough fixed-positioning support to make the
reftest in layout/reftests/async-scrolling/bg-fixed-child.html pass.
That test implements a simple background-attachment:fixed which is
implemented in the layer tree with a combination of a scrolled clip and
a fixed-position layer. Previous patches in this patchset implemented
support for the scrolled clip, and this patch implements the
fixed-position layer support.

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

Comment 8

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

Martin, it would be great if you also took a look at these patches. You're probably not that familiar with the gecko side of things but I'm happy to answer questions and/or provide a high-level view of how things work. Even if you don't understand the details of what all the patches are doing it would be good to see how we're interacting with the WR API as it might help inform the direction of future API changes.

Comment 9

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

Also obviously if see better ways to do this, or potential pitfalls in what I'm doing, please point them out, even if you're not entirely sure.

Comment 10

[Martin Robinson (email:[e-mail])]

Adding my comments here since I don't have permissions to leave comments in the review tool:

Last patch, gfx/layers/wr/ScrollingLayersHelper.cpp, line 86:
> mBuilder->PushClipAndScrollInfo(scrollsWith.valueOr(0), clipId.ptrOr(nullptr));

Do you want this to be fixed if scrollsWith is Nothing? In that case it might be better to grab the root_reference_frame explicitly.


Second to last patch, gfx/webrender_bindings/WebRenderAPI.cpp, line 625:
> DisplayListBuilder::PushClipAndScrollInfo(const layers::FrameMetrics::ViewID& aScrollId,
>                                           const uint64_t* aClipId)

Hrm. I'm not sure I understand completely why these two parameters are different types since the arguments are both ClipIds in Rust. Is it possible to expose some kind of data type that encapsulates ClipId?

Third to last patch, gfx/webrender_bindings/webrender_ffi_generated.h, line 711:

> uint64_t wr_dp_push_clip(WrState *aState,
> >>>>                     WrRect aClipRect,
> >>>>                     const WrImageMask *aMask

These look a little bit like leftover merge markers?

All in all, this patch series looks good to me. Let me know if you run into issue with the clipping API.

Comment 11

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

(In reply to Martin Robinson (mrobinson) from comment #10)
> Last patch, gfx/layers/wr/ScrollingLayersHelper.cpp, line 86:
> > mBuilder->PushClipAndScrollInfo(scrollsWith.valueOr(0), clipId.ptrOr(nullptr));
> 
> Do you want this to be fixed if scrollsWith is Nothing? In that case it
> might be better to grab the root_reference_frame explicitly.
> 

What's the semantic difference between using the root_scroll_node and the root_reference_frame, in this context (i.e. when making an item fixed)? Shouldn't they have the same effect? Also, what would be the difference between this and pushing a new stacking context (with no transform and 0,0 in the top-left bounds) that uses ScrollPolicy::Fixed?

> Second to last patch, gfx/webrender_bindings/WebRenderAPI.cpp, line 625:
> > DisplayListBuilder::PushClipAndScrollInfo(const layers::FrameMetrics::ViewID& aScrollId,
> >                                           const uint64_t* aClipId)
> 
> Hrm. I'm not sure I understand completely why these two parameters are
> different types since the arguments are both ClipIds in Rust. Is it possible
> to expose some kind of data type that encapsulates ClipId?

The FrameMetrics::ViewID corresponds to a ClipId::ClipExternalId and the uin64_t corresponds to a ClipId::ClipId. I could encapsulate the latter with a typedef, but I don't want to unify the two types on the C++ side because they come from difference places and are treated differently. It would also be an unnecessary removal of abstraction (i.e. it would expose the fact that WebRender treats the two as a unifiied ClipId type, which there's no reason to expose).

> Third to last patch, gfx/webrender_bindings/webrender_ffi_generated.h, line
> 711:
> 
> > uint64_t wr_dp_push_clip(WrState *aState,
> > >>>>                     WrRect aClipRect,
> > >>>>                     const WrImageMask *aMask
> 
> These look a little bit like leftover merge markers?

Those are actually whitespace markers. I indented those overhanging lines to keep them lined up and ReviewBoard highlights those extra spaces with that marker.

> All in all, this patch series looks good to me. Let me know if you run into
> issue with the clipping API.

Thanks!

Comment 12

[Martin Robinson (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> (In reply to Martin Robinson (mrobinson) from comment #10)
> > Last patch, gfx/layers/wr/ScrollingLayersHelper.cpp, line 86:
> > > mBuilder->PushClipAndScrollInfo(scrollsWith.valueOr(0), clipId.ptrOr(nullptr));
> > 
> > Do you want this to be fixed if scrollsWith is Nothing? In that case it
> > might be better to grab the root_reference_frame explicitly.
> > 
> 
> What's the semantic difference between using the root_scroll_node and the
> root_reference_frame, in this context (i.e. when making an item fixed)?
> Shouldn't they have the same effect? Also, what would be the difference
> between this and pushing a new stacking context (with no transform and 0,0
> in the top-left bounds) that uses ScrollPolicy::Fixed?

The root reference frame is a frame the size of the viewport that doesn't move at all during scrolling. Items that are attached to this frame will not scroll. Content inside the root scroll frame will scroll around when the scroll position of the frame changes. If you push a stacking context with ScrollPolicy::Fixed, WebRender will attach the contents and all new ClipScrollNodes to the nearest ancestor reference frame. 
 
> The FrameMetrics::ViewID corresponds to a ClipId::ClipExternalId and the
> uin64_t corresponds to a ClipId::ClipId. I could encapsulate the latter with
> a typedef, but I don't want to unify the two types on the C++ side because
> they come from difference places and are treated differently. It would also
> be an unnecessary removal of abstraction (i.e. it would expose the fact that
> WebRender treats the two as a unifiied ClipId type, which there's no reason
> to expose).

Okay. This makes sense to me.

Comment 13

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

(In reply to Martin Robinson (mrobinson) from comment #12)
> > What's the semantic difference between using the root_scroll_node and the
> > root_reference_frame, in this context (i.e. when making an item fixed)?
> > Shouldn't they have the same effect? Also, what would be the difference
> > between this and pushing a new stacking context (with no transform and 0,0
> > in the top-left bounds) that uses ScrollPolicy::Fixed?
> 
> The root reference frame is a frame the size of the viewport that doesn't
> move at all during scrolling. Items that are attached to this frame will not
> scroll. Content inside the root scroll frame will scroll around when the
> scroll position of the frame changes.

But is it even possible to scroll the root scroll frame? As far as I can tell the clip_stack is initialized with the root scroll id, and while it's the "parent" of all the other clips, there's no way to define how much it can scroll. Therefore the first actual scrollable clip id will have an id > 0. Effectively the root scroll frame is not actually scrollable, so using that as the ancestor scrollable is effectively the same as using the root reference frame. Or am I mistaken somewhere?

I tried modifying the scrolling example so that the first define_clip call used the ClipId::root_scroll_node(..) as the id instead of "42", but that resulted in an assertion which is not really surprising because it would try to add the root scrolling clip as a child of itself.

> If you push a stacking context with
> ScrollPolicy::Fixed, WebRender will attach the contents and all new
> ClipScrollNodes to the nearest ancestor reference frame. 

Ok, this makes sense.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8873870 [details]
Bug 1368496 - Extract a helper function from WebRenderLayer::ClipRect().

https://reviewboard.mozilla.org/r/145266/#review150228

::: gfx/layers/wr/WebRenderLayer.cpp:97
(Diff revision 1)
>  
> +LayerRect
> +WebRenderLayer::UntransformClip(const ParentLayerIntRect& aClipRect)
> +{
> +  LayerToParentLayerMatrix4x4 transform = GetLayer()->GetLocalTransformTyped();
> +  return transform.Inverse().TransformBounds(IntRectToRect(aClipRect));

I prefer not to have magical Inverse()s in helpers like this, but I don't yet understand how it's used. I look into it more.

Comment 15

[Martin Robinson (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
 
> > The root reference frame is a frame the size of the viewport that doesn't
> > move at all during scrolling. Items that are attached to this frame will not
> > scroll. Content inside the root scroll frame will scroll around when the
> > scroll position of the frame changes.
> 
> But is it even possible to scroll the root scroll frame? As far as I can
> tell the clip_stack is initialized with the root scroll id, and while it's
> the "parent" of all the other clips, there's no way to define how much it
> can scroll. Therefore the first actual scrollable clip id will have an id >
> 0. Effectively the root scroll frame is not actually scrollable, so using
> that as the ancestor scrollable is effectively the same as using the root
> reference frame. Or am I mistaken somewhere?

Scrolling of the root scroll frame is determined by the display list content size. If the content size if larger than the view size, the content should be scrollable. If you apply this patch to scrolling.rs you should see the root scroll frame actually scroll:
https://pastebin.mozilla.org/9023627

> I tried modifying the scrolling example so that the first define_clip call
> used the ClipId::root_scroll_node(..) as the id instead of "42", but that
> resulted in an assertion which is not really surprising because it would try

I think this is just a simple mistake. The ClipId argument that you pass to define_clip is used to assign a clip to the new Clip item. This is to allow efficient assignment and tracking of scroll frame clip ids in Servo. define_clip should probably assert if the clip isn't something that it expects. I will make that change in WebRender.

Comment 16

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

(In reply to Martin Robinson (mrobinson) from comment #15)
> Scrolling of the root scroll frame is determined by the display list content
> size. If the content size if larger than the view size, the content should
> be scrollable. If you apply this patch to scrolling.rs you should see the
> root scroll frame actually scroll:
> https://pastebin.mozilla.org/9023627

Ah, that makes sense. In gecko we are always setting the display list content size to be equal to the viewport size, because at the time we create the display list we don't have the information about the actual size [1]. So in our case the "root scroll frame" will never be scrollable until that is changed.

> I think this is just a simple mistake. The ClipId argument that you pass to
> define_clip is used to assign a clip to the new Clip item. This is to allow
> efficient assignment and tracking of scroll frame clip ids in Servo.
> define_clip should probably assert if the clip isn't something that it
> expects. I will make that change in WebRender.

Sounds good, thanks.

[1] http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/gfx/layers/wr/WebRenderLayerManager.cpp#201

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8873873 [details]
Bug 1368496 - Expose the push_clip_and_scroll_info API in WR via WebRenderAPI.

https://reviewboard.mozilla.org/r/145272/#review150282

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8873874 [details]
Bug 1368496 - Add support for fixed-positioning with APZ.

https://reviewboard.mozilla.org/r/145274/#review150330

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8873872 [details]
Bug 1368496 - Track the pushed clip and scroll IDs.

https://reviewboard.mozilla.org/r/145270/#review150332

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8873871 [details]
Bug 1368496 - Propagate the layer's "scrolled clip" to WebRender.

https://reviewboard.mozilla.org/r/145268/#review150334

Comment 21

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

Attachment: Bug 1368496 - Skip over scrollinfo layers instead of aborting entirely.

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

Comment 22

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

Comment on attachment 8873870 [details]
Bug 1368496 - Extract a helper function from WebRenderLayer::ClipRect().

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

Comment 23

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

Comment on attachment 8873871 [details]
Bug 1368496 - Propagate the layer's "scrolled clip" to WebRender.

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

Comment 24

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

Comment on attachment 8873872 [details]
Bug 1368496 - Track the pushed clip and scroll IDs.

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

Comment 25

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

Comment on attachment 8873873 [details]
Bug 1368496 - Expose the push_clip_and_scroll_info API in WR via WebRenderAPI.

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

Comment 26

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

Comment on attachment 8873874 [details]
Bug 1368496 - Add support for fixed-positioning with APZ.

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

Comment 27

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

^ rebased and inserted a new small patch as part 2. With the new code added in the later parts we need to make sure we don't early-exit from the function beforehand.

Comment 28

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

Updated try push is green so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f088967f31548dfdb5b70ee63fca36a04fda01ee

Comment 29

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> > +  LayerToParentLayerMatrix4x4 transform = GetLayer()->GetLocalTransformTyped();
> > +  return transform.Inverse().TransformBounds(IntRectToRect(aClipRect));
> 
> I prefer not to have magical Inverse()s in helpers like this, but I don't
> yet understand how it's used. I look into it more.

After working through another reftest I am starting to think that this Inverse() is going to be unnecessary. The reason we have it is because we generally push the clip after pushing the stacking context. Therefore we need to unapply the transform from the stacking context in order to get the clip to come out right. But really what we should probably do is push the clip *before* we push the stacking context, and then we wouldn't need to do this inverse transform.

However I'd like to punt that to a follow-up bug, as that's a pre-existing problem. The patch in this bug is just a refactoring, so it doesn't affect the existing code.

Comment 30

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

Comment on attachment 8875295 [details]
Bug 1368496 - Skip over scrollinfo layers instead of aborting entirely.

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

Comment 31

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

Comment on attachment 8873871 [details]
Bug 1368496 - Propagate the layer's "scrolled clip" to WebRender.

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

Comment 32

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

Comment on attachment 8873872 [details]
Bug 1368496 - Track the pushed clip and scroll IDs.

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

Comment 33

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

Comment on attachment 8873873 [details]
Bug 1368496 - Expose the push_clip_and_scroll_info API in WR via WebRenderAPI.

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

Comment 34

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

Comment on attachment 8873874 [details]
Bug 1368496 - Add support for fixed-positioning with APZ.

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

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8875295 [details]
Bug 1368496 - Skip over scrollinfo layers instead of aborting entirely.

https://reviewboard.mozilla.org/r/146704/#review151354

Comment 36

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

New patches remove the UntransformClip call which I now believe is incorrect. So that removes what used to be part 1, and modifies what used to be part 3 (now part 2) to remove the call to UntransformClip.

Try pushes with [1] and without [2] APZ enabled show basically the same results as before.

I have more patches that I'm working on to move the clip pushing to before the stacking context push, so that we don't need to untransform the clip rect any more.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=de6f52da3bf8617644c6648d5a13561a03135e8b
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=932edd064b6d2a2d15ee770122c385a937ccda5a

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de72271be8b8
Skip over scrollinfo layers instead of aborting entirely. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/6ca11746a8fc
Propagate the layer's "scrolled clip" to WebRender. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/fd266694b1f8
Track the pushed clip and scroll IDs. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/c48397f4c2e2
Expose the push_clip_and_scroll_info API in WR via WebRenderAPI. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/dcffcfdf03ad
Add support for fixed-positioning with APZ. r=jrmuizel

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de72271be8b8
https://hg.mozilla.org/mozilla-central/rev/6ca11746a8fc
https://hg.mozilla.org/mozilla-central/rev/fd266694b1f8
https://hg.mozilla.org/mozilla-central/rev/c48397f4c2e2
https://hg.mozilla.org/mozilla-central/rev/dcffcfdf03ad