890280 - APZC input event transformations should consist of the async transform relative to the correct coordinate system

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #36)
> I just realized that part 3 here is not actually correct, with respect to
> the return value of aRelativePointOut. In the case where there is no CSS
> transforms set it's fine, but once CSS transforms are applied, then the
> aLayer.GetLocalTransform().Inverse() will include the un-transformation of
> those transforms, which we don't really want to do. Layout is already aware
> of those transforms and will unapply them during its own hit testing, so the
> aRelativePointOut that we want to return shouldn't have them unapplied.
> 
> I think the aRelativePointOut value should be the aPoint untransformed by
> the ViewTransform that SampleContentTransformForFrame returns - that is the
> async pan/zoom transform, which doesn't include any CSS transforms. Matt
> Woodrow pointed me to nsDisplayTransform::HitTest [1] which is the code that
> does the CSS un-transform for hit testing, so the point that we pass to
> layout should still include CSS transforms.
> 
> (Another way of looking at this is that the current patch will return a
> LayerPoint, but it's actually in the "LayerPoint minus CSS transforms"
> coordinate space).
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#4002

Comment 1

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

Also:

I think the fix for the issue should look something like this: https://github.com/staktrace/mozilla-central/commit/975d8df103d81fd9f50badd82e9fd82bcc544fd6

I'm working on a more comprehensive fix that also deals with the threading problems in this function. Stay tuned.

Comment 2

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

Attachment: WIP part 1 - Extract a GetCurrentAsyncTransform method callable from the UI thread

This pulls out parts of SampleContentTransformForFrame so that there is a standalone function that just returns the transform, and all the other stuff is left behind. The new function is callable from the UI thread as well. It will be needed in part 2.

Comment 3

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

Attachment: WIP part 2 - Move GetAPZCAtPoint over to CompositorParent and the UI thread

This is a very rough WIP that hopefully shows what I'm proposing. In a nutshell, we have a tree of APZCs, where there is a 1:1 mapping between scrollable layers and APZCs. The APZC tree has the same shape as the layer tree with all the non-scrollable layers taken out. The idea here is that the visible regions of all the layers that are panned/zoomed by an APZC are collapsed into a single visible region. This happens at layer update time, and the tree structure of the APZCs is not allowed to be mutated at any other time.

Then, when we need to do hit detection, we basically do what BenWa's code was doing before, except we do it in the APZC tree. The point is untransformed by the "async transform" (i.e. what the APZC has done based on user's interaction) and so can be hit tested directly against what Gecko sent over in the layer tree.

I made up some functions that allow this to work, those functions will need to be implemented.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 771517 [details] [diff] [review]
WIP part 2 - Move GetAPZCAtPoint over to CompositorParent and the UI thread

Review of attachment 771517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few tweaks which will make this code easier to understand. f+ since I think the approach is sound.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +648,5 @@
> +
> +private:
> +  nsRegion mVisibleRegion;
> +  nsRefPtr<AsyncPanZoomController> mLastChild;
> +  nsRefPtr<AsyncPanZoomController> mPrevSibling;

I've never seen a tree definition that keeps these kind of links but it's useful for hit testing.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +573,5 @@
> +
> +/* Build a tree of APZC instances that has the same shape as the layer tree,
> +   but excludes all the layers that are not scrollable. Note that this means
> +   APZCs corresponding to layers at different levels in the tree may end up
> +   becoming siblings. It also means the "root" APZC may have siblings. */

This is an annoying property to have but it makes sense :(.

@@ +597,5 @@
> +      aParent->SetLastChild(controller);
> +    } else {
> +      mRootApzc = controller;
> +    }
> +    aParent = controller;

I would kill this line since it's confusing. See below:

@@ +600,5 @@
> +    }
> +    aParent = controller;
> +  }
> +
> +  if (aParent) {

aParent == controller but it's clearer if you use 'controller' here and below.

@@ +606,5 @@
> +    // the closest ancestor APZC.
> +
> +    // XXX Implement code so that the following line actually compiles and works.
> +    nsRegion layerRegion = aLayer->GetCumulativeTransform().Transform(aLayer->GetVisibleRegion());
> +    aParent->AddVisibleRegion(layerRegion);

We're not accumulating a visible region since we always clear and do a single add. Maybe all we want is a single SetVisibleRegion.

@@ +612,5 @@
> +
> +  AsyncPanZoomController *next = nullptr;
> +  for (Layer* child = aLayer->GetLastChild();
> +      child; child = child->GetPrevSibling()) {
> +    next = UpdatePanZoomControllerTree(child, aParent, next);

I would replace aParent by controller here.

@@ +624,5 @@
> +  }
> +  return aNextSibling;
> +}
> +
> +/* Called from the UI thread, NOT the compositor thread */

Is it worth asserting that we're the UI thread?

::: gfx/layers/ipc/CompositorParent.h
@@ +262,5 @@
>  
>    DISALLOW_EVIL_CONSTRUCTORS(CompositorParent);
> +
> +private:
> +  void UpdatePanZoomControllerTree(Layer* aRoot);

Can we introduce a new 'APZCTree' object that contains everything we've just added in this patch? Let's keep Compositor clean.

Comment 5

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

Comment on attachment 771515 [details] [diff] [review]
WIP part 1 - Extract a GetCurrentAsyncTransform method callable from the UI thread

Obsoleting this, bug 866232 takes care of this

Comment 6

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

Comment on attachment 771517 [details] [diff] [review]
WIP part 2 - Move GetAPZCAtPoint over to CompositorParent and the UI thread

Ditto

Comment 7

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

I'm morphing this bug to generally cover the input event transformations done by the APZC, as per the patches in bug 866232. I realized that there are actually two un-transformations that APZC needs to do, and they are subtly different.

(1) The first is the one that is executed on the ReceiveInputEvent(nsInputEvent& aEvent, nsInputEvent& aOutEvent) codepath. In this code path, layout is asking the APZC to untransform the input event by the async transform, so that it can dispatch the touch event to the right content element.

The second is the one that is executed in things like OnSingleTapConfirmed, where the APZC is notifying the callback of gestures.

Both of these currently use the WidgetSpaceToCompensatedViewportSpace function, which just divides by a scale factor. Really this function should be deleted and replaced with two different functions:

For code path (1) the input event should be untransformed by the async transform, relative to the last layers update. This is because the ReceiveInputEvent function in question is run on the gecko thread, and the thing it returns to layout must be relative to where layout thinks everything is. That coordinate system, I believe, will be the same as the last layers update received by the APZC, which is stored in mLastContentPaintMetrics.

For code path (2) the input event should be untransformed by the async transform, relative to the last paint request issued by APZC. This is because this code runs on the controller thread, and may be interleaved with calls to RequestContentRepaint() which are happening independent of gecko actually repainting and issuing layers updates. The gesture event dispatched to the controller should be relative to the last coordinate system that was sent via RequestContentRepaint, which will be what the controller is aware of. This is stored in the mLastPaintRequestMetrics field in APZC.

Comment 9

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

Attachment: WIP part 1 - Ensure APZCTreeManager gets coordinates relative to root

The APZCTreeManager class I added recently tracks the entire layer tree, even across processes. Therefore the input events it receives from the RenderFrameParent need to be screen-relative. This patch moves the call that translates the events to the child process space from before the call to MaybeForwardEventToRenderFrame to after it.

Comment 10

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

Attachment: WIP part 2 - Fix up input event transformations for APZC and touch events

Please read the big comment before trying to understand the code! Also note these patches apply on top of the one from bug 900742.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 785513 [details] [diff] [review]
WIP part 1 - Ensure APZCTreeManager gets coordinates relative to root

Could we move nsEventStateManager::MapEventCoordinatesForChildProcess to TabParent?

+TabParent::MapEventCoordinatesToChild(nsEvent* e)
s/e/aEvent/

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 785515 [details] [diff] [review]
WIP part 2 - Fix up input event transformations for APZC and touch events

Review of attachment 785515 [details] [diff] [review]:
-----------------------------------------------------------------

The complexity of APZCTreeManager.cpp is growing very quickly :(

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +432,5 @@
> +   Next, if we want user inputs sent to gecko for event-dispatching, we will need to strip
> +   out all of the async transforms that are involved in this chain. This is because async
> +   transforms are stored only in the compositor and gecko does not account for them when
> +   doing display-list-based hit-testing for event dispatching. Therefore, given a user input
> +   in screen space, the following transforms need to be applied (in order from top to bottom):

We've talked about this. It's a bit scary that we don't how what state gecko will be when the queued event is processed but we're already in that state. What's worse is by the time we receive our event the main thread's transform might of changed and we might have a layer transaction queued in the compositor behind the event we're processing. So we might already be out of date when computing this event. I think we should continue for now.

@@ +508,3 @@
>  {
> +  // The comments below assume there is a chain of layers L..R with L and P having APZC instances as
> +  // explained in the big comment above. This function will recurse with aApzc at L and R, and the

'as explained by the comment in ::GetTargetAPZC'

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ -247,5 @@
>    return aPoint / aCurrentZoom;
>  }
>  
> -nsEventStatus
> -AsyncPanZoomController::ReceiveInputEvent(const nsInputEvent& aEvent,

Is this entirely handled by APZCTreeManager::ReceiveInputEvent now?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +656,5 @@
>    LayerRect mVisibleRect;
> +  /* This is the cumulative CSS transform for all the layers between the parent
> +   * APZC and this one (not inclusive) */
> +  gfx3DMatrix mAncestorTransform;
> +  /* This is the layer transform for this APZC's layer. */

This comment is confusing. It's the layer transform but it's called CSSTransform.

Comment 13

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

Attachment: Part 1 - Ensure APZCTreeManager gets coordinates relative to root

Cleaned up version

Comment 14

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

Attachment: Part 2 - Fix up input event transformations for APZC and touch events

Now with proper matrix multiplication. The events sent out through the GeckoContentController still need to be transformed properly, but really I just want to get rid of those entirely; see bug 900638 comment 3.

Comment 15

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

Comment on attachment 788529 [details] [diff] [review]
Part 2 - Fix up input event transformations for APZC and touch events

Review of attachment 788529 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Benoit Girard (:BenWa) from comment #12)
> @@ +508,3 @@
> >  {
> > +  // The comments below assume there is a chain of layers L..R with L and P having APZC instances as
> > +  // explained in the big comment above. This function will recurse with aApzc at L and R, and the
> 
> 'as explained by the comment in ::GetTargetAPZC'

Fixed

> > -nsEventStatus
> > -AsyncPanZoomController::ReceiveInputEvent(const nsInputEvent& aEvent,
> 
> Is this entirely handled by APZCTreeManager::ReceiveInputEvent now?

Yes

> ::: gfx/layers/ipc/AsyncPanZoomController.h
> > +  /* This is the layer transform for this APZC's layer. */
> 
> This comment is confusing. It's the layer transform but it's called
> CSSTransform.

Fixed the comment.

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 788529 [details] [diff] [review]
Part 2 - Fix up input event transformations for APZC and touch events

Review of attachment 788529 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +380,3 @@
>  
>    // No APZC attached so hit testing will return no APZC at (20,20)
> +  nsRefPtr<AsyncPanZoomController> hit = manager->GetTargetAPZC(ScreenPoint(20, 20), matrix, matrix);

Not a fan of passing the same value twice by reference. While compilers may not assume strict aliasing it's easy for the implementation to assume it. Why not test the values here?

Comment 17

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

(In reply to Benoit Girard (:BenWa) from comment #16)
> Not a fan of passing the same value twice by reference. While compilers may
> not assume strict aliasing it's easy for the implementation to assume it.
> Why not test the values here?

I was waiting to do that in a part 3, which would do proper transformation testing via the HandleSingleTap code, but you're right that it can be done here as well. I will add a follow-up patch to do this, so marking this bug as [leave open]. I'd rather land these two patches as-is to unblock QA on smoketesting B2G master.

https://hg.mozilla.org/integration/b2g-inbound/rev/a71ae794b742
https://hg.mozilla.org/integration/b2g-inbound/rev/34d8ee5c3ed1

Comment 18

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

Attachment: Part 3 - Update tests to test transformation matrices

This addresses your review comment above.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a71ae794b742
https://hg.mozilla.org/mozilla-central/rev/34d8ee5c3ed1

Comment 20

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

https://hg.mozilla.org/integration/b2g-inbound/rev/eef979f08ae2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eef979f08ae2