Closed Bug 1357754 Opened 7 years ago Closed 7 years ago

Hook up hit-testing and scroll metadata to APZ when webrender is in use

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(10 files)

Bug 1357754 - Add a mechanism to send scroll data to APZ over PWebRenderBridge.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1357754 - Extract a template function from UpdateHitTestingTree.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1357754 - Replace LayerMetricsWrapper::AsRefLayer with GetReferentId.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1357754 - Add WebRenderScrollDataWrapper.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1357754 - Implement the traversal functions of WebRenderScrollDataWrapper.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1357754 - Add ScrollDirection IPC serialization code.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1357754 - Add more data to the WebRenderScrollData object.
59 bytes, text/x-review-board-request
botond
: review+
jrmuizel
: review+
Details
Bug 1357754 - Hook up chaining layer trees.
59 bytes, text/x-review-board-request
botond
: review+
jrmuizel
: review+
Details
Bug 1357754 - Record the isFirstPaint flag in the WebRenderScrollData.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1357754 - Trigger the APZ hit-testing tree rebuild.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
In Gecko normally, we send layer tree updates over PLayerTransaction and then call APZCTreeManager::UpdateHitTestingTree when those layer tree updates are received in CompositorBridgeParent/CrossProcessCompositorBridgeParent. With WebRender, we don't use PLayerTransaction and don't have layer trees being sent over to the compositor. Instead we build a webrender display list from the layer tree on the client side, and send a blob of already-serialized stuff over PWebRenderBridge, which is then fed into the WR compositor.

For APZ this means we can't use existing mechanism as-is, but instead we need to pull off what we need from the client-side layer tree, bundle that up, and send it over PWebRenderBridge. Then on the parent side, in WebRenderBridgeParent, we need to pick that up and somehow glue it to APZCTreeManager::UpdateHitTestingTree.

I have patches that do this. They're a little hard to test exhaustively because without hooking up the AsyncCompositionManager-equivalent code we don't actually get async scrolling happening. But I got to a "checkpoint" state where, if you:
1) apply the patches
2) run with gfx.webrender.enabled=true and apz.allow_with_webrender=true
3) turn off smooth scrolling (via about:preferences)

then mousewheel scrolling seems to work. At least, the APZ data structures seem to be populated and working reasonably correctly, it sends repaint requests back to the main thread, etc. APZ animations don't work yet, because those need AsyncCompositionManager hookup.

I think this is a good point to land what I have (it will be behind the apz.allow_with_webrender pref, disabled by default).

Also note that even the reftest-sanity suite doesn't pass with apz.allow_with_webrender=true because it tries to use DOMWindowUtils.setAsyncScrollOffset which isn't hooked up in webrender (that's bug 1316906).
Comment on attachment 8859601 [details]
Bug 1357754 - Add a mechanism to send scroll data to APZ over PWebRenderBridge.

https://reviewboard.mozilla.org/r/131600/#review134390

::: gfx/layers/wr/WebRenderLayerManager.cpp:339
(Diff revision 1)
>    return false;
>  }
>  
> +/*static*/ int32_t
> +PopulateScrollData(WebRenderScrollData& aTarget, Layer* aLayer)
> +{

This is a cute function.
Attachment #8859601 - Flags: review?(jmuizelaar) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=23ebc8dab98a51745ddba8f97ff79cf23089688d

This has debug build bustage which I fixed locally. I also fixed up some code comments in a few of the patches, no code changes. New try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b254af9143369cebc9b63258ee50d4c91644c3

I'll push the updated patches as well since the updated code comments should help with reviewing.
Comment on attachment 8859607 [details]
Bug 1357754 - Add more data to the WebRenderScrollData object.

https://reviewboard.mozilla.org/r/131612/#review134458
Attachment #8859607 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8859609 [details]
Bug 1357754 - Record the isFirstPaint flag in the WebRenderScrollData.

https://reviewboard.mozilla.org/r/131616/#review134460
Attachment #8859609 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8859610 [details]
Bug 1357754 - Trigger the APZ hit-testing tree rebuild.

https://reviewboard.mozilla.org/r/131618/#review134462
Attachment #8859610 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8859602 [details]
Bug 1357754 - Extract a template function from UpdateHitTestingTree.

https://reviewboard.mozilla.org/r/131602/#review134564

Please replace "T" with a descriptive name, such as "Node", "LayerNode", "LayerTreeNode", etc.
Attachment #8859602 - Flags: review?(botond) → review+
Comment on attachment 8859603 [details]
Bug 1357754 - Replace LayerMetricsWrapper::AsRefLayer with GetReferentId.

https://reviewboard.mozilla.org/r/131604/#review134570

::: gfx/layers/composite/AsyncCompositionManager.cpp:411
(Diff revision 2)
>      // ancestor with the scroll id |aFixedWithRespectTo| (this could happen
>      // e.g. if the scroll frame with that scroll id uses containerless
>      // scrolling). In such a case, stop the walk, as a new layers id could
>      // have a different layer with scroll id |aFixedWithRespectTo| which we
>      // don't intend to match.
> -    if (current && current.AsRefLayer() != nullptr) {
> +    if (current && current.GetReferentId().isSome()) {

Consider adding IsRefLayer() for readability.
Attachment #8859603 - Flags: review?(botond) → review+
Comment on attachment 8859604 [details]
Bug 1357754 - Add WebRenderScrollDataWrapper.

https://reviewboard.mozilla.org/r/131606/#review134574

::: gfx/layers/apz/src/APZCTreeManager.cpp:359
(Diff revision 2)
> +                                      const WebRenderScrollData& aScrollData,
> +                                      bool aIsFirstPaint,
> +                                      uint64_t aOriginatingLayersId,
> +                                      uint32_t aPaintSequenceNumber)
> +{
> +  WebRenderScrollDataWrapper wrapper(&aScrollData);

Any reason not to just make the template version public, and have the call site perform construction of the wrapper object?

::: gfx/layers/wr/WebRenderScrollDataWrapper.h:160
(Diff revision 2)
> +  {
> +    // TODO
> +    return 0;
> +  }
> +
> +  const void* GetLayer() const

Pretty sure you don't need this function.
Attachment #8859604 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #30)
> 
> Any reason not to just make the template version public, and have the call
> site perform construction of the wrapper object?

Is there a way to do that without putting the body of the function in the .h file?

> ::: gfx/layers/wr/WebRenderScrollDataWrapper.h:160
> > +  const void* GetLayer() const
> 
> Pretty sure you don't need this function.

There's some APZCTM_LOG statements that use it. It's just needed to provide some value to print.
Comment on attachment 8859605 [details]
Bug 1357754 - Implement the traversal functions of WebRenderScrollDataWrapper.

https://reviewboard.mozilla.org/r/131608/#review134600

::: gfx/layers/wr/WebRenderScrollData.h:87
(Diff revision 2)
> +  size_t GetLayerCount() const;
> +
>    // Return a pointer to the scroll data at the given index. Use with caution,
>    // as the pointer may be invalidated if this WebRenderScrollData is mutated.
>    WebRenderLayerScrollData* GetLayerDataMutable(size_t aIndex);
> +  const WebRenderLayerScrollData* GetLayerData(size_t aIndex) const;

This isn't Rust, you can overload the method on constness :) (If you specifically don't want to, to call extra attention to mutable accesses in call sites, that's fine.)

::: gfx/layers/wr/WebRenderScrollDataWrapper.h:309
(Diff revision 2)
> +  // The upper bound on the set of valid indices inside the subtree rooted at
> +  // the parent of this "layer". That is, any layer index |i| in the range
> +  // mLayerIndex <= i < mContainingSubtreeLastIndex is guaranteed to point to
> +  // a layer that is a descendant of "parent", where "parent" is the parent
> +  // layer of the layer at mLayerIndex.
> +  size_t mContainingSubtreeLastIndex;

Clever :)

Might be worth mentioning that this is needed by GetPrevSibling().
Attachment #8859605 - Flags: review?(botond) → review+
Comment on attachment 8859601 [details]
Bug 1357754 - Add a mechanism to send scroll data to APZ over PWebRenderBridge.

https://reviewboard.mozilla.org/r/131600/#review134594

::: gfx/layers/wr/WebRenderScrollData.h:42
(Diff revision 2)
> +                  int32_t aDescendantCount);
> +
> +  friend struct IPC::ParamTraits<WebRenderLayerScrollData>;
> +
> +private:
> +  // The number of descendants this layer had (not including the layer itself).

had, at some point in the past, or has?
Comment on attachment 8859606 [details]
Bug 1357754 - Add ScrollDirection IPC serialization code.

https://reviewboard.mozilla.org/r/131610/#review134612
Attachment #8859606 - Flags: review?(botond) → review+
Comment on attachment 8859607 [details]
Bug 1357754 - Add more data to the WebRenderScrollData object.

https://reviewboard.mozilla.org/r/131612/#review134620
Attachment #8859607 - Flags: review?(botond) → review+
Comment on attachment 8859608 [details]
Bug 1357754 - Hook up chaining layer trees.

https://reviewboard.mozilla.org/r/131614/#review134624

LGTM but please flag :jrmuizel for review on this patch too.

Thanks for the nicely organized patch series!
Attachment #8859608 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #28)
> Please replace "T" with a descriptive name, such as "Node", "LayerNode",
> "LayerTreeNode", etc.

I called it "ScrollNode". Tough to pick a name that wasn't too long yet appropriately descriptive :/

(In reply to Botond Ballo [:botond] from comment #29)
> Consider adding IsRefLayer() for readability.

I realized that I could just the existing AsRefLayer() function in the LayerMetricsWrapper as-is. The new GetReferentId() function is in both LayerMetricsWrapper and WebRenderScrollDataWrapper. The APZ code uses GetReferentId() and so works fine, and the AsyncCompositionManager code I left untouched so it continues to work with LayerMetricsWrapper::AsRefLayer. It doesn't need to know about WebRenderScrollDataWrapper.

(In reply to Botond Ballo [:botond] from comment #32)
> >    WebRenderLayerScrollData* GetLayerDataMutable(size_t aIndex);
> > +  const WebRenderLayerScrollData* GetLayerData(size_t aIndex) const;
> 
> This isn't Rust, you can overload the method on constness :) (If you
> specifically don't want to, to call extra attention to mutable accesses in
> call sites, that's fine.)

Yeah that initially got left over from my earlier bumbling attempts at const'ing stuff. And then I discovered I liked making it explicit in the function name so I left it.

> ::: gfx/layers/wr/WebRenderScrollDataWrapper.h:309
> Clever :)
> 
> Might be worth mentioning that this is needed by GetPrevSibling().

Thanks. I updated the comment to note that.

(In reply to Botond Ballo [:botond] from comment #33)
> > +  // The number of descendants this layer had (not including the layer itself).
> 
> had, at some point in the past, or has?

"has" is more correct, fixed.

(In reply to Botond Ballo [:botond] from comment #36)
> LGTM but please flag :jrmuizel for review on this patch too.

Flagged

> Thanks for the nicely organized patch series!

Thanks for the quick reviews!
Comment on attachment 8859608 [details]
Bug 1357754 - Hook up chaining layer trees.

https://reviewboard.mozilla.org/r/131614/#review134868
Attachment #8859608 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/9801b8ad5dc2
Add a mechanism to send scroll data to APZ over PWebRenderBridge. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/022304bf1ad7
Extract a template function from UpdateHitTestingTree. r=botond
https://hg.mozilla.org/projects/graphics/rev/9ce54d2f9c4f
Replace LayerMetricsWrapper::AsRefLayer with GetReferentId. r=botond
https://hg.mozilla.org/projects/graphics/rev/2a256d666481
Add WebRenderScrollDataWrapper. r=botond
https://hg.mozilla.org/projects/graphics/rev/1f032699c9c5
Implement the traversal functions of WebRenderScrollDataWrapper. r=botond
https://hg.mozilla.org/projects/graphics/rev/e8979e2cb9b7
Add ScrollDirection IPC serialization code. r=botond
https://hg.mozilla.org/projects/graphics/rev/ea612503b63f
Add more data to the WebRenderScrollData object. r=botond,jrmuizel
https://hg.mozilla.org/projects/graphics/rev/e891376e9f56
Hook up chaining layer trees. r=botond,jrmuizel
https://hg.mozilla.org/projects/graphics/rev/9a89384b0d2c
Record the isFirstPaint flag in the WebRenderScrollData. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/c003db4fe3a4
Trigger the APZ hit-testing tree rebuild. r=jrmuizel
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> (In reply to Botond Ballo [:botond] from comment #30)
> > 
> > Any reason not to just make the template version public, and have the call
> > site perform construction of the wrapper object?
> 
> Is there a way to do that without putting the body of the function in the .h
> file?

Whoops, I overlooked this comment yesterday. Good point.

(You could write explicit template instantiation statements in the .cpp file for the two concrete wrapper types, but that would negate both the "we don't need to know what concrete types will be passed in" aspect, and arguably the simplicity aspect of my suggestion, so let's just stick with the way it is.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: