Closed Bug 1063224 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: botond, Assigned: botond)

Details

Attachments

(7 files, 6 obsolete files)

(deleted), patch
kats
: review+
Details | Diff | Splinter Review
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
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
Attachment #8485287 - Flags: review?(bugmail.mozilla)
Attached patch Patch 2 - Add an Abs() overload for BaseCoord (obsolete) (deleted) — Splinter Review
Attachment #8485288 - Flags: review?(bugmail.mozilla)
Attachment #8485289 - Flags: review?(bugmail.mozilla)
Attached patch Patch 4 - Add a bit more infrastructure to Axis (obsolete) (deleted) — Splinter Review
Attachment #8485290 - Flags: review?(bugmail.mozilla)
Attachment #8485292 - Flags: review?(bugmail.mozilla)
Attachment #8485293 - Flags: review?(bugmail.mozilla)
Attachment #8485287 - Flags: review?(bugmail.mozilla) → review+
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 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;
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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().
Attachment #8486048 - Flags: review?(bugmail.mozilla)
Attachment #8485288 - Attachment is obsolete: true
Attachment #8485288 - Flags: review?(bugmail.mozilla)
Attachment #8486057 - Flags: review?(bugmail.mozilla)
Attachment #8485290 - Attachment is obsolete: true
Attachment #8485290 - Flags: review?(bugmail.mozilla)
Attachment #8485289 - Attachment is obsolete: true
Attachment #8485289 - Flags: review?(bugmail.mozilla)
Attachment #8486057 - Attachment description: Patch 4 - Add a bit more infrastructure to Axis → Patch 3 - Add a bit more infrastructure to Axis
Attachment #8486060 - Flags: review?(bugmail.mozilla)
Attachment #8485291 - Attachment is obsolete: true
Attachment #8485291 - Flags: review?(bugmail.mozilla)
Attachment #8485292 - Attachment is obsolete: true
Attachment #8485292 - Flags: review?(bugmail.mozilla)
Attachment #8485293 - Attachment is obsolete: true
Attachment #8485293 - Flags: review?(bugmail.mozilla)
Attachment #8486048 - Flags: review?(bugmail.mozilla) → review+
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; > +} > + > +/** /*
Attachment #8486058 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8486059 - Flags: review?(bugmail.mozilla) → review+
Attachment #8486060 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8486061 - Flags: review?(bugmail.mozilla) → review+
Attachment #8486057 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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" :)
Addressed review comments and landed.
Blocks: 1062483
No longer blocks: 1062483
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: