1063224 - APZ uses DPI in calculations involving coordinates that aren't in global screen space

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

APZCTreeManager::GetDPI() is used in three places in AsyncPanZoomController and Axis [1] [2] [3].

In each case, we multiply the DPI by a quantity in inches (or something related like inches per second) - a unit that only makes sense in the context of global screen space. 

However, in each case, we compare the resulting quantity to a quantity in an APZC's local screen space - a coordinate system that can potentially be (very) different from global screen space.

We should fix this, by converting the second quantity into global screen space (with the tree manager's help) before performing the comparison.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=9fe5fe620802#820
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=9fe5fe620802#1637
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?rev=b54f8eca9cb9#51

Comment 1

[Botond Ballo [:botond]]

Attachment: Patch 1 - Const-correctness improvements in APZ classes

Comment 2

[Botond Ballo [:botond]]

Attachment: Patch 2 - Add an Abs() overload for BaseCoord

Comment 3

[Botond Ballo [:botond]]

Attachment: Patch 3 - Make some more things strongly typed in APZ code

Comment 4

[Botond Ballo [:botond]]

Attachment: Patch 4 - Add a bit more infrastructure to Axis

Comment 5

[Botond Ballo [:botond]]

Attachment: Patch 5 - Give AsyncPanZoomController a mechanism for converting between local and global screen coordinates

Comment 6

[Botond Ballo [:botond]]

Attachment: Patch 6 - Add a PanStart() method to AsyncPanZoomController

Comment 7

[Botond Ballo [:botond]]

Attachment: Patch 7 - Use global screen coordinates when working with quantities in inches

Comment 8

[Botond Ballo [:botond]]

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

Comment 9

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

Comment on attachment 8485289 [details] [diff] [review]
Patch 3 - Make some more things strongly typed in APZ code

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1782,5 @@
>      angle = fabs(angle); // range [0, pi]
>  
>      float breakThreshold = gfxPrefs::APZAxisBreakoutThreshold() * APZCTreeManager::GetDPI();
>  
> +    if (fabs(aDelta.x) > breakThreshold || fabs(aDelta.y) > breakThreshold) {

The fact that you can use fabs on a ScreenCoord here without any trouble seems to imply that you don't need gfx::Abs at all. Coord<float> has an implicit constructor, so you should be able to do ScreenCoord foo = fabs(bar) where bar is also a ScreenCoord. Does that not work?

Comment 10

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

Comment on attachment 8485291 [details] [diff] [review]
Patch 5 - Give AsyncPanZoomController a mechanism for converting between local and global screen coordinates

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +979,5 @@
>  
> +Matrix4x4
> +APZCTreeManager::TransformFromGlobalScreenCoordinates(const AsyncPanZoomController* aApzc) const {
> +  Matrix4x4 transformToApzc;
> +  Matrix4x4 transformToGecko;  // ignored

We already have a bunch of places where we do this sort of thing (call GetInputTransforms with an ignored transformToGecko). It seems to me it would be better to just make the transformToGecko parameter optional in GetInputTransforms. Or maybe even split it into two separate functions, which might make it clearer.

Matrix4x4 GetScreenToApzcTransform(const AsyncPanZoomController*) const;
Matrix4x4 GetApzcToGeckoTransform(const AsyncPanZoomController*) const;

Comment 11

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

Leaving the reviews for now as the above changes might be fairly significant so I don't want to spend time reviewing details if the overall structure will change.

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Comment on attachment 8485289 [details] [diff] [review]
> Patch 3 - Make some more things strongly typed in APZ code
> 
> Review of attachment 8485289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1782,5 @@
> >      angle = fabs(angle); // range [0, pi]
> >  
> >      float breakThreshold = gfxPrefs::APZAxisBreakoutThreshold() * APZCTreeManager::GetDPI();
> >  
> > +    if (fabs(aDelta.x) > breakThreshold || fabs(aDelta.y) > breakThreshold) {
> 
> The fact that you can use fabs on a ScreenCoord here without any trouble
> seems to imply that you don't need gfx::Abs at all.

(aDelta.x is no longer a ScreenCoord, but your point below is still valid.)

> Coord<float> has an
> implicit constructor, so you should be able to do ScreenCoord foo =
> fabs(bar) where bar is also a ScreenCoord. Does that not work?

That does appear to work - thanks!

I remember having tried this before, and 

  fabs(mPos - mStartPos)

giving a compiler error, so I had to change it to

  fabs((mPos - mStartPos).value)

but I guess that was back when mPos and mStartPos were integers, and now they are floats thanks to bug 1046993.

I'll update the patches to get rid of Abs() and just use fabs().

Comment 13

[Botond Ballo [:botond]]

Attachment: Patch 2 - Make some more things strongly typed in APZ code

Comment 14

[Botond Ballo [:botond]]

Attachment: Patch 3 - Add a bit more infrastructure to Axis

Comment 15

[Botond Ballo [:botond]]

Attachment: Patch 4 - Split APZCTreeManager::GetInputTransforms() into GetScreenToApzcTransform() and GetApzcToGeckoTransform()

Comment 16

[Botond Ballo [:botond]]

Attachment: Patch 5 - Give AsyncPanZoomController a mechanism for converting between local and global screen coordinates

Comment 17

[Botond Ballo [:botond]]

Attachment: Patch 6 - Add a PanStart() method to AsyncPanZoomController

Comment 18

[Botond Ballo [:botond]]

Attachment: Patch 7 - Use global screen coordinates when working with quantities in inches

Comment 19

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

Comment on attachment 8486058 [details] [diff] [review]
Patch 4 - Split APZCTreeManager::GetInputTransforms() into GetScreenToApzcTransform() and GetApzcToGeckoTransform()

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

Much nicer, thanks!

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1309,5 @@
>     The APZCs also obviously have LT, LN, PT, and PN, so all of the above transformation combinations
>     required can be generated.
>   */
> +
> +/**

Use /*

The "long comment" won't show up in generated docs, but this comment will if you use /**, which means it will refer to something that is not present.

@@ +1329,5 @@
>    Matrix4x4 ancestorUntransform = aApzc->GetAncestorTransform();
>    ancestorUntransform.Invert();
>    // asyncUntransform is LA.Inverse()
>    Matrix4x4 asyncUntransform = aApzc->GetCurrentAsyncTransform();
>    asyncUntransform.Invert();

Remvoe asyncUntransform here and just declare it inside the loop, since it's not actually used at this point

@@ +1355,5 @@
> +
> +  return result;
> +}
> +
> +/**

/*

Comment 20

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

Comment on attachment 8486059 [details] [diff] [review]
Patch 5 - Give AsyncPanZoomController a mechanism for converting between local and global screen coordinates

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

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +328,5 @@
>  
> +  /**
> +   * Convert the vector |aVector|, rooted at the point |aAnchor|, from
> +   * this APZC's local screen coordinates into global screen coordinates.
> +   * The anchor is necessary because with 3D tranforms, the local of the vector

local? Not sure that's the word you meant to use. Ditto other comment below

Comment 21

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

Comment on attachment 8486061 [details] [diff] [review]
Patch 7 - Use global screen coordinates when working with quantities in inches

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

::: gfx/layers/apz/src/Axis.cpp
@@ +50,5 @@
>    float newVelocity = mAxisLocked ? 0.0f : (float)(mPos - aPos) / (float)(aTimestampMs - mPosTimeMs);
>    if (gfxPrefs::APZMaxVelocity() > 0.0f) {
> +    ScreenPoint maxVelocity = MakePoint(gfxPrefs::APZMaxVelocity() * APZCTreeManager::GetDPI());
> +    mAsyncPanZoomController->ToLocalScreenCoordinates(&maxVelocity, mAsyncPanZoomController->PanStart());
> +    newVelocity = std::min(newVelocity, maxVelocity.Length());

This makes me realize the max-velocity clamp is a per-axis max, which means diagonal pans might exceed the max-velocity. I don't really care though.

Comment 22

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

Sweet, nice work :)

Comment 23

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +328,5 @@
> >  
> > +  /**
> > +   * Convert the vector |aVector|, rooted at the point |aAnchor|, from
> > +   * this APZC's local screen coordinates into global screen coordinates.
> > +   * The anchor is necessary because with 3D tranforms, the local of the vector
> 
> local? Not sure that's the word you meant to use. Ditto other comment below

Thanks for catching that. I meant "location", but my mind must have been occupied too much by the words "local" and "global" :)

Comment 24

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3065e0ac5ab3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2549a67a8041
https://hg.mozilla.org/integration/mozilla-inbound/rev/3220317cf03a
https://hg.mozilla.org/integration/mozilla-inbound/rev/600d8514b1f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0016208a9220
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9879a291a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/879dcad4aded

Comment 25

[Botond Ballo [:botond]]

Addressed review comments and landed.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3065e0ac5ab3
https://hg.mozilla.org/mozilla-central/rev/2549a67a8041
https://hg.mozilla.org/mozilla-central/rev/3220317cf03a
https://hg.mozilla.org/mozilla-central/rev/600d8514b1f4
https://hg.mozilla.org/mozilla-central/rev/0016208a9220
https://hg.mozilla.org/mozilla-central/rev/8a9879a291a9
https://hg.mozilla.org/mozilla-central/rev/879dcad4aded