Closed Bug 1443792 Opened 6 years ago Closed 6 years ago

Reduce usage of AsyncPanZoomController class in the codebase

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 5 obsolete files)

Bug 1443792 - Move the SampleAPZAnimations function into APZSampler.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1443792 - Move ComputeTransformForScrollThumb to APZCTreeManager.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1443792 - Tighten the equivalence between a layer being scrollable and having an APZC.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1443792 - Remove direct access to AsyncPanZoomController from AsyncCompositionManager.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1443792 - Remove direct access to AsyncPanZoomController from ContainerLayerComposite.
59 bytes, text/x-review-board-request
botond
: review+
Details 
This is the same as bug 1442627, but for AsyncPanZoomController:

In order to prepare for the APZ sampler thread getting mapped to the render backend thread when WR is enabled, we should make sure that all the things that are supposed to happen on the sampler thread are clearly identified. I plan to do this by making sure all of those things go through the APZSampler interface class introduced in bug 1441916. And therefore less code should be using AsyncPanZoomController directly.
One of the things I wanted to do as part of this was remove the pointers to AsyncPanZoomController instances that we stash on the layer tree [1]. We use these pointers quite a bit and my plan was to instead have an unordered_map<ScrollableLayerGuid, RefPtr<AsyncPanZoomController>> in the APZCTreeManager to do the equivalent job, and then places like AsyncCompositionManager that currently pull the APZC from the layer would instead pass a guid to the APZSampler interface to accomplish an equivalent job. However in order to do this, it needs to be easy get from a Layer (or LayerMetricsWrapper/WebRenderScrollDataWrapper, which is mostly what we use when walking the layer tree) to a ScrollableLayerGuid. This is not easy right now, because the LayerMetricsWrapper doesn't hold the layers id which is a required piece of the guid.

I came up with a couple of possible approaches, and wanted to feedback on these or other possibilities in terms of how to do this.

1) Have users of LayerMetricsWrapper be responsible for finding the layers and providing it to the LayerMetricsWrapper during construction. I think this would have to be optional - as in, not all users will need to, or be able to, get a hold of the layers id (e.g. ClientTiledPaintedLayer). So the LayerMetricsWrapper would basically have a Maybe<uint64_t> that stores the layers id if it has one, and can only produce ScrollableLayerGuids in those cases. We would assert that only LayerMetricsWrappers that have this would be allowed in APZSampler methods that require looking up the APZC. As users "walk the tree" using the LayerMetricsWrappers tree-navigation methods, the layers is could mostly be propagated directly from one LayerMetricsWrapper to another (walking down from a ref layer would pick up the new layers id). However walking *up* the tree into a reflayer means the layers id would be lost, because there's no easy way to recover that. So that's kind of a "gotcha" that we'd have to watch out for.

2) Store the layers id on the FrameMetrics alongside the scroll id. The catch here is that the layers id is sensitive security-wise, and we shouldn't allow the content processes (which create the FrameMetrics) to set them, since a malicious content process could spoof the layers id for another content process. So the content process FrameMetrics would never set the layers id, but when it crosses the IPDL bridge to the compositor, we can populate it at that point (in LayerTransactionParent and WebRenderBridgeParent).

I spent a while thinking about (1) and it feels like a pretty bad approach overall, for various reasons. I only came up with (2) last night, but it seems much better. With this approach the LayerMetricsWrapper and WebRenderScrollDataWrapper both have easy access to the ScrollMetadata/FrameMetrics and so will be able to produce the guid easily in the compositor, which is where we would need them. It's also more straightforward to reason about.

However there are other options that might be preferable: (3) don't bother with this at all, and just leave AsyncPanZoomController exposed on the Layer, or (4) use something other than a map from ScrollableLayerGuid -> AsyncPanZoomController. Botond, do you have any thoughts or suggestions? I'm happy to go ahead and implement (2) to see what it looks like, and if you don't like it or we come up with something better we can throw it out or modify it as needed.

[1] https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/gfx/layers/Layers.h#1802-1803
Flags: needinfo?(botond)
We discussed this on IRC. To summarize the suggestions I made:

  (5) Store a layers id field on Layer, set during UpdateHitTestingTree
      whenever we currently set the APZC field.

  (6) Keep storing the APZC on Layer, but wrapped in a class that only
      exposes the relevant sampler-thread interfaces (similar to how
      APZSampler wraps AsyncPanZoomController).

I'm also OK with doing (3) for now (i.e. just leaving the APZC pointers), if we want to unblock progress on the render backend thread switch.
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #2)
>   (6) Keep storing the APZC on Layer, but wrapped in a class that only
>       exposes the relevant sampler-thread interfaces (similar to how
>       APZSampler wraps AsyncPanZoomController).

Er, "similar to how APZSampler wraps APZCTreeManager".
I think for now I'll go with (3) and leave the APZC pointers as-is because it's not clear to me which of (2), (5) or (6) would be better for the long term. For the short term I'm just going to put the methods on APZSampler to make sure they're clearly identified. But if we go with (6) these can be trivially moved to the APZCSampler class instead.

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=98be9605b05866725c1ba4d78435c0b12b3fd20d
Assignee: nobody → bugmail
Priority: -- → P3
Whiteboard: [gfx-noted]
Attachment #8957251 - Attachment is obsolete: true
Attachment #8957251 - Flags: review?(botond)
Attachment #8957252 - Attachment is obsolete: true
Attachment #8957252 - Flags: review?(botond)
Attachment #8957253 - Attachment is obsolete: true
Attachment #8957253 - Flags: review?(botond)
Attachment #8957254 - Attachment is obsolete: true
Attachment #8957254 - Flags: review?(botond)
Attachment #8957255 - Attachment is obsolete: true
Attachment #8957255 - Flags: review?(botond)
Comment on attachment 8957707 [details]
Bug 1443792 - Move the SampleAPZAnimations function into APZSampler.

https://reviewboard.mozilla.org/r/226648/#review232514
Attachment #8957707 - Flags: review?(botond) → review+
Comment on attachment 8957708 [details]
Bug 1443792 - Move ComputeTransformForScrollThumb to APZCTreeManager.

https://reviewboard.mozilla.org/r/226650/#review232516

I assume the implementation is a straight move without modifications.

::: gfx/layers/apz/public/APZSampler.h:80
(Diff revision 1)
>                          const LayerToParentLayerScale& aZoom);
>  
>    bool SampleAnimations(const LayerMetricsWrapper& aLayer,
>                          const TimeStamp& aSampleTime);
>  
> +  LayerToParentLayerMatrix4x4 ComputeTransformForScrollThumb(

Please describe |aContent|, and refer readers to APZCTreeManager::ComputeTransformForScrollThumb() for a description of the other parameters.
Attachment #8957708 - Flags: review?(botond) → review+
Comment on attachment 8957709 [details]
Bug 1443792 - Tighten the equivalence between a layer being scrollable and having an APZC.

https://reviewboard.mozilla.org/r/226652/#review232520
Attachment #8957709 - Flags: review?(botond) → review+
Comment on attachment 8957710 [details]
Bug 1443792 - Remove direct access to AsyncPanZoomController from AsyncCompositionManager.

https://reviewboard.mozilla.org/r/226654/#review232522

::: gfx/layers/apz/src/APZSampler.cpp:172
(Diff revision 1)
>  }
>  
> +AsyncTransform
> +APZSampler::GetCurrentAsyncTransform(const LayerMetricsWrapper& aLayer)
> +{
> +  return aLayer.GetApzc()->GetCurrentAsyncTransform(AsyncPanZoomController::eForCompositing);

Do we want to add some asserts that aLayer.GetApzc() is not null here?
Attachment #8957710 - Flags: review?(botond) → review+
Comment on attachment 8957711 [details]
Bug 1443792 - Remove direct access to AsyncPanZoomController from ContainerLayerComposite.

https://reviewboard.mozilla.org/r/226656/#review232524

::: gfx/layers/apz/src/APZSampler.cpp:205
(Diff revision 1)
> +APZSampler::HasUnusedAsyncTransform(const LayerMetricsWrapper& aLayer)
> +{
> +  AsyncPanZoomController* apzc = aLayer.GetApzc();
> +  return apzc
> +      && !apzc->GetAsyncTransformAppliedToContent()
> +      && !AsyncTransformComponentMatrix(apzc->GetCurrentAsyncTransform(AsyncPanZoomController::eForHitTesting)).IsIdentity();

This is an existing issue, but I think using eForHitTesting here is wrong: it should be using eForCompositing, especially now that we have the APZ frame delay, since we could have an eForHitTesting async transform in one frame that hasn't been applied by AsyncCompositionManager yet because of the frame delay.

::: gfx/layers/composite/ContainerLayerComposite.cpp:371
(Diff revision 1)
>    // If enabled, render information about visibility.
>    if (gfxPrefs::APZMinimapVisibilityEnabled()) {
> -    // Retrieve the APZC scrollable layer guid, which we'll use to get the
> +    // Retrieve the scrollable layer guid, which we'll use to get the
>      // appropriate visibility information from the layer manager.
> -    AsyncPanZoomController* controller = aLayer->GetAsyncPanZoomController(0);
> -    MOZ_ASSERT(controller);
> +    MOZ_ASSERT(compositor->GetCompositorBridgeParent()); // we wouldn't have an APZSampler otherwise
> +    ScrollableLayerGuid guid(compositor->GetCompositorBridgeParent()->RootLayerTreeId(), fm);

Why is the layers id the root layers id?
Attachment #8957711 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #16)
> I assume the implementation is a straight move without modifications.

Yup.

> ::: gfx/layers/apz/public/APZSampler.h:80
> Please describe |aContent|, and refer readers to
> APZCTreeManager::ComputeTransformForScrollThumb() for a description of the
> other parameters.

Done

(In reply to Botond Ballo [:botond] from comment #18)
> > +  return aLayer.GetApzc()->GetCurrentAsyncTransform(AsyncPanZoomController::eForCompositing);
> 
> Do we want to add some asserts that aLayer.GetApzc() is not null here?

Good point, done (in all the related functions)

(In reply to Botond Ballo [:botond] from comment #19)
> This is an existing issue, but I think using eForHitTesting here is wrong:
> it should be using eForCompositing, especially now that we have the APZ
> frame delay, since we could have an eForHitTesting async transform in one
> frame that hasn't been applied by AsyncCompositionManager yet because of the
> frame delay.

Filed bug 1445019 for this, I'll put a patch up there.

> ::: gfx/layers/composite/ContainerLayerComposite.cpp:371
> > +    ScrollableLayerGuid guid(compositor->GetCompositorBridgeParent()->RootLayerTreeId(), fm);
> 
> Why is the layers id the root layers id?

Good catch. I think this got messed up when I untangled my patch queue to remove my attempt at (2) from comment 1. Anyhow, the code in question is for the frame-visibility component of the APZ minimap which I just deleted in bug 1258238, so this isn't an issue any more.
Comment on attachment 8957711 [details]
Bug 1443792 - Remove direct access to AsyncPanZoomController from ContainerLayerComposite.

https://reviewboard.mozilla.org/r/226656/#review232930
Attachment #8957711 - Flags: review?(botond) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d6d92d8bb2b
Move the SampleAPZAnimations function into APZSampler. r=botond
https://hg.mozilla.org/integration/autoland/rev/5a049955eaf3
Move ComputeTransformForScrollThumb to APZCTreeManager. r=botond
https://hg.mozilla.org/integration/autoland/rev/cfec3ca8b042
Tighten the equivalence between a layer being scrollable and having an APZC. r=botond
https://hg.mozilla.org/integration/autoland/rev/9957f3a8d0c3
Remove direct access to AsyncPanZoomController from AsyncCompositionManager. r=botond
https://hg.mozilla.org/integration/autoland/rev/b34c4056a6d1
Remove direct access to AsyncPanZoomController from ContainerLayerComposite. r=botond
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: