Closed Bug 1236750 Opened 4 years ago Closed 4 years ago

Use strongly typed matrices to represent CSS and async transforms in APZ code

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(6 files, 3 obsolete files)

58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
Attached patch apz-typed-matrices.patch (obsolete) — Splinter Review
In bug 1069417, we introduced strongly typed matrix classes and started using them in many places in APZ code.

Some places in APZ code, however, still use untyped matrices, notably to represent CSS and async transforms.

Based on a discussion with Kats in Orlando, I tried to bring strong typing to these quantities by introducing a CSSTransformedLayerPixel which represents the result of applying a layer's CSS transform, but not its async transforms.

A CSS transform can then be represented as a Layer -> CSSTransformedLayer transform.

Representing async transforms is a bit more tricky. If there were only one per layer, we could represent it as a CSSTransformedLayer -> ParentLayer transform. However, there are multiple per layer (one for async animations, and two ("regular" and "overscroll") for each APZC associated with the layer).

To try to make sense of this, I introduced two new typedefs:

  - AsyncTransformMatrix is a CSSTransformedLayer -> ParentLayer transform,
    and represents the product of all async transforms that pertain to
    a layer.

  - AsyncTransformComponentMatrix is a ParentLayer -> ParentLayer transform,
    and represents a single async transform pertaining to a layer.

I then used these in one of two ways:

  1) Multiple component matrices together, and ViewAs the result (with a 
     justification added for this purpose) to say "these are all the 
     components, we now have a complete AsyncTransformMatrix).

  2) Create an initial Layer -> ParentLayer matrix by starting with the
     layer's CSS transform matrix multipled by an empty AsyncTransformMatrix,
     and then multiply AsyncTransformComponentMatrices "into it" to apply
     various async transforms. (This is what AsyncCompositionManager does.)

It's not entirely clear to me whether the resulting code constitutes a net improvement in readability and type safety; Kats, I'd be curious to hear what you think.
Attachment #8703913 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8703913 [details] [diff] [review]
apz-typed-matrices.patch

Oh whoops, this patch has a dependency that I should post first.
Attachment #8703913 - Attachment is obsolete: true
Attachment #8703913 - Flags: feedback?(bugmail.mozilla)
This is just a straight rename. The idea is to limit the scope of the class, to justify replacing (in the next patch) its conversion operator to Matrix4x4 with a conversion operator to a specific types matrix. All existing uses are already in the intended scope.
Attachment #8703918 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8703919 [details] [diff] [review]
Part 2 - Use strongly typed matrices to represent CSS and async transforms in APZ code

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +388,5 @@
>      node = RecycleOrCreateNode(aState, nullptr, aLayersId);
>      AttachNodeToTree(node, aParent, aNextSibling);
> +    node->SetHitTestData(
> +        GetEventRegions(aLayer),
> +        ViewAs<CSSTransformMatrix>(aLayer.GetTransform()),

I meant to move the GetLayerTransform() helper in AsyncCompositionManager.cpp to a shared header, and call it here and in similar places.

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +227,5 @@
>  HitTestingTreeNode::Untransform(const ParentLayerPoint& aPoint) const
>  {
>    // convert into Layer coordinate space
> +  LayerToParentLayerMatrix4x4 transform = mTransform *
> +      ViewAs<AsyncTransformMatrix>(

And here, CompleteAsyncTransform().
Comment on attachment 8703919 [details] [diff] [review]
Part 2 - Use strongly typed matrices to represent CSS and async transforms in APZ code

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +240,5 @@
> +{
> +  return ViewAs<CSSTransformMatrix>(aLayer->GetTransform());
> +}
> +
> +// Get a layer's shadow transform in typed units.

(The idea is, of course, for Layer::GetTransform() and Layer::GetLocalTransform() to eventually return typed matrices themselves; these helper functions are just to provide a bridge until their call sites are all transitioned. I could make these Layer::GetTransformTyped() and GetLocalTransformTyped() instead, if you prefer.)
Comment on attachment 8703919 [details] [diff] [review]
Part 2 - Use strongly typed matrices to represent CSS and async transforms in APZ code

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

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +227,5 @@
>  HitTestingTreeNode::Untransform(const ParentLayerPoint& aPoint) const
>  {
>    // convert into Layer coordinate space
> +  LayerToParentLayerMatrix4x4 transform = mTransform *
> +      ViewAs<AsyncTransformMatrix>(

One more thing I should point out: 

While I was writing this, I realized that AsyncCompositionManager and APZCTreeManager use ParentLayer coordinates in slightly different ways. 

In AsyncCompositionManager, ParentLayer coordinates actually refer to the parent layer. That is, to get from Layer to ParentLayer you apply async transforms corresponding to _all_ APZCs that scroll that layer (in addition to the layer's CSS transform).

In APZCTreeManager, we use ParentLayer as meaning something more like Parent{LayerMetrics}. That is, we consider ParentLayer pixels the result of applying the CSS and async transforms of a single LayerMetrics.

I'm pretty sure this discrepancy doesn't cause any correctness problems - we're just naming things in slightly different ways - but I thought I'd point it out anyways.
Attachment #8703918 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8703919 [details] [diff] [review]
Part 2 - Use strongly typed matrices to represent CSS and async transforms in APZ code

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

On the whole I do think this is a win. With further changes to make Layer::GetTransform and such return the right type I think it will help a lot. Thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +230,5 @@
>    /**
>     * Returns the same transform as GetCurrentAsyncTransform(), but includes
>     * any transform due to axis over-scroll.
>     */
> +  AsyncTransformComponentMatrix GetCurrentAsyncTransformWithOverscroll() const;

I guess technically this includes two different transforms - the async transform and the overscroll transform. But I agree it makes sense to treat this as a AsyncTransformComponentMatrix.
Attachment #8703919 - Flags: feedback?(bugmail.mozilla) → feedback+
Cleaned up patches and split the second one up a bit. Posting for review.

(In reply to Botond Ballo [:botond] from comment #5)
> I could make these Layer::GetTransformTyped() and
> GetLocalTransformTyped() instead, if you prefer.

I ended up doing this.
It's only used to represent async transforms, and making this clear
allows us to replace (in a subsequent change) its conversion operator
to Matrix4x4 with a conversion operator to a specific typed matrix.

Review commit: https://reviewboard.mozilla.org/r/29779/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29779/
Attachment #8704862 - Flags: review?(bugmail.mozilla)
Attachment #8703918 - Attachment is obsolete: true
Attachment #8703919 - Attachment is obsolete: true
Comment on attachment 8704862 [details]
MozReview Request: Bug 1236750 - Rename ViewTransform to AsyncTransform. r=kats

https://reviewboard.mozilla.org/r/29779/#review26623
Attachment #8704862 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8704863 [details]
MozReview Request: Bug 1236750 - Introduce a new unit type CSSTransformedLayerPixel. r=kats

https://reviewboard.mozilla.org/r/29781/#review26625
Attachment #8704863 - Flags: review?(bugmail.mozilla) → review+
Attachment #8704864 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8704864 [details]
MozReview Request: Bug 1236750 - Add a ViewAs() overload for casting (with a justification) one typed matrix to another. r=kats

https://reviewboard.mozilla.org/r/29783/#review26627
Attachment #8704865 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8704865 [details]
MozReview Request: Bug 1236750 - Add some specialized typedefs of Matrix4x4 to represent layer transform matrices. r=kats

https://reviewboard.mozilla.org/r/29785/#review26629

::: gfx/layers/apz/src/APZUtils.h:12
(Diff revision 1)
> +#include "UnitTransforms.h"

Move UnitTransforms.h down to the bottom of the list to alphabetize

::: gfx/layers/apz/src/APZUtils.h:67
(Diff revision 1)
> +// one or more AsyncTarnsformComponentMatrix objects) as constituting a

typo s/Tarn/Tran/
Comment on attachment 8704866 [details]
MozReview Request: Bug 1236750 - Add typedef getters for layer transform matrices. r=kats

https://reviewboard.mozilla.org/r/29787/#review26631
Attachment #8704866 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> MozReview Request: Bug 1236750 - Add typedef getters for layer transform
> matrices. r=kats

Whoops, that commit message is meant to say "typed getters", not "typedef getters". Will correct before landing.

(I guess my fingers are conditioned to, having typed "typed", go on to type "ef" :)).
Comment on attachment 8704867 [details]
MozReview Request: Bug 1236750 - Use strongly-typed matrices to represent layer transforms in APZ code. r=kats

https://reviewboard.mozilla.org/r/29789/#review26633

::: gfx/layers/apz/src/HitTestingTreeNode.cpp:12
(Diff revision 1)
> +#include "mozilla/layers/APZUtils.h"                    // for CompleteAsyncTransform

move this below APZThreadUtils

::: gfx/layers/composite/AsyncCompositionManager.cpp:1135
(Diff revision 1)
> -    Matrix4x4 asyncUntransform = (asyncTransform * apzc->GetOverscrollTransform()).Inverse();
> +    Matrix4x4 asyncUntransform = (asyncTransform * apzc->GetOverscrollTransform()).Inverse().ToUnknownMatrix();

I think you can make this a AsyncTransformComponentMatrix, use the Typed version on the next line, and get rid of the ViewAs a few lines down.
Attachment #8704867 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 8704865 [details]
> MozReview Request: Bug 1236750 - Add some specialized typedefs of Matrix4x4
> to represent layer transform matrices. r=kats
> 
> https://reviewboard.mozilla.org/r/29785/#review26629
> 
> ::: gfx/layers/apz/src/APZUtils.h:12
> (Diff revision 1)
> > +#include "UnitTransforms.h"
> 
> Move UnitTransforms.h down to the bottom of the list to alphabetize

I've been alphabetizing things in ASCII order (capital letters before small letters).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:1135
> (Diff revision 1)
> > -    Matrix4x4 asyncUntransform = (asyncTransform * apzc->GetOverscrollTransform()).Inverse();
> > +    Matrix4x4 asyncUntransform = (asyncTransform * apzc->GetOverscrollTransform()).Inverse().ToUnknownMatrix();
> 
> I think you can make this a AsyncTransformComponentMatrix, use the Typed
> version on the next line, and get rid of the ViewAs a few lines down.

I'm afraid the units don't quite work out here. The typed contentTransform is a Layer -> CSSTransformedLayer matrix; to multiply that with an AsyncTransformComponentMatrix, we first need to complete that to an AsyncTransformMatrix. Say we do that; their product will be a Layer -> ParentLayer matrix, which cannot be multiplied by the contentUntransform, which is a CSSTransformedLayer -> Layer matrix.

In an effort to try and make the units work out, I went back and re-read bug 1043859 comment 18, where you explain the mathematical basis of the computation being performed here. Looking at what's happening there, the transforms of two different layers (the scrollbar layer, and scrolled content layer) are in play at once. I don't think our strongly typed units are capable of representing that computation right now, necessitating an escape to untyped units. (I'm open to being corrected on this point.)
Try push with comments (except where noted) addressed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7372ea0eef02
You need to log in before you can comment on or make changes to this bug.