1168263 - [APZ] Scrolling 3d-transformed things is whacky

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

Attachment 8610003 [details] scrolls very strangely. It looks like APZ and the main thread don't agree about the post-scroll transform.

Comment 1

[Markus Stange [:mstange]]

See http://keithclark.co.uk/articles/pure-css-parallax-websites/ for a good explanation and small test cases.

Comment 2

[Matt Woodrow (:mattwoodrow)]

The elements being scrolled are being scrolled relative to the element that establishes the perspective origin.

The computation of the perspective component of the transform [1] needs to be repeated in order for us to scroll these correctly.

I think the only way to fix this is to ship the perspective across to the compositor, and do the combination of the various transform sources (css transform, perspective transform, scroll translation, svg transform stuff?) during APZ.

We can probably refactor GetResultingTransformMatrixInternal a bit so that we can use the same code for this combination for both the main thread and APZ.


[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#4969

Comment 3

[Matt Woodrow (:mattwoodrow)]

Kats, we need to handle the case where the transformed element is scrolled relative to the perspective element (always its parent).

Any ideas on what the best way to store and compute this in our code? Store the scroll frame id of the parent frame if it differs from that of the current frame?

Comment 4

[Matt Woodrow (:mattwoodrow)]

My current plan is to introduce an optional data object on Layer for recomputing transforms.

Layers with perspective (and a scroll frame between the transform and perspective) can compute one of these in addition to the transform, and APZ can use this to compute a new, scrolled transform.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Simplified testcase

Comment 6

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

We discussed this on IRC, and while I still don't have a solid mental model about this, it seems like adding the perspective info to the layer and then adjusting both the APZ hit-testing code and AsyncCompositionManager transform-combination code makes sense.

Another option might be to just bail on content with perspectives set, and fall back to synchronous scrolling. If things get too crazy that might be a short-term backup plan.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP

Initial WIP, seems to 'work' and fixes this testcase.

Creates a new ContainerLayer for the perspective transform instead of combining it into the normal transform.

Since the perspective transform has no effect on 2d rectangles when applied in isolation, we continue to add the perspective into the transform item when computing visible rects etc. Only during layer construction is the perspective omitted and added to the perspective layer instead. 

Things to do:

* Wait for bug 1097464 to land, and tidy up a bunch of stuff related to that.

* Create the nsDisplayPerspective items in the BuildDisplayListForStackingContext for the transformed frame (not the perspective frame) and add an index (for GetPerFrameKey) since we can now have multiple for the perspective frame.

* Remove any clipping from the nsDisplayTransform and put it on the nsDisplayPerspective. This makes sure we don't end up with any clip on the inner ContainerLayer which would force an intermediate surface and break rendering.

* Make sure the layer state of the nsDisplayPerspective and nsDisplayTransform are always the same, and if they belong to different active geometry roots (i.e. the transform scrolls independently to the perspective) then make sure this is LAYER_ACTIVE.

* Fix APZ assertion that is being hit.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP v2

Cleaned up most pieces, still hitting a bunch of APZ asserts, hopefully bug 1202050 will fix that.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove TransformRectOut since it's unused

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add ApplyScrollToChild for APZ

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add nsDisplayPerspective

This does a few things:

* Adds a flags word to GetResultingTransformMatrix
* Add a new nsDisplayPerspectiveItem display item.
* Hooks up FrameLayerBuilder to tell APZ about these so they get scrolled correctly.

I can split the first piece out if necessary, it was just non-trivial to do so.

This seems to all work, and the remaining reftest failures should be fixed by the dependent bugs.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8674702 [details] [diff] [review]
Part 2: Add ApplyScrollToChild for APZ

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

::: gfx/layers/FrameMetrics.h
@@ +105,5 @@
>             mClipRect == aOther.mClipRect &&
>             mMaskLayerIndex == aOther.mMaskLayerIndex &&
>             mIsLayersIdRoot == aOther.mIsLayersIdRoot &&
> +		   mUsesContainerScrolling == aOther.mUsesContainerScrolling &&
> +		   mApplyScrollToChild == aOther.mApplyScrollToChild;

I'll fix this indenting...

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8674702 [details] [diff] [review]
Part 2: Add ApplyScrollToChild for APZ

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

This generally looks good. However, there are some other places where we need to do something different based on the value of ApplyScrollToChild(). For example, APZCTreeManager::GetScreenToApzcTransform() and APZCTreeManager::GetApzcToGeckoTransform() need to multiply in the async transform in a different place for layers for which ApplyScrollToChild() is true. There may be other places as well.

If you change this patch to just introduce the flag, I'm happy to write the follow-up patch which checks it in the various places where it's needed.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +518,4 @@
>    Matrix4x4 transform =
>      nsDisplayTransform::GetResultingTransformMatrix(props, origin,
>                                                      data.appUnitsPerDevPixel(),
> +                                                    0,

These changes to SampleValue() seem like they should be in a different patch.

@@ +898,5 @@
> +      MOZ_ASSERT(child);
> +      MOZ_ASSERT(!child->GetMaskLayer());
> +      MOZ_ASSERT(!child->AsLayerComposite()->GetShadowClipRect());
> +      SetShadowTransform(child,
> +        child->GetLocalTransform() * combinedAsyncTransformForChild);

Rather than applying the async transform to the child, would it be equivalent to apply the async transform to this layer, but multiply it on the left? i.e.

  SetShadowTransform(aLayer,
    combinedAsyncTransform * child->GetLocalTransform());

I'm concerned that setting a transform on the child here has other ramifications (e.g. fixed and sticky layers in its subtree will not be aligned correctly).

If you agree that this equivalency holds, perhaps we can rename the flag to something like ApplyAsyncTransformBeforeCSSTransform, or something like that?

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Botond Ballo [:botond] from comment #13)
 
> This generally looks good. However, there are some other places where we
> need to do something different based on the value of ApplyScrollToChild().
> For example, APZCTreeManager::GetScreenToApzcTransform() and
> APZCTreeManager::GetApzcToGeckoTransform() need to multiply in the async
> transform in a different place for layers for which ApplyScrollToChild() is
> true. There may be other places as well.
> 
> If you change this patch to just introduce the flag, I'm happy to write the
> follow-up patch which checks it in the various places where it's needed.

Ok, that sounds great!

> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +518,4 @@
> >    Matrix4x4 transform =
> >      nsDisplayTransform::GetResultingTransformMatrix(props, origin,
> >                                                      data.appUnitsPerDevPixel(),
> > +                                                    0,
> 
> These changes to SampleValue() seem like they should be in a different patch.

Right, these should be in part 3.

> 
> @@ +898,5 @@
> > +      MOZ_ASSERT(child);
> > +      MOZ_ASSERT(!child->GetMaskLayer());
> > +      MOZ_ASSERT(!child->AsLayerComposite()->GetShadowClipRect());
> > +      SetShadowTransform(child,
> > +        child->GetLocalTransform() * combinedAsyncTransformForChild);
> 
> Rather than applying the async transform to the child, would it be
> equivalent to apply the async transform to this layer, but multiply it on
> the left? i.e.
> 
>   SetShadowTransform(aLayer,
>     combinedAsyncTransform * child->GetLocalTransform());
> 
> I'm concerned that setting a transform on the child here has other
> ramifications (e.g. fixed and sticky layers in its subtree will not be
> aligned correctly).
> 
> If you agree that this equivalency holds, perhaps we can rename the flag to
> something like ApplyAsyncTransformBeforeCSSTransform, or something like that?

That should work just fine too, I'm happy to make that change.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8674703 [details] [diff] [review]
Part 3: Add nsDisplayPerspective

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

Generally looks really good!

::: layout/base/FrameLayerBuilder.cpp
@@ +4130,5 @@
>        newLayerEntry->mAnimatedGeometryRoot = animatedGeometryRoot;
>        newLayerEntry->mAnimatedGeometryRootForScrollMetadata = animatedGeometryRootForScrollMetadata;
>        newLayerEntry->mFixedPosFrameForLayerData = fixedPosFrame;
> +      if (item->GetType() == nsDisplayItem::TYPE_PERSPECTIVE) {
> +        newLayerEntry->mIsPerspectiveItem = true;

Use itemType

::: layout/base/nsDisplayList.cpp
@@ +4975,5 @@
>  
> +  nscoord perspective = 0;
> +  Point3D perspectiveOrigin =
> +    GetDeltaToPerspectiveOrigin(frame, aAppUnitsPerPixel, perspective);
> +  bool hasPerspective = perspective > 0 && aFlags & INCLUDE_PERSPECTIVE;

() around & subexpression

@@ +4982,5 @@
>      // This is a simplification of the following |else| block, the
>      // simplification being possible because we don't need to apply
>      // mToTransformOrigin between two transforms.
>      Point3D offsets = roundedOrigin + aProperties.mToTransformOrigin;
> +    if (aFlags & OFFSET_BY_ORIGIN &&

here too

@@ +5017,5 @@
>      // Similar to the code in the |if| block above, but since we've accounted
>      // for mToTransformOrigin so we don't include that. We also need to reapply
>      // refBoxOffset.
>      Point3D offsets = roundedOrigin + refBoxOffset;
> +    if (aFlags & OFFSET_BY_ORIGIN &&

etc

@@ +5041,5 @@
>        result.PreTranslate(roundedOrigin);
>      }
>    }
>  
> +  if (aFlags & INCLUDE_PRESERVE3D_ANCESTORS &&

etc

@@ +5786,5 @@
> +
> +  // Sort of a lie, but we want to pretend that the perspective layer extends a 3d context
> +  // so that it gets its transform combined with children. Might need a better name that reflects
> +  // this use case and isn't specific to preserve-3d.
> +  container->SetContentFlags(container->GetContentFlags() | Layer::CONTENT_EXTEND_3D_CONTEXT);

How about a separate flag CONTENT_ACCUMULATE_TRANSFORM_INTO_CHILDREN or something like that?

::: layout/base/nsDisplayList.h
@@ +3918,5 @@
>    // True if mTransformPreserves3D have been initialized.
>    bool mTransformPreserves3DInited;
>  };
>  
> +class nsDisplayPerspective : public nsDisplayItem

Add a comment explaining why we've got a separate item for this.

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)
> This generally looks good. However, there are some other places where we
> need to do something different based on the value of ApplyScrollToChild().
> For example, APZCTreeManager::GetScreenToApzcTransform() and
> APZCTreeManager::GetApzcToGeckoTransform() need to multiply in the async
> transform in a different place for layers for which ApplyScrollToChild() is
> true. There may be other places as well.
> 
> If you change this patch to just introduce the flag, I'm happy to write the
> follow-up patch which checks it in the various places where it's needed.

I've been thinking some more about this. 

Re-capping the discussion in bug 1213716 for context: 

We're talking about a layer tree that looks something like this:

     A
   /   \
  B     C
        |
        D

where B and D scroll together, and D is affected by a perspective transform stored on C. APZ currently doesn't support this because it requires that layers that scroll together each have the same transform to their common ancestor.

The proposed approaches for fixing this are:

  (1) Keep the FrameMetrics on D and the transform on C.
      Annotate the transform on C as being a special perspective transform.
      
      From APZ's point of view, this entails ignoring the transform on C
      for certain purposes, such as converting input events from screen
      coordinates to the coordinates of the scroll frame.

      This is what was originally proposed, but it suffers from the problem
      described in bug 1213716 comment 6 (the clip in the FrameMetrics on D
      is inteded to apply to C, not to D).

  (2) Put the FrameMetrics on C, but do something to ensure that the
      perspective transform on C is applied after the async transform
      associated with the FrameMetrics.

      There are two variations on this:

        (a) Store the perspective transform in the regular CSS transform
            field, but add a flag saying that it should be applied after
            the async transform.

            IIUC this is what the current patches implement.

        (b) Add a new "post-async" transform field to Layer, which is
            always applied after the async transform, and store the 
            perspective transform in that.

            This is discussed in bug 1213716 comments 7-8.
            
After considering the impact of the various options on APZ code, I would very much prefer to do (1):

  - (2a) requires changing every piece of code that applies both regular
    and async transforms (which is quite a few places) to check the flag
    and apply them in a different order based on the flag.

  - (2b) similarly requires changing all these places to also apply the
    post-async transform.

  - (1), by contrast, ensures that the existing logic of "apply the
    async transform on top of the regular transform" continues to be
    valid, and only affects a very specific place in APZ code where we
    want to ignore the perspective transform.

Matt, is there a way we can do (1), and somehow solve the problem it suffers from (related to the clip)? For example, by storing the clip in question on layer C instead of the FrameMetrics, or by changing the code that applies the clip to check the flag/annotation and apply it in the correct place?

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to Botond Ballo [:botond] from comment #16)
> Matt, is there a way we can do (1), and somehow solve the problem it suffers
> from (related to the clip)? For example, by storing the clip in question on
> layer C instead of the FrameMetrics, or by changing the code that applies
> the clip to check the flag/annotation and apply it in the correct place?


Yeah, if it makes a big difference for the APZ code then we can probably do (1).

(2) ended up being much more natural from the layout side which I why I went with that, but I think I can work around it without too many problems.

Comment 19

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

Bug 1168263 - Proposed Layers API

Comment 20

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1168263 - Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space. r=kats

Bug 1168263 - APZ changes

Comment 21

[Botond Ballo [:botond]]

The attached patches show the new layers API (new flag on Layer) I'm proposing for approach (1), and the APZ changes that make use of it (which ended up being quite minimal).

Matt, if you adapt the layout side of things to set this flag, you should be able to test with these patches and no longer run into the assertion described in bug 1213716.

Comment 22

[Matt Woodrow (:mattwoodrow)]

(In reply to Botond Ballo [:botond] from comment #21)
> The attached patches show the new layers API (new flag on Layer) I'm
> proposing for approach (1), and the APZ changes that make use of it (which
> ended up being quite minimal).
> 
> Matt, if you adapt the layout side of things to set this flag, you should be
> able to test with these patches and no longer run into the assertion
> described in bug 1213716.

Won't these patches still have the finger dragging problem (described in bug 1213716 comment 0)?

Comment 23

[Botond Ballo [:botond]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> Won't these patches still have the finger dragging problem (described in bug
> 1213716 comment 0)?

I don't expect so. Let's go through an example.

Say you have this layer tree:

     A
   /   \
  B     C
        |
        D

with B and D sharing an APZC, and C having a transform that scales by 2x. Assume all other scales are 1, so that 100 CSS pixels of content in B correspond to 100 screen pixels, and 100 CSS pixels of content in D correspond to 200 screen pixels.

My understanding is that if the user drags their finger by 100 screen pixels, we want the corresponding scroll to be 100 CSS pixels, regardless of whether the scroll is over B or D. (This effectively means that visually, content in D will scroll twice as fast as content in B).

Since the patch ignores perspective transforms when computing the ancestor-transform, the APZC's ancestor-transform will be the identity, and the scroll of 100 screen pixels will be interpreted as 100 pixels in the APZC's space (which is also 100 CSS pixels since there are no other scales). This scroll of 100 pixels will be reflected in the async transforms on layers B and D. After applying C's transform, we get the visual effect that content in B scrolled by 100 pixels while content in D scrolled by 200 pixels.

Let me know if that makes sense, or if I missed something.

Comment 24

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

(I'm assuming that fixing this bug will give us a good way to implement parallax scrolling with APZ, which we can then evangelize to authors for all the parallax issues we have).

Comment 25

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

Oops, typo

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Botond Ballo [:botond] from comment #16)

> 
> Matt, is there a way we can do (1), and somehow solve the problem it suffers
> from (related to the clip)? For example, by storing the clip in question on
> layer C instead of the FrameMetrics, or by changing the code that applies
> the clip to check the flag/annotation and apply it in the correct place?

Ok, had a go at rebasing this and building on top of your patches.

The FrameMetrics we compute for D include a clip to the scrollport bounds, and the current APZ code then applies this to the shadow-clip for D.

We need this clip to be on C for things to work correctly, and it's not obvious how to do that from the layout side.

I think we need APZ to check if the parent layer is a perspective one, and propagate clipping it to it if necessary.

Note that your current patch is also missing the change to ShadowLayers.cpp that copies mTransformIsPerspective into CommonLayerAttributes.

Comment 27

[Botond Ballo [:botond]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> (In reply to Botond Ballo [:botond] from comment #16)
> 
> > 
> > Matt, is there a way we can do (1), and somehow solve the problem it suffers
> > from (related to the clip)? For example, by storing the clip in question on
> > layer C instead of the FrameMetrics, or by changing the code that applies
> > the clip to check the flag/annotation and apply it in the correct place?
> 
> Ok, had a go at rebasing this and building on top of your patches.
> 
> The FrameMetrics we compute for D include a clip to the scrollport bounds,
> and the current APZ code then applies this to the shadow-clip for D.
> 
> We need this clip to be on C for things to work correctly, and it's not
> obvious how to do that from the layout side.
> 
> I think we need APZ to check if the parent layer is a perspective one, and
> propagate clipping it to it if necessary.

It sounds like this is something we could do in AsyncCompositionManager. I can put together a patch illustrating how, if you'd like.

> Note that your current patch is also missing the change to ShadowLayers.cpp
> that copies mTransformIsPerspective into CommonLayerAttributes.

Indeed, thanks. (I actually had this fixed locally, just forgot to upload the updated patch.)

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8682123 [details]
MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23975/diff/1-2/

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8682124 [details]
MozReview Request: Bug 1168263 - Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23977/diff/1-2/

Comment 30

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #27)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> > (In reply to Botond Ballo [:botond] from comment #16)
> > 
> > > 
> > > Matt, is there a way we can do (1), and somehow solve the problem it suffers
> > > from (related to the clip)? For example, by storing the clip in question on
> > > layer C instead of the FrameMetrics, or by changing the code that applies
> > > the clip to check the flag/annotation and apply it in the correct place?
> > 
> > Ok, had a go at rebasing this and building on top of your patches.
> > 
> > The FrameMetrics we compute for D include a clip to the scrollport bounds,
> > and the current APZ code then applies this to the shadow-clip for D.
> > 
> > We need this clip to be on C for things to work correctly, and it's not
> > obvious how to do that from the layout side.
> > 
> > I think we need APZ to check if the parent layer is a perspective one, and
> > propagate clipping it to it if necessary.
> 
> It sounds like this is something we could do in AsyncCompositionManager. I
> can put together a patch illustrating how, if you'd like.

The third patch I posted implements this. Let me know if it does the trick!

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add nsDisplayPerspective v2

Yep, all seems to work now!

Comment 33

[Botond Ballo [:botond]]

Great! I've cleaned up my patches and bit and will post them for review.

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8682123 [details]
MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23975/diff/2-3/

Comment 35

[Botond Ballo [:botond]]

Comment on attachment 8682124 [details]
MozReview Request: Bug 1168263 - Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23977/diff/2-3/

Comment 36

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats

Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 8689151 [details]
MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25559/diff/1-2/

Comment 38

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

Comment on attachment 8682124 [details]
MozReview Request: Bug 1168263 - Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space. r=kats

https://reviewboard.mozilla.org/r/23977/#review23115

Comment 39

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

Comment on attachment 8689631 [details]
MozReview Request: Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats

https://reviewboard.mozilla.org/r/25681/#review23117

Comment 40

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

Comment on attachment 8689151 [details]
MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

https://reviewboard.mozilla.org/r/25559/#review23121

This is partly a rubber stamp, I don't think I fully understand this code but it looks sane other than the one comment I have below.

::: gfx/layers/composite/AsyncCompositionManager.cpp:910
(Diff revision 2)
> -      aLayer->AsLayerComposite()->SetShadowClipRect(asyncClip);
> +        IntersectMaybeRects(asyncClip, clipDeferredFromChildren));

Under the following conditions:
hasAsyncTransform = true
asynClip = true
clipDeferredFromChildren = false

the old code would still set asyncClip as the shadow clip rect, but the new code doesn't set a shadow clip rect at all. Is this intentional?

Comment 41

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #40)
> Under the following conditions:
> hasAsyncTransform = true
> asynClip = true
> clipDeferredFromChildren = false
> 
> the old code would still set asyncClip as the shadow clip rect, but the new
> code doesn't set a shadow clip rect at all. Is this intentional?

Ignore this, I misread the code. It looks fine.

Comment 42

[Botond Ballo [:botond]]

Comment on attachment 8689151 [details]
MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

I'd like Markus to have a look at this as well, because this is tricky code, and he's touching it in bug 1147673.

Comment 43

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8682123 [details]
MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

https://reviewboard.mozilla.org/r/23975/#review23143

::: gfx/layers/Layers.h:1806
(Diff revision 3)
> +  bool mTransformIsPerspective;

Need to initialize this to false in the constructor, since we won't explicitly set it for most layers.

Comment 44

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8682123 [details]
MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

https://reviewboard.mozilla.org/r/23975/#review23145

Comment 45

[Botond Ballo [:botond]]

Comment on attachment 8682123 [details]
MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23975/diff/3-4/

Comment 46

[Botond Ballo [:botond]]

Comment on attachment 8682124 [details]
MozReview Request: Bug 1168263 - Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23977/diff/3-4/

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8689631 [details]
MozReview Request: Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25681/diff/1-2/

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8689151 [details]
MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25559/diff/2-3/

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 8689151 [details]
MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

https://reviewboard.mozilla.org/r/25559/#review23195

I don't foresee any problems with this. If our assumptions are wrong, the assertion will catch it.

::: gfx/layers/composite/AsyncCompositionManager.cpp:750
(Diff revision 3)
> +                                                          Maybe<ParentLayerIntRect>& clipDeferredToParent)

Rename to aClipDeferredToParent?

I

Comment 50

[Botond Ballo [:botond]]

Comment on attachment 8689151 [details]
MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself. r=kats,mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25559/diff/3-4/

Comment 51

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #49)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:750
> (Diff revision 3)
> > +                                                          Maybe<ParentLayerIntRect>& clipDeferredToParent)
> 
> Rename to aClipDeferredToParent?

Fixed.

Comment 52

[Botond Ballo [:botond]]

Matt, I'll leave it up to you to land the combined patch series.

Comment 53

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add flags to GetResultingTransformMatrix

This ended up being a real pain to get it to pass test (I think I've got it now).

I've split the patches up further, and asking for review again since there was a fair bit of change.

Comment 54

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Simplify GetResultingTransformMatrix calculations

The existing code really confused me, took a while to figure out how it actually worked.

For future reference:

The old code for when we had a transform with perspective and wanted the final transform offset to the reference frame was approximately:

result = ReadTransforms()
result.ChangeBasis(frameToReferenceFrame + frameToTransformOrigin)

perspective = ComputePerspective()
perspective.ChangeBasis(frameToReferenceFrame + frameToPerspectiveOrigin)

result = result * perspective

result.PreTranslate(frameToReferenceFrame)

This seems really confusing to me, since both ChangeBasis calls seem to be adding two values that both offset from frame local space and it's clear how adding them together makes sense. PreTranslating to the reference frame when we've asked for it to be translated *after* the transform is applied is also weird.

If we combine all these together into a list of matrices to multiply together we get:

frameToReferenceFrame * -frameToReferenceFrame * -frameToTransformOrigin * transformList * frameToReferenceFrame * frameToTransformOrigin * -frameToReferenceFrame * -frameToPerspectiveOrigin * perspective * frameToReferenceFrame * frameToPerspectiveOrigin

Knowing that we can reorder transforms that are only translations (everything except transformList and perspective), and that M * -M simplifies to nothing, we can simplify this to:

-frameToTransformOrigin * transformList * frameToTransformOrigin * -frameToPerspectiveOrigin * perspective * frameToPerspectiveOrigin * frameToReferenceFrame

This makes a lot more sense, and matches what the spec asks us to do, with the addition of the last two operations which move us from coordinates in the perspective frame's coordinate space into the reference frame's coordinate space. Note that we translate back to the transform frame's coordinate space, and then to the reference frame's coordinate frame in separate steps where we could just directly translate from the perspective frame to the reference frame. The current way is simpler with our code, especially since we snap the translation to the reference frame to pixels.

This patch changes us to more directly output the simplified transform list, and should be much easier to follow.

Comment 55

[Matt Woodrow (:mattwoodrow)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #54)
> 
> This seems really confusing to me, since both ChangeBasis calls seem to be
> adding two values that both offset from frame local space and it's clear how
> adding them together makes sense.

It's *NOT* clear.

Comment 56

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Add nsDisplayPerspective v4

Comment 57

[Matt Woodrow (:mattwoodrow)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #56)
> Created attachment 8692250 [details] [diff] [review]
> Part 4: Add nsDisplayPerspective v4

A (probably not exhaustive) list of changes:

* nsDisplayPerspective's mFrame is now the perspective frame, not the transform frame
* Fixed ItemParticipatesIn3DContext code to recognize nsDisplayPerspective and not insert separator transforms
* Made nsDisplayTransform::GetTransformForRendering understand separator transforms.
* Fixed AsyncCompositionManager/LayerTransactionParent to understand perspective transform, and only correct for/add the origin offset if necessary.
* Fixed nsDisplayPerspective::BuildLayer w.r.t part 3 of this patch set so that it correctly implements the last 4 steps of the simplified computation.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8692250 [details] [diff] [review]
Part 4: Add nsDisplayPerspective v4

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

Heroic!

::: layout/base/nsDisplayList.cpp
@@ +5980,5 @@
> +  }
> +
> +  // Sort of a lie, but we want to pretend that the perspective layer extends a 3d context
> +  // so that it gets its transform combined with children. Might need a better name that reflects
> +  // this use case and isn't specific to preserve-3d.

A separate flag would probably be better.

Comment 59

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #58)
 
> A separate flag would probably be better.

I decided I'd rather just use the existing one, since all the places that check the flag want the same behaviour (as far as I can tell). I don't think there's much point in having two flags and checking for either at all callsites.

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3970cb487d1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1496f9413788
https://hg.mozilla.org/integration/mozilla-inbound/rev/f225e5b0663e
https://hg.mozilla.org/integration/mozilla-inbound/rev/47c793c9547b
https://hg.mozilla.org/integration/mozilla-inbound/rev/afd640a20e2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4447e71d7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/885889d182fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e18014be68d

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3970cb487d1f
https://hg.mozilla.org/mozilla-central/rev/1496f9413788
https://hg.mozilla.org/mozilla-central/rev/f225e5b0663e
https://hg.mozilla.org/mozilla-central/rev/47c793c9547b
https://hg.mozilla.org/mozilla-central/rev/afd640a20e2c
https://hg.mozilla.org/mozilla-central/rev/7a4447e71d7a
https://hg.mozilla.org/mozilla-central/rev/885889d182fd
https://hg.mozilla.org/mozilla-central/rev/7e18014be68d

Comment 62

[Markus Stange [:mstange]]

No tests? :(

Comment 63

[Thinker Li [:sinker]]

Comment on attachment 8692247 [details] [diff] [review]
Part 3: Simplify GetResultingTransformMatrix calculations

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

::: layout/base/nsDisplayList.cpp
@@ +5207,2 @@
>      } else {
> +      result.ChangeBasis(refBoxOffset);

Matt, I am working on bug 1232060.  I found this change make hittest() shifting the hit area corresponding to the offset to the reference frame.  Is there any reason we need to change this?

Comment 64

[Matt Woodrow (:mattwoodrow)]

I have replied in bug 1232060.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d

Comment 66

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified the attached testcase and the samples from comment 1 using Firefox 46 beta 10 under Win 7 64-bit and Mac OS X 10.11.
Also went through the dependencies and didn't found any issues. Marking as verified.