Closed Bug 1120683 Opened 10 years ago Closed 9 years ago

Audit applications of matrices to points in APZ code

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: botond, Assigned: kevin.m.wern, Mentored)

References

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file, 6 obsolete files)

w-coord-check-final.patch
17.50 KB, patch
botond
: review+
Details | Diff | Splinter Review
In APZ code, we apply a Matrix4x4 to a 2D point in several places. In some of these places [1], we call Matrix4x4::ProjectPoint(), which yields a 4D point, which we then check for HasPositiveWCoord(), and if so, convert to a 2D point via As2DPoint() (otherwise we fail whatever operation was trying to perform the transformation). In other places ([2], [3], and [4] among several others), we use the TransformTo<>() helper function, which calls Matrix4x4::operator*(Point), which truncates the result to a 2D point without peforming any check on the w-coordinate. I'm wondering whether some of these latter call sites should also be doing the checking for the w-coordinate. Matt, could you help me understand the conditions under which this checking for the w-coordinate is important? Should we be doing it every time we apply a 4x4 matrix to a 2D point? [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/HitTestingTreeNode.cpp?rev=27d6d4a76992#179 [2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=27d6d4a76992#590 [3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=27d6d4a76992#612 [4] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=27d6d4a76992#796
Flags: needinfo?(matt.woodrow)
Whiteboard: gfx-noted
We want to check the w coordinate when reverse transforming a point. A forward transformation takes a 2d point and outputs a 4d point, and we then truncate it to 2d since that's all we care about. When trying to convert one of those final 2d points back into the original coordinate space, we need to recover the other 2 dimensions, which is what ProjectPoint does. We need to check the w coordinate in this case to make sure the point was actually on the visible side of the w plane.
Flags: needinfo?(matt.woodrow)
Thanks, Matt, that helps! In APZ, we deal with two types of transforms: (A) CSS transforms (i.e. layer->GetTransform()), which, IIUC, can be arbitrary 3D projective transforms. (B) Async transforms, which are just 2D translations and scales. I'd like to run by you two types of calculations performed by APZ: (1) Un-apply a bunch of transforms of types (A) and (B), interspersed. Here, is it reasonable to multiply all the transforms together, take their inverse, then do a single ProjectPoint call and w-coordinate check with the resulting matrix? (2) Un-apply a bunch of transforms of types (A) and (B), interspersed. Re-apply all the transforms of type (A). In this case, it's not necessary to use ProjectPoint because all the 3D transforms I'm un-applying, I'm also re-applying, and the interspersed (B) transforms, which I'm not re-applying, are 2D?
Flags: needinfo?(matt.woodrow)
(In reply to Botond Ballo [:botond] from comment #2) > > (1) Un-apply a bunch of transforms of types (A) and (B), interspersed. > > Here, is it reasonable to multiply all the transforms together, > take their inverse, then do a single ProjectPoint call and > w-coordinate check with the resulting matrix? Yes. > > (2) Un-apply a bunch of transforms of types (A) and (B), interspersed. > Re-apply all the transforms of type (A). > > In this case, it's not necessary to use ProjectPoint because all > the 3D transforms I'm un-applying, I'm also re-applying, and the > interspersed (B) transforms, which I'm not re-applying, are 2D? I think so, as long as you're concatenating them all into a single matrix. You should assert that the result is 2D to be sure.
Flags: needinfo?(matt.woodrow)
Thanks, Matt! It's clear to me what changes have to be made now. I'd be happy to mentor someone in making them.
Mentor: botond
Whiteboard: gfx-noted → [gfx-noted] [lang=c++]
Hey, I'm interested in fixing this bug, although it seems like it's been a while. Is this still set to be worked on? -Kevin Wern
Flags: needinfo?(botond)
Yes! You're very welcome to work on it. On a high level, this is what needs to be done: - Go through the places in APZ code where we apply a Matrix4x4 to a 2D point using Matrix4x4::operator*(Point). In APZ code, I would expect this happens via the TransformTo<>() function in all cases. (I mention some call sites in comment 0.) - Identify whether the transformation falls into category (1) or (2) in comment 2, or something else. When determining this: - Transforms that come from Layer::GetTransform() are of type (A) in that comment, i.e. they can be arbitrary projective 3D transforms. - Transforms that come from AsyncPanZoomController::GetCurrentAsyncTransform() are of type (B), i.e. they are just 2D translations and scales. - For the transformations in category (1), change the transformation to be done using ProjectPoint, and add a w-coordinate check as described in comment 2. - If the w-coordinate check fails, the operation that needs to perform the transformation should fail. For example, a hit test should not succeed, or an event should not be dispatched. Let me know if that's enough to get you started. I'm happy to go into more detail if you'd like.
Flags: needinfo?(botond)
Botond, I'm a little slow here so just bare with me. Perhaps an example would be suffice, so let's choose line number 796 ([4] in comment Description) of APZ code. Which category would the transformation in line 766 fall under? For lines 590 and 612 I would think it falls under category 2...do you concur?
Attached patch apz-transformto-refactor.patch (obsolete) — — Splinter Review
Draft of apz TransformTo refactor in apz directory only.
Flags: needinfo?(botond)
Attached patch apz-transformto-refactor.patch (obsolete) — — Splinter Review
Just fixed the subject line
Attachment #8608021 - Attachment is obsolete: true
Attached patch apz-transformto-refactor.patch (obsolete) — — Splinter Review
Didn't fix the subject line
Attachment #8608023 - Attachment is obsolete: true
Sorry it's been awhile, I've been busy and it took me some time to reason out the code. I found a handful of changeable instances in gfx/layouts/apz/src. All the mochitests are passing, but if there's anything else I should be checking, let me know. My reasoning for which transforms were in which categories involved the GetScreenToApzcTransform() and GetApzcToGeckoTransform() functions. The first unapplies all of the transformations types, and the second reapplies CSS transformations (as far as I can tell), so transformations involving both fall in category (2) and transformations only involving GetScreenToApzcTransform fall in category (1). I refactored the transforms in category (1). Also, for now, I used MOZ_ASSERT to assert that the W coordinate is positive. I am not sure if there is some other convention I should be using... Let me know if I'm missing something, as I'm not so confident that this is completely correct.
Comment on attachment 8608386 [details] [diff] [review] Sorry, for some reason the subject was still not changed (this is a pretty terrible series of edits...) Review of attachment 8608386 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Kevin, this is a good start! I have a few comments: - Your patch changes: TransformTo<...>(matrix, point) to: matrix.Inverse().ProjectPoint(point.ToUnknownPoint()) Inverting there is incorrect. TransformTo<>() just multiplies the matrix by the point, without doing any inverting, as does ProjectPoint(). It should be just: matrix.ProjectPoint(point.ToUnknownPoint()) - Negative w-coordinates can arise legitimately if web content uses certain types of 3D transforms, so asserting is not the appropriate way to handle them - we need to handle them gracefully. - We should change TransformDisplacement() to return bool, and return false if we get a negative w-coordinate. TransformDisplacement() in turn has two callers, APZCTreeManager::DispatchScroll() and DispatchFling(). Both return bool, and both can return false if TransformDisplacement() does. - AsyncPanZoomController::Contains() can return false if we get a negative w-coordinate. - TransformDisplacement() is a bit tricky to analyze in terms of the category of transformations it performs. Keeping in mind that GetScreenToApzcTransform() *unapplies* transforms: - TransformDisplacement() first multiplies by the *inverse* of GetScreenToApzcTransform() for one APZC. Therefore, this is *applying* transforms. - It then multiplies by GetScreenToApzcTransform() for another APZC. This is *unapplying* transforms. Since only *unapplying* needs the w-coordinate check, only the second operation in TransformDisplacement() needs the check. - There are a few other calls to TransformTo<>() that need to be changed: - The ones in the TransformToLocal() functions in widget/InputData.cpp. These functions take a matrix that originates from a GetScreenToApzcTransform(), so they should be performing the w-coordinate check. They can be changed to return bool, and their caller, AsyncPanZoomController::HandleInputEvent(), can return nsEventStatus_eIgnore without doing further processing, if they return false. - AsyncPanZoomController::ToParentLayerCoordinates() calls TransformTo<>() (indirectly, via TransformVector<>()) with a GetScreenToApzcTransform(). Currently, its only caller is Axis::ToLocalVelocity(); it's not clear to me what that should do if we get a negative w-coordinate. It's probably fine to leave this one as-is for now, but it may be a good idea to add a comment to ToParentLayerCoordinates() saying that technically it should be doing a w-coordinate check. Note: It's OK to deal with these other calls in a follow-up patch if you'd like - no need to do everything at once. - Finally, for cases where we're doing a category (2) transformation (i.e. unapplying both transform types, and reapplying the CSS transforms), such as the TransformTo<>() calls in APZCTreeManager::ReceiveInputEvent(), Matt suggests asserting that the combined matrix is 2D. This is also OK to do in a follow-up. ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +590,4 @@ > Matrix4x4 transformToGecko = GetScreenToApzcTransform(apzc) > * GetApzcToGeckoTransform(apzc); > wheelInput.mOrigin = > + TransformTo<ScreenPixel>(transformToGecko, wheelInput.mOrigin); This line contains a whitespace change only - let's not include it in the patch.
(In reply to Ryan Nath from comment #7) > I'm a little slow here so just bare with me. Perhaps an example would be > suffice, so let's choose line number 796 ([4] in comment Description) of APZ > code. Which category would the transformation in line 766 fall under? That would be category (2), because the matrix we're transforming by was obtained by multiplying a GetScreenToApzcTransform() (which un-applies a combination of CSS and async transforms) and a GetApzcToGeckoTransform() (which re-applies the CSS transforms). > For > lines 590 and 612 I would think it falls under category 2...do you concur? Yep!
Flags: needinfo?(botond)
Ryan, I'm going to assign the bug to Kevin because he expressed interest sooner and has already gotten started. Hope that's cool! If you'd like, I'm happy to suggest a different bug for you to work on.
Assignee: nobody → kevin.m.wern
No problem, Botond.
I also realized that, while the plan in comment 6 handles transformations of *points*, there are also places where we un-transform *rectangles* by a matrix that can potentially be a 3D projective transform. Matt, what is the appropriate way to handle rectangles? Currently we use Matrix4x4::TransformBounds() - is that fine as-is?
Flags: needinfo?(matt.woodrow)
Matrix4x4::ProjectRectBounds should do what you want.
Flags: needinfo?(matt.woodrow)
All the changes you mentioned, including a comment to add a w-coordinate check for ToParentLayerCoordinates function. I also noticed a ToScreenCoordinates function that was structured similarly. It seems like it could be handled ok, but I'm not exactly sure what parts of these objects are called where so I wanted to get your input. I also would like to add another patch for the corresponding rectangle checks, but I wanted to make sure that the changes I've made were what you had in mind. I should be able to get changes turned around faster, now, too.
Attachment #8608386 - Attachment is obsolete: true
Flags: needinfo?(botond)
Comment on attachment 8616453 [details] [diff] [review] transform-w-check-and-assert-2D.patch Review of attachment 8616453 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in getting to this! This is looking much better. In addition to the specific comments below, I think there is a general improvement we can make: instead of repeating the w-coordinate check in each place that it's needed, we can encapsulate it into a function similar to TransformTo(), which returns a Maybe<Point> (so we have something to return if the w-coordinate check fails). We could call this function 'UntransformTo', and it could look something like this: template <typename TargetUnits, typename SourceUnits> static Maybe<gfx::PointTyped<TargetUnits>> UntransformTo( const gfx::Matrix4x4& aTransform, gfx::PointTyped<SourceUnits>& aPoint) { Point4D result = aTransform.ProjectPoint(aPoint.ToUnknownPoint()); if (!result.HasPositiveWCoord()) { return Nothing(); } return Some(ViewAs<TargetUnits>(result.As2DPoint())); } (If you're not familiar with Maybe<T> and the related functions Nothing() and Some(), have a look at the comment at the top of mfbt/Maybe.h. It's a very useful utility!) This would accomplish two things: 1) encapsulate the w-coordinate check in one place 2) force call sites to handle the case where the check failed (Note that this matches what we did in bug 1109873 for HitTestingTreeNode::Untransform(). In fact, the implementation of that function can be revised to use UntransformTo() once that's added.) Here's what a call site would look like, using TransformDisplacement() as an example: Maybe<ParentLayerPoint> startPoint = UntransformTo<ParentLayerPixel>(transformToApzc, screenStart); if (!startPoint) { return false; } aStartPoint = *startPoint; We will of course need an overload of UntransformTo<>() for IntPointTyped, too. ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +1126,4 @@ > * @param aTarget the target APZC > * @param aStartPoint the start point of the displacement > * @param aEndPoint the end point of the displacement > + * @return true on success, false if aStartPoint or aEndPoint has w <= 0 This comment is a bit inaccurate, as it's not aStartPoint and aEndPoint that we perform the w-coordinate check on (in fact they're 2D points), but the things we transform them to. I would suggest saying "false if aStartPoint and aEndPoint cannot be transformed into the target's coordinate space". @@ +1136,4 @@ > ParentLayerPoint& aEndPoint) { > // Convert start and end points to Screen coordinates. > Matrix4x4 untransformToApzc = aTreeManager->GetScreenToApzcTransform(aSource).Inverse(); > + MOZ_ASSERT(untransformToApzc.Is2D()); This matrix is allowed to be 3D, we just don't have to do the w-coordinate check because we're *applying* the 3D transform rather than *unapplying* it. (I know this is confusing. Transforms are applied as we go from the APZC's coordinate space to the screen coordinate space, so GetScreenToApzcTransform() is *unapplying* a potentially-3D transform, and GetScreenToApzcTransform().Inverse() is *applying* it.) @@ +1144,4 @@ > // Convert start and end points to aTarget's ParentLayer coordinates. > Matrix4x4 transformToApzc = aTreeManager->GetScreenToApzcTransform(aTarget); > + Point4D point = transformToApzc.ProjectPoint(screenStart.ToUnknownPoint()); > + if(!point.HasPositiveWCoord()) return false; Style nit about if-statements, here and elsewhere: please add a space before the opening parenthesis, put the statement inside the if on its own line, and enclose it in braces. Example: if (!point.HasPositiveWCoord()) { return false; } ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +1840,4 @@ > return TransformVector<ScreenPixel>(GetTransformToThis().Inverse(), aVector, aAnchor); > } > > +// Todo: figure out a good way to check the w-coordinate is positive and return the result Nit: capitalize 'TODO' (for consistency) ::: widget/InputData.cpp @@ +229,5 @@ > + Point4D point = aTransform.ProjectPoint(mPanStartPoint.ToUnknownPoint()); > + if (!point.HasPositiveWCoord()) return false; > + mLocalPanStartPoint = ViewAs<ParentLayerPixel>(point.As2DPoint()); > + > + point = aTransform.ProjectPoint(mPanDisplacement.ToUnknownPoint()) - aTransform.ProjectPoint(mPanDisplacement.ToUnknownPoint() + mPanStartPoint.ToUnknownPoint()); I don't think this calculation is quite right. Looking at what TransformVector() does, I think it needs to be: aTransform.ProjectPoint(mPanDisplacement.ToUnknownPoint() + mPanStartPoint.ToUnknownPoint()) - aTransform.ProjectPoint(mPanStartPoint.ToUnknownPoint()) I would recommend writing an UntransformVector() function to encapsulate this.
(In reply to kevin.m.wern from comment #20) > I also noticed a > ToScreenCoordinates function that was structured similarly. It seems like it > could be handled ok, but I'm not exactly sure what parts of these objects > are called where so I wanted to get your input. ToScreenCoordinates() is similar to the first transformation in TransformDisplacement(), it's *applying* a potentially-3D transform, rather than *unapplying* it, so it doesn't need a w-coordinate check.
See Also: → 1173521
(In reply to kevin.m.wern from comment #20) > I also would like to add another patch for the corresponding rectangle > checks, but I wanted to make sure that the changes I've made were what you > had in mind. I filed a new bug for rectangles: bug 1173521. You're welcome to work on it once you're done with this one, if you're interested!
Flags: needinfo?(botond)
Let me know what you think.
Attachment #8616453 - Attachment is obsolete: true
Flags: needinfo?(botond)
Comment on attachment 8626948 [details] [diff] [review] w-coordinate-check-matrix-transform-apz-revised.patch Review of attachment 8626948 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Kevin, this patch looks great! I only have a few very minor remaining comments: ::: layout/base/UnitTransforms.h @@ +128,4 @@ > return RoundedToInt(ViewAs<TargetUnits>(aTransform.TransformBounds(rect))); > } > > +template <typename TargetUnits, typename SourceUnits> Please add a comment describing what the UntransformTo() and UntransformVector() functions do. Suggestion: "The UntransformTo() and UntransformVector() functions are similar to TransformTo() and TransformVector(), except that they are meant to be used when the transform matrix is the inverse of a projective 3D transform. When using such transforms, the result is only meaningful if the Point4D that results from applying the transform has a positive w-coordinate. To handle this, these functions return a Maybe object which contains a value if and only if the result would be meaningful." @@ +128,5 @@ > return RoundedToInt(ViewAs<TargetUnits>(aTransform.TransformBounds(rect))); > } > > +template <typename TargetUnits, typename SourceUnits> > +static Maybe<gfx::PointTyped<TargetUnits> > UntransformTo(const gfx::Matrix4x4& aTransform, There's no need to put a space between the closing angle brackets any more. (C++11 removed that requirement, and we support that feature as described in https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code). @@ +132,5 @@ > +static Maybe<gfx::PointTyped<TargetUnits> > UntransformTo(const gfx::Matrix4x4& aTransform, > + const gfx::PointTyped<SourceUnits>& aPoint) > +{ > + gfx::Point4D point = aTransform.ProjectPoint(aPoint.ToUnknownPoint()); > + if (!point.HasPositiveWCoord()) return Nothing(); Please fix formatting of if statement. @@ +137,5 @@ > + return Some(ViewAs<TargetUnits>(point.As2DPoint())); > +} > + > +template <typename TargetUnits, typename SourceUnits> > +static Maybe<gfx::IntPointTyped<TargetUnits> > UntransformTo(const gfx::Matrix4x4& aTransform, Same here (no need for space). @@ +141,5 @@ > +static Maybe<gfx::IntPointTyped<TargetUnits> > UntransformTo(const gfx::Matrix4x4& aTransform, > + const gfx::IntPointTyped<SourceUnits>& aPoint) > +{ > + gfx::Point4D point = aTransform.ProjectPoint(aPoint.ToUnknownPoint()); > + if (!point.HasPositiveWCoord()) return Nothing(); Same here (formatting of if statement). @@ +165,5 @@ > + const gfx::PointTyped<SourceUnits>& aVector, > + const gfx::PointTyped<SourceUnits>& aAnchor) { > + gfx::Point4D point = aTransform.ProjectPoint(aAnchor.ToUnknownPoint() + aVector.ToUnknownPoint()) - aTransform.ProjectPoint(aAnchor.ToUnknownPoint()); > + if (!point.HasPositiveWCoord()) > + { Please put brace on previous line. ::: widget/InputData.cpp @@ +229,3 @@ > PanGestureInput::TransformToLocal(const gfx::Matrix4x4& aTransform) > +{ > + Maybe<ParentLayerPoint> point = UntransformTo<ParentLayerPixel>(aTransform, mPanStartPoint); Let's call this variable panStartPoint @@ +233,5 @@ > + return false; > + } > + mLocalPanStartPoint = *point; > + > + Maybe<ParentLayerPoint> displacementPoint = UntransformVector<ParentLayerPixel>(aTransform, mPanDisplacement, mPanStartPoint); And this one, panDisplacement
Attachment #8626948 - Flags: feedback+
Flags: needinfo?(botond)
Since the only remaining changes I asked for are comments/renaming/formatting, I went ahead and pushed the current patch to the Try server so we can be sure it passes tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9464db4af05
(In reply to Botond Ballo [:botond] from comment #26) > Since the only remaining changes I asked for are > comments/renaming/formatting, I went ahead and pushed the current patch to > the Try server so we can be sure it passes tests: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9464db4af05 My bad, I rebased the patch incorrectly. Let me try that again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=014760f912c8
(In reply to Botond Ballo [:botond] from comment #27) > (In reply to Botond Ballo [:botond] from comment #26) > > Since the only remaining changes I asked for are > > comments/renaming/formatting, I went ahead and pushed the current patch to > > the Try server so we can be sure it passes tests: > > > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9464db4af05 > > My bad, I rebased the patch incorrectly. Let me try that again: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=014760f912c8 Looks good. Kevin, once you address the comments in from comment 25, the patch will be ready to be committed!
In addition to the mentioned changes, I decided to group the Untransform* functions at the end of UnitTransforms.h so the where the new comment applies would be more clear.
Attachment #8626948 - Attachment is obsolete: true
Comment on attachment 8628151 [details] [diff] [review] w-coord-check-final.patch Review of attachment 8628151 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Kevin! Reworded commit message and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd3c5132792
Attachment #8628151 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2fd3c5132792
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Thanks again, Kevin, you did some good work here! If you're interested in working on another bug, and would like some suggestions, please let me know!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: