1055741 - Unify the ParentLayerPixel and ScreenPixel coordinate systems and remove FrameMetrics::mTransformScale

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Some of the patches on bug 1052063 reverse the order of transforms that layers go through. Prior to those patches, a layer would have the async transform and the CSS transform applied (in that order) before it got flattened into the parent layer. Specifically:

LayerPixel * LT * LN * LC = ParentLayerPixel

where LT is the transient async transform, LN is the nontransient async transform, and LC is the CSS transform. In this model, LayerPixel * LT was considered "ScreenPixels" for layer L, and LN * LC was L's "transform scale".

After those patches we do this:

LayerPixel * LC * LN * LT = ParentLayerPixel

In this model the old definitions of ScreenPixels and "transform scale" become pretty useless. My thinking so far is to get rid of the "transform scale", and refer to "LayerPixel * LC" as the "transformed layer pixels". Also, replace "ScreenPixel" with "ParentLayerPixel" to make it more obvious that this is a per-layer coordinate space. We can still keep "ScreenPixel" around for global screen pixels, and the only way to get there would be to use a PixelCastJustification from the root layer's ParentLayerPixel space (i.e. ScreenToParentLayerForRoot).

However, before we do all this, we should figure out what needs to change for bug 993525 and incorporate the necessary changes into this model.

Comment 1

[Botond Ballo [:botond]]

With the apz-css-trans-stage1 patches being complete, this bug is ripe for fixing.

As dicussed with Kats during the work week, the approach I'll take it to remove FrameMetrics::mTransformScale and change uses of ScreenPixel that represent "local Screen coordinates", which are now identical to ParentLayer coordinates, to ParentLayerPixel.

Comment 2

[Botond Ballo [:botond]]

Attachment: WIP

Here's a work-in-progress patch for the approach described in comment 1. It does not compile yet.

What is done:

  - Almost all uses of local ScreenPixels have been converted to
    ParentLayerPixel. See below for what remains to be done.

  - Where appropriate, I did some light refactoring to accompany these
    changes. For example, I changed

      void APZC::ToGlobalScreenCoordinates(ScreenPoint* aOutVector, 
                                           ScreenPoint aAnchor)

    to

      ScreenPoint APZC::ToScreenCoordinates(ParentLayerPoint aVector, 
                                            ParentLayerPoint aAnchor)

What remains to be done:

  - A few fields in event structures which start off in global Screen
    coordinates and are then transformed in-place to ParentLayer
    coordinates need to be dealt with. The current approach Kats and I
    are considering is to store the transformed values in new fields.

  - Code in APZCTreeManager that makes these transformations needs to
    be updated.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> What is done:

Oh, a few things I forgot:

  - FrameMetrics::mPresShellResolution was changed to be a plain 'float'.
    Its previous type, ParentLayerToLayer scale, did not make sense (it
    used an old interpretation of 'ParentLayer' space which is inconsistent
    with our current interpretation, and would have caused confusion as
    new uses of the current interpretation are introduced), and we don't
    have names for the coordinate spaces A and B such that making its type
    'AToBScale' would be correct (nor do we work with quantities in those
    coordinate spaces).

  - The type of ViewTransform::mScale was changed from ParentLayerToScreenScale
    to LayerToParentLayerScale. The change in the target type (Screen to
    ParentLayer) is just a routine part of the changes in this bug. The
    change in the source type (ParentLayer to Layer) is to avoid confusion,
    as the ParentLayer in the source type also referred to the old
    interpretation of ParentLayer space. Once stage 2 of apz-css-transforms
    (bug 1076192) lands, the new type will reflect reality.

Comment 4

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> Created attachment 8508080 [details] [diff] [review]
> Unify the 'local Screen' and 'ParentLayer' coordinate systems

This patch completes the work by adding new fields to input event structures to store points in transformed coordinates, and updating APZCTreeManager code accordingly. I also fixed a bunch of other compiler errors in the original patch.

As part of the APZCTreeManager changes, I:

  - Removed the copying of input event objects before passing them to 
    APZC. The point of the copying was that we used to stomp on the
    event point by overwriting it with the transformed version, but
    that's now moot.

  - Removed the ApplyTransform() functions, replacing their uses with
    TransformTo<>() (for consistency, even in the remaining cases
    where the source and taget objects are the same).

Comment 6

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c1d56401548f

Comment 7

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

The Try run shows Android bustages and gtest failures.

This updated patch fixes the Android bustages. I still have to fix the gtest failures. Will drop review flag until they're sorted out.

Comment 8

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

The cause of the gtest failures was that they were passing inputs directly to APZC without setting the transformed point fields (e.g. SingleTouchData::mLocalScrenPoint).

Fixed. Putting patch back up for review.

Comment 9

[Botond Ballo [:botond]]

New Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f54afca15028

Comment 10

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

Comment on attachment 8508298 [details] [diff] [review]
Unify the 'local Screen' and 'ParentLayer' coordinate systems

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

r=me with comments addressed

::: gfx/layers/FrameMetrics.h
@@ +40,1 @@
>  typedef gfx::MarginTyped<ParentLayerPixel> ParentLayerMargin;

We should probably move all this ParentLayer stuff into Units.h since it's a more "public" pixel type now.

@@ -352,5 @@
>    // resolution. This information is provided by Gecko at layout/paint time.
>    LayoutDeviceToLayerScale mCumulativeResolution;
>  
> -  // TODO(botond): This is now always 1 and should be removed (see bug 1055741).
> -  ScreenToParentLayerScale mTransformScale;

\o/ removing things from FrameMetrics!

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +509,5 @@
>          apzc->GetGuid(aOutTargetGuid);
> +        Matrix4x4 transformToGecko = transformToApzc * GetApzcToGeckoTransform(apzc);
> +        panInput.mPanStartPoint = TransformTo<ScreenPixel>(
> +            transformToGecko, panInput.mPanStartPoint);
> +        panInput.mPanDisplacement = TransformVector<ScreenPixel>(

Ah. For the record this isn't what I originally had in mind, but what I had in mind doesn't actually work so this makes sense. What I was thinking was that you would have two fields in the InputData object, one in screen and one in parentlayer coords. This code would just populate the parentlayer one from the screen one, and leave the screen one untouched. What I forgot was that we actually have *three* coordinate systems in play here - there's the gecko one as well; for some reason I thought we could just use the parentlayer versions in both APZ and as the "untransformed" coordinates.

But never mind all that and this seems good :)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1751,3 @@
>  }
>  
>  float AsyncPanZoomController::PanDistance() const {

Can we make the return type here a ScreenCoord for clarity? Same for the return value of GetTouchStartTolerance(). If you have to throw in some static_casts until we can make Length() return a typed-coord, so be it. I think it would be nicer overall.

@@ +2598,5 @@
>        currentScrollOffset.y = clamped(currentScrollOffset.y, minScrollOffset.y, maxScrollOffset.y);
>      }
>    }
>  
> +  LayerToParentLayerScale scale(  mFrameMetrics.mPresShellResolution    // non-transient portion

nit: no spaces after "("

::: gfx/layers/apz/src/Axis.h
@@ +10,5 @@
>  #include <sys/types.h>                  // for int32_t
>  #include "Units.h"
>  #include "mozilla/TimeStamp.h"          // for TimeDuration
>  #include "nsTArray.h"                   // for nsTArray
> +#include "FrameMetrics.h"               // for ParentLayerPixel

Shouldn't need this if we move it to Units.h

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +38,3 @@
>  {
> +  const ParentLayerPoint& firstTouch = aEvent.mTouches[0].mLocalScreenPoint,
> +                           secondTouch = aEvent.mTouches[1].mLocalScreenPoint;

While you're touching this, split these into two separate declaration statements.

@@ +43,5 @@
>  
>  float GetCurrentSpan(const MultiTouchInput& aEvent)
>  {
> +  const ParentLayerPoint& firstTouch = aEvent.mTouches[0].mLocalScreenPoint,
> +                          secondTouch = aEvent.mTouches[1].mLocalScreenPoint;

Ditto

::: gfx/layers/client/TiledContentClient.cpp
@@ +149,5 @@
>    // This is basically the same code as AsyncPanZoomController::GetCurrentAsyncTransform
>    // but with aContentMetrics used in place of mLastContentPaintMetrics, because they
>    // should be equivalent, modulo race conditions while transactions are inflight.
>  
> +  LayerToParentLayerScale scale(  aCompositorMetrics.mPresShellResolution

nit: remove spaces after "("

::: layout/base/nsDisplayList.cpp
@@ +720,5 @@
>  
>    // Only the root scrollable frame for a given presShell should pick up
>    // the presShell's resolution. All the other frames are 1.0.
>    if (aScrollFrame == presShell->GetRootScrollFrame()) {
> +    metrics.mPresShellResolution = presShell->GetXResolution();

Maybe add a NS_WARNING here if the x and y resolutions are different? If people actually checked for warnings it might be useful, but I'll leave it to you to decide.

@@ +742,5 @@
>      (float)nsPresContext::AppUnitsPerCSSPixel() / auPerDevPixel);
>  
>    // Initially, AsyncPanZoomController should render the content to the screen
>    // at the painted resolution.
> +  const LayerToParentLayerScale layerToParentLayerScale(1.0f);

Might be easier to just inline this in the use sites. Up to you.

::: layout/base/nsLayoutUtils.cpp
@@ +2763,4 @@
>    if (aScrollFrame == presShell->GetRootScrollFrame()) {
>      // Only the root scrollable frame for a given presShell should pick up
>      // the presShell's resolution. All the other frames are 1.0.
> +    resolution = presShell->GetXResolution();

Ditto on the warning

@@ +2774,5 @@
>    LayoutDeviceToLayerScale cumulativeResolution(
>        presShell->GetCumulativeResolution().width
>      * nsLayoutUtils::GetTransformToAncestorScale(aScrollFrame).width);
>  
> +  LayerToParentLayerScale layerToParentLayerScale(1.0f);

And ditto on the inlining. Although here I guess you explicitly un-inlined so maybe you prefer it this way.

::: widget/InputData.h
@@ +11,5 @@
>  #include "nsTArray.h"
>  #include "Units.h"
>  #include "mozilla/EventForwards.h"
>  #include "mozilla/TimeStamp.h"
> +#include "FrameMetrics.h"

This include should go away if we move the ParentLayerPixel stuff to Units.h

@@ +127,5 @@
> +  // Construct a SingleTouchData from a ParentLayer point.
> +  // mScreenPoint remains (0,0) unless it's set later.
> +  SingleTouchData(int32_t aIdentifier,
> +                  ParentLayerPoint aLocalScreenPoint,
> +                  ScreenSize aRadius,

Doesn't matter right now, but if we ever start using the radius for anything then we should make that a ParentLayerSize as well and do all the same untransforms on it that we do on the point.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1751,3 @@
> >  }
> >  
> >  float AsyncPanZoomController::PanDistance() const {
> 
> Can we make the return type here a ScreenCoord for clarity? Same for the
> return value of GetTouchStartTolerance(). If you have to throw in some
> static_casts until we can make Length() return a typed-coord, so be it. I
> think it would be nicer overall.

In my mind, a Coord is a location along an axis, and it's a conceptual error to represent a distance in 2D as a Coord.

I can imagine introducing a separate typed class (Distance2D?) for representing these quantities.

(For the record, I also think it's a conceptual error to represent a distance in 1D as a Coord, for the same reason that I think it's a conceptual error to represent a vector in 2D as a Point, but we already commit this error everywhere and I've sort of given up on it for now.)

> ::: layout/base/nsDisplayList.cpp
> @@ +720,5 @@
> >  
> >    // Only the root scrollable frame for a given presShell should pick up
> >    // the presShell's resolution. All the other frames are 1.0.
> >    if (aScrollFrame == presShell->GetRootScrollFrame()) {
> > +    metrics.mPresShellResolution = presShell->GetXResolution();
> 
> Maybe add a NS_WARNING here if the x and y resolutions are different? If
> people actually checked for warnings it might be useful, but I'll leave it
> to you to decide.

Stage 3 of apz-css-transforms will make the need for a warning go away, as we'll then actually support different x- and y-resolutions.

> @@ +742,5 @@
> >      (float)nsPresContext::AppUnitsPerCSSPixel() / auPerDevPixel);
> >  
> >    // Initially, AsyncPanZoomController should render the content to the screen
> >    // at the painted resolution.
> > +  const LayerToParentLayerScale layerToParentLayerScale(1.0f);
> 
> Might be easier to just inline this in the use sites. Up to you.

The point of not inlining here is so that the comment explaining why we use 1.0f as the scale can be in one place.

> @@ +127,5 @@
> > +  // Construct a SingleTouchData from a ParentLayer point.
> > +  // mScreenPoint remains (0,0) unless it's set later.
> > +  SingleTouchData(int32_t aIdentifier,
> > +                  ParentLayerPoint aLocalScreenPoint,
> > +                  ScreenSize aRadius,
> 
> Doesn't matter right now, but if we ever start using the radius for anything
> then we should make that a ParentLayerSize as well and do all the same
> untransforms on it that we do on the point.

Good point. I'll add a comment to make sure we don't forget.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> > @@ +1751,3 @@
> > >  }
> > >  
> > >  float AsyncPanZoomController::PanDistance() const {
> > 
> > Can we make the return type here a ScreenCoord for clarity? Same for the
> > return value of GetTouchStartTolerance(). If you have to throw in some
> > static_casts until we can make Length() return a typed-coord, so be it. I
> > think it would be nicer overall.
> 
> In my mind, a Coord is a location along an axis, and it's a conceptual error
> to represent a distance in 2D as a Coord.
> 
> I can imagine introducing a separate typed class (Distance2D?) for
> representing these quantities.
> 
> (For the record, I also think it's a conceptual error to represent a
> distance in 1D as a Coord, for the same reason that I think it's a
> conceptual error to represent a vector in 2D as a Point, but we already
> commit this error everywhere and I've sort of given up on it for now.)

Thinking some more about this, I think it would still be a net win to make these things ScreenCoords - at least we are expressing what coordinate system they are in - so I'll do that.

Comment 13

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

Updated patch to address review comments. Carrying r+.

I didn't add warnings about the different x/y resolutions due to the impending apz-css-transforms, stage 3 work.

Comment 14

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

Rebased to apply on top of bug 1083395. Carrying r+.

Comment 15

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

Rebased to apply to latest trunk. Carrying r+.

Comment 16

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

Rebased to apply to latest trunk.

Requesting re-review on the CreateSingleTouchData and CreatePinchGestureInput functions in TestAsyncPanZoomController only.

Comment 17

[Botond Ballo [:botond]]

Attachment: Unify the 'local Screen' and 'ParentLayer' coordinate systems

Whoops, had a couple of typos in there, fixed them.

Comment 18

[Botond Ballo [:botond]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a96930f1e26b

(Clean Try push here: https://tbpl.mozilla.org/?tree=Try&rev=64cb0111f38c)

Comment 19

[Carsten Book [:Tomcat]]

Sorry had to back this change out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=8cb112e23067 since this changes from this bug or bug 1055741 caused Android 4.0 M2 Crashes like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3849267&repo=mozilla-inbound

Comment 20

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

Sorted out the crashes over in bug 1076163.

Re-landing: https://hg.mozilla.org/integration/mozilla-inbound/rev/8273b2521c2e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8273b2521c2e

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like this patch completely broke a Firefox for Android Talos test:

http://graphs.mozilla.org/graph.html#tests=%5B%5B201,63,29%5D%5D&sel=1415820386244.2632,1415871165191.6316,0,148.23529411764707&displayrange=7&datatype=running

I spotted it in a merge to FX-Team, but traced it back to an Inbound push

Regression: Fx-Team - Robocop Checkerboarding Real User Benchmark - Android 4.0.4 - 1.21e+03% increase
------------------------------------------------------------------------------------------------------
    Previous: avg 7.624 stddev 0.957 of 12 runs up to revision 8bbf5db85ada
    New     : avg 100.000 stddev 0.000 of 12 runs since revision 7f0d92595432
    Change  : +92.376 (1.21e+03% / z=96.548)

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

I have retriggered as this seemed odd, it really is this patch:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android%204.0%20Panda%20mozilla-inbound%20talos%20remote-trobocheck2&fromchange=f8ff4c7a978a&tochange=c49c2442f666

you can see when it originally landed it caused the regression, then when it was backed out the regression went away, likewise when it landed again our values went up.

:botond, do you want to reopen this bug and track it, or would you like me to create a new bug?

Comment 24

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

The talos regressions are likely because of bug 1076163 which landed just before this, and regressed Fennec. bug 1099104 fixed both the behavior bustage and the Talos regression.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

kats, thanks, that had a failure to build, so it was hard to point fingers appropriately, although :botond was the same author so I thought a one shot fix would do it all :)  Thanks for helping out with the book keeping!