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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Attachment: apz-typed-matrices.patch

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.

Comment 1

[Botond Ballo [:botond]]

Comment on attachment 8703913 [details] [diff] [review]
apz-typed-matrices.patch

Oh whoops, this patch has a dependency that I should post first.

Comment 2

[Botond Ballo [:botond]]

Attachment: Part 1 - Rename ViewTransform to AsyncTransform

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.

Comment 3

[Botond Ballo [:botond]]

Attachment: Part 2 - Use strongly typed matrices to represent CSS and async transforms in APZ code

Comment 4

[Botond Ballo [:botond]]

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 5

[Botond Ballo [:botond]]

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 6

[Botond Ballo [:botond]]

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.

Comment 7

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

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.

Comment 8

[Botond Ballo [:botond]]

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.

Comment 9

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1236750 - Rename ViewTransform to AsyncTransform. r=kats

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/

Comment 10

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1236750 - Introduce a new unit type CSSTransformedLayerPixel. r=kats

Review commit: https://reviewboard.mozilla.org/r/29781/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29781/

Comment 11

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1236750 - Add a ViewAs() overload for casting (with a justification) one typed matrix to another. r=kats

Review commit: https://reviewboard.mozilla.org/r/29783/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29783/

Comment 12

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1236750 - Add some specialized typedefs of Matrix4x4 to represent layer transform matrices. r=kats

Also add a related PixelCastJustification and a utility function.

Review commit: https://reviewboard.mozilla.org/r/29785/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29785/

Comment 13

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1236750 - Add typedef getters for layer transform matrices. r=kats

Review commit: https://reviewboard.mozilla.org/r/29787/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29787/

Comment 14

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1236750 - Use strongly-typed matrices to represent layer transforms in APZ code. r=kats

Review commit: https://reviewboard.mozilla.org/r/29789/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29789/

Comment 15

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

Comment on attachment 8704862 [details]
MozReview Request: Bug 1236750 - Rename ViewTransform to AsyncTransform. r=kats

https://reviewboard.mozilla.org/r/29779/#review26623

Comment 16

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

Comment on attachment 8704863 [details]
MozReview Request: Bug 1236750 - Introduce a new unit type CSSTransformedLayerPixel. r=kats

https://reviewboard.mozilla.org/r/29781/#review26625

Comment 17

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

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

Comment 18

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

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 19

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

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

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 21

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

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.

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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).

Comment 23

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.)

Comment 24

[Botond Ballo [:botond]]

Try push with comments (except where noted) addressed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7372ea0eef02

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2606d3daf0b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/459d018540fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/247d76c15d7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/213b9d0906b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e3cc29c67d
https://hg.mozilla.org/integration/mozilla-inbound/rev/93c486c2a71b

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2606d3daf0b7
https://hg.mozilla.org/mozilla-central/rev/459d018540fe
https://hg.mozilla.org/mozilla-central/rev/247d76c15d7a
https://hg.mozilla.org/mozilla-central/rev/213b9d0906b0
https://hg.mozilla.org/mozilla-central/rev/09e3cc29c67d
https://hg.mozilla.org/mozilla-central/rev/93c486c2a71b