1173521 - Audit applications of matrices to rectangles in APZ code

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

We need to do something similar to what we did for points in bug 1120683, to rectangles.

See bug 1120683 comment 18 and bug 1120683 comment 19.

Comment 1

[Kevin Wern]

Hey, Botond.

I've been looking at this bug, and I'm kind of confused after reading through a lot of the code.  I've started to implement the UntransformTo() function for rectangles and did a first pass over instances where TransformTo or TransformBounds is used.

A few instances of rectangle transforms, and what I think:

* http://lxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp#1407 : Appears to perform potentially 3D untransforms (see GetTransformToAncestorsParentLayer, which is inverted in the linked function) and should have a w-coordinate check
* http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledPaintedLayer.cpp#61: Also inverts potentially 3d transforms--needs w-coordinate check.
* http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#157 : Only deals with 2d translations (and in one case, unapplying and reapplying potentially 3d transforms), does not need a w-coordinate check.
* http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1166 : Comments and logic indicates that transform must be 2D, does not need w-coordinate check.
* http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#679 : Only deals with APZC transforms, does not need w-coordinate check.

I guess it's safe to say I have a few questions:

* I also looked at a bunch of direct TransformBounds calls, but I'm not sure whats considered in the scope of the APZ project.  Every transform that I've checked to be either 2d or a forward 3d transform.  (Granted, I skimmed through the 2D and SVG directories, and mainly focused on layers/composition). There are a lot of calls in the project--is there any way to narrow them down to a smaller set?
* ProjectRectBounds takes in a clip, which is essentially a rectangle or view a transform result is constrained to.  What are some examples of what I would use as a clip?
* ProjectRectBounds doesn't really check w-coordinates in a direct way, at least not where it's readily apparent.  It basically checks for adjacent points with different HasPositiveWCoord values, and in the case where they are different, finds the w-intercept for display.  In my head, this doesn't handle the case where all w-coordinates are 0, but it's possible that in my UntransformTo implementation, I should just check to see if the resulting rectangle from ProjectRectBounds is empty, or that there should be no case where an already invisible rectangle would be transformed.  Could you give me any direction with this?

Thanks, and sorry it took me a while to look at this.

-Kevin

Comment 2

[Botond Ballo [:botond]]

Hi Kevin,

Thanks for looking at this! I've assigned the bug to you to indicate that you're working on it.

(In reply to kevin.m.wern from comment #1)
> *
> http://lxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TiledContentClient.cpp#1407 : Appears to perform potentially 3D untransforms
> (see GetTransformToAncestorsParentLayer, which is inverted in the linked
> function) and should have a w-coordinate check
> *
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> ClientTiledPaintedLayer.cpp#61: Also inverts potentially 3d
> transforms--needs w-coordinate check.
> *
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> AsyncCompositionManager.cpp#157 : Only deals with 2d translations (and in
> one case, unapplying and reapplying potentially 3d transforms), does not
> need a w-coordinate check.
> *
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#1166 : Comments and logic indicates that transform must be 2D, does not
> need w-coordinate check.
> *
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> AsyncCompositionManager.cpp#679 : Only deals with APZC transforms, does not
> need w-coordinate check.

That all looks correct to me!

> * I also looked at a bunch of direct TransformBounds calls, but I'm not sure
> whats considered in the scope of the APZ project.  Every transform that I've
> checked to be either 2d or a forward 3d transform.  (Granted, I skimmed
> through the 2D and SVG directories, and mainly focused on
> layers/composition). There are a lot of calls in the project--is there any
> way to narrow them down to a smaller set?

I think it's reasonable to restrict our attention in this bug to calls of TransformTo(). I've had a look through the direct calls of TransformBounds(), and none of them are in APZ code.

> * ProjectRectBounds takes in a clip, which is essentially a rectangle or
> view a transform result is constrained to.  What are some examples of what I
> would use as a clip?

Good question. It varies from case to case.

In our case, the transformations we've identified above as needing to use UntransformTo() involve transforming a rectangle from the coordinate space of an ancestor layer of a tiled layer, into the coordinate space of the tiled layer. Tiled layers are painted layers, and painted layers have a "layer bounds" property set on them [1] which bound all the content in the layer; we can use these bounds as the clip for transforms that target the coordinate space of the tiled layer.

The layer bounds can be obtained via GetLayerBounds() on the tiled layer (the tiled layer is "mPaintedLayer" in ClientMultiTiledLayerBuffer and "this" in ClientTiledPaintedLayer).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h?force=1#1744

> * ProjectRectBounds doesn't really check w-coordinates in a direct way, at
> least not where it's readily apparent.  It basically checks for adjacent
> points with different HasPositiveWCoord values, and in the case where they
> are different, finds the w-intercept for display.  In my head, this doesn't
> handle the case where all w-coordinates are 0, but it's possible that in my
> UntransformTo implementation, I should just check to see if the resulting
> rectangle from ProjectRectBounds is empty, or that there should be no case
> where an already invisible rectangle would be transformed.  Could you give
> me any direction with this?

I think it's sufficient to check in UntransformTo() whether the result of ProjectRectBounds() is empty (and return an empty Maybe in that case).

Finally, some advice on how to handle the failure case (empty Maybe) at the call sites:

  - ClientTiledPaintedLayer::BeginPaint() should change the fields of mPaintData that are
    initialized to 0/false/empty at the beginning of the function, back to those values,
    and return

  - ClientMultiTiledLayerBuffer::ComputeProgressiveUpdateRegion() should set 
    aPaintData->mPaintFinished to true, and return false
    (GetCompositorSideCompositionBounds() can just propagate the Maybe)

Comment 3

[Kevin Wern]

Attachment: apz-rectangle-w-coordinate-check.patch

I added a function to BasicTiledLayerPaintData that performs the 'reset' of mPaintData, but I'm not sure if the naming (Clear) is really accurate to the meaning, or if there is a better name.  I just thought repeating the code in 3 places looked redundant.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8650944 [details] [diff] [review]
apz-rectangle-w-coordinate-check.patch

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

Looking, good thanks! I just have a few comments.

I'm also going to ask Kats to review this, because the tiling code is tricky and it doesn't hurt to have more eyes on it.

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +54,5 @@
>    aAttrs = PaintedLayerAttributes(GetValidRegion());
>  }
>  
> +static Maybe<LayerRect>
> +ApplyParentLayerToLayerTransform(const gfx::Matrix4x4& aTransform, const ParentLayerRect& aParentLayerRect, const Rect& aClip)

Please take the clip in typed units and have the caller do the ViewAs().

::: gfx/layers/client/TiledContentClient.cpp
@@ +1414,4 @@
>  GetCompositorSideCompositionBounds(const LayerMetricsWrapper& aScrollAncestor,
>                                     const Matrix4x4& aTransformToCompBounds,
> +                                   const ViewTransform& aAPZTransform, 
> +				   const Rect& aClip)

Please take the clip in typed units and have the caller do the ViewAs().

@@ +1498,4 @@
>      }
>    }
>  
> +  Maybe<LayerRect> transformedCompositionBoundsResult =

Rather than introducing a new variable, I'd just leave this named |transformedCompositionBounds|, and rewrite its uses after the check to |*transformedCompositionBounds| or |transformedCompositionBounds->|.

::: layout/base/UnitTransforms.h
@@ +173,4 @@
>    return Some(RoundedToInt(ViewAs<TargetUnits>(point.As2DPoint())));
>  }
>  template <typename TargetUnits, typename SourceUnits>
> +static Maybe<gfx::RectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,

Please add a comment above these functions along the following lines:

// The versions of UntransformTo() that take a rectangle also take a clip,
// which represents the bounds within which the target must fall. The
// result of the transform is intersected with this clip, and is considered 
// meaningful if the intersection is not empty.

@@ +174,5 @@
>  }
>  template <typename TargetUnits, typename SourceUnits>
> +static Maybe<gfx::RectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,
> +                                                const gfx::RectTyped<SourceUnits>& aRect,
> +                                                const gfx::RectTyped<SourceUnits>& aClip)

The clip should be in the target units, because it's used to constrain the result of the transformation.

@@ +185,5 @@
> +}
> +template <typename TargetUnits, typename SourceUnits>
> +static Maybe<gfx::IntRectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,
> +                                                const gfx::IntRectTyped<SourceUnits>& aRect,
> +                                                const gfx::IntRectTyped<SourceUnits>& aClip)

Same here.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8650944 [details] [diff] [review]
apz-rectangle-w-coordinate-check.patch

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

::: layout/base/UnitTransforms.h
@@ +187,5 @@
> +static Maybe<gfx::IntRectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,
> +                                                const gfx::IntRectTyped<SourceUnits>& aRect,
> +                                                const gfx::IntRectTyped<SourceUnits>& aClip)
> +{
> +  gfx::Rect rect = aTransform.ProjectRectBounds(aRect.ToUnknownRect());

Just realized, you're missing the clip argument here.

Comment 6

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

Comment on attachment 8650944 [details] [diff] [review]
apz-rectangle-w-coordinate-check.patch

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

Thanks for working on this! I'm fairly sure this is good, although I have a really hard time visualizing 3D transforms so I may be wrong :).

I have some other comments below. Also in the future when attaching patches please generate them with 8 lines of context instead of 3 (you can add "unified = 8" to the [diff] section of your .hgrc file to accomplish this). Thanks!

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +160,4 @@
>    gfx::Matrix4x4 transformDisplayPortToLayer =
>      GetTransformToAncestorsParentLayer(this, displayPortAncestor);
>    transformDisplayPortToLayer.Invert();
> +  

nit: remove trailing whitepace here and a few other places in the patch

@@ +173,5 @@
>        (displayportMetrics.GetCriticalDisplayPort() * displayportMetrics.GetZoom())
>        + displayportMetrics.GetCompositionBounds().TopLeft();
> +    Maybe<LayerRect> criticalDisplayPortTransformed =
> +      ApplyParentLayerToLayerTransform(transformDisplayPortToLayer, criticalDisplayPort, layerBounds);
> +    if (!criticalDisplayPortTransformed){

nit: Please insert a space before '{' (here and everywhere else in this patch)

::: gfx/layers/client/TiledContentClient.cpp
@@ +1700,5 @@
>    mTiledBuffer.Dump(aStream, aPrefix, aDumpHtml);
>  }
>  
> +void
> +BasicTiledLayerPaintData::Clear()

I'd call this ResetPaintData()

::: layout/base/UnitTransforms.h
@@ +176,5 @@
> +static Maybe<gfx::RectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,
> +                                                const gfx::RectTyped<SourceUnits>& aRect,
> +                                                const gfx::RectTyped<SourceUnits>& aClip)
> +{
> +  gfx::Rect rect = aTransform.ProjectRectBounds(aRect.ToUnknownRect(), aRect.ToUnknownRect());

Shouldn't this be passing aClip as the second param?

Comment 7

[Kevin Wern]

Attachment: apz-rectangle-w-coordinate-check-revised.patch

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8652756 [details] [diff] [review]
apz-rectangle-w-coordinate-check-revised.patch

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

Thanks! Just one issue left to fix.

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +160,5 @@
>    gfx::Matrix4x4 transformDisplayPortToLayer =
>      GetTransformToAncestorsParentLayer(this, displayPortAncestor);
>    transformDisplayPortToLayer.Invert();
>  
> +  Rect layerBounds = Rect(GetLayerBounds());

Might as well make this a LayerRect, so you only have to do the ViewAs once.

::: layout/base/UnitTransforms.h
@@ +181,5 @@
> +static Maybe<gfx::RectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,
> +                                                const gfx::RectTyped<SourceUnits>& aRect,
> +                                                const gfx::RectTyped<TargetUnits>& aClip)
> +{
> +  gfx::Rect rect = aTransform.ProjectRectBounds(aRect.ToUnknownRect(), aRect.ToUnknownRect());

The second argument should be |aClip.ToUnknownRect()|.

@@ +192,5 @@
> +static Maybe<gfx::IntRectTyped<TargetUnits>> UntransformTo(const gfx::Matrix4x4& aTransform,
> +                                                const gfx::IntRectTyped<SourceUnits>& aRect,
> +                                                const gfx::IntRectTyped<TargetUnits>& aClip)
> +{
> +  gfx::Rect rect = aTransform.ProjectRectBounds(aRect.ToUnknownRect(), aRect.ToUnknownRect());

Same here.

Comment 9

[Kevin Wern]

Attachment: apz-rectangle-w-coordinate-check-revised-2.patch

Sorry for seeming so dense missing that last change--I thought you and Kat were commenting on the same issue with UntransformTo the first reviews, so I foolishly skimmed Kat's last point assuming it was the same.

Never assume when it comes to coding (or anything), I guess.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8653243 [details] [diff] [review]
apz-rectangle-w-coordinate-check-revised-2.patch

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

Great, thanks!

Comment 11

[Botond Ballo [:botond]]

Pushed patch to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc575dc9e18

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5a9ebbca96

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> Pushed patch to Try:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc575dc9e18

We're experiencing some infrastructure problems that caused a lot of the builds to fail, but we did get a full set of mochitest and reftest runs for "B2G ICS Emulator opt", and they're green, and the patch is building on enough platforms, that I'm happy with the Try push, so I went ahead and landed the patch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb5a9ebbca96

Comment 15

[Botond Ballo [:botond]]

Thanks again for your work on this bug, Tim!

If you're interested in working on something else, and would like some suggestions, let me know :)

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> Thanks again for your work on this bug, Tim!

Sorry, s/Tim/Kevin! (Mixed up bugs in my mind, was thinking of Tim who worked on bug 1171312...)