[APZ] Scrolling 3d-transformed things is whacky

VERIFIED FIXED in Firefox 46

Status

()

Core
Panning and Zooming
VERIFIED FIXED
3 years ago
5 months ago

People

(Reporter: mstange, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox45 disabled, firefox46 verified)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 5 obsolete attachments)

1.55 KB, text/html
Details
6.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
40 bytes, text/x-review-board-request
kats
: review+
Details | Review
40 bytes, text/x-review-board-request
kats
: review+
mstange
: review+
Details | Review
40 bytes, text/x-review-board-request
kats
: review+
Details | Review
20.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
51.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

3 years ago
Whiteboard: [gfx-noted]
(Reporter)

Comment 1

3 years ago
See http://keithclark.co.uk/articles/pure-css-parallax-websites/ for a good explanation and small test cases.
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8638132 [details]
Simplified testcase
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.
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Updated

3 years ago
Depends on: 1097464
(Assignee)

Comment 7

3 years ago
Created attachment 8657333 [details] [diff] [review]
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.
Assignee: nobody → matt.woodrow
(Assignee)

Updated

3 years ago
Depends on: 1202050
(Assignee)

Comment 8

3 years ago
Created attachment 8666234 [details] [diff] [review]
WIP v2

Cleaned up most pieces, still hitting a bunch of APZ asserts, hopefully bug 1202050 will fix that.
Attachment #8657333 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1215412, 1210784
(Assignee)

Comment 9

3 years ago
Created attachment 8674701 [details] [diff] [review]
Part 1: Remove TransformRectOut since it's unused
Attachment #8674701 - Flags: review?(roc)
(Assignee)

Comment 10

3 years ago
Created attachment 8674702 [details] [diff] [review]
Part 2: Add ApplyScrollToChild for APZ
Attachment #8666234 - Attachment is obsolete: true
Attachment #8674702 - Flags: review?(botond)
(Assignee)

Comment 11

3 years ago
Created attachment 8674703 [details] [diff] [review]
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.
Attachment #8674703 - Flags: review?(roc)
(Assignee)

Comment 12

3 years ago
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 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?
Attachment #8674702 - Flags: review?(botond) → review-
(Assignee)

Comment 14

3 years ago
(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 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.
Attachment #8674703 - Flags: review?(roc) → review+

Updated

3 years ago
See Also: → bug 1213716
(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?
Flags: needinfo?(matt.woodrow)

Updated

3 years ago
Duplicate of this bug: 1213716
(Assignee)

Comment 18

3 years ago
(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.
Flags: needinfo?(matt.woodrow)
Created attachment 8682123 [details]
MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow

Bug 1168263 - Proposed Layers API
Created 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

Bug 1168263 - APZ changes
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.
(Assignee)

Comment 22

3 years ago
(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)?
(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.
(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).
Blocks: 1201321
Oops, typo
Blocks: 1201231
No longer blocks: 1201321
(Assignee)

Comment 26

3 years ago
(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.
Flags: needinfo?(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.)
Flags: needinfo?(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 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/
Created 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

Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself
(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!
(Assignee)

Comment 32

3 years ago
Created attachment 8689330 [details] [diff] [review]
Part 3: Add nsDisplayPerspective v2

Yep, all seems to work now!
Attachment #8674702 - Attachment is obsolete: true
Attachment #8674703 - Attachment is obsolete: true
Attachment #8689330 - Flags: review+
Great! I've cleaned up my patches and bit and will post them for review.
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/
Attachment #8682123 - Attachment description: MozReview Request: Bug 1168263 - Proposed Layers API → MozReview Request: Bug 1168263 - Annotate layers with a perspective transform. r=mattwoodrow
Attachment #8682123 - Flags: review?(matt.woodrow)
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/
Attachment #8682124 - Attachment description: MozReview Request: Bug 1168263 - APZ changes → MozReview Request: Bug 1168263 - Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space. r=kats
Attachment #8682124 - Flags: review?(bugmail.mozilla)
Created attachment 8689631 [details]
MozReview Request: Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats

Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats
Attachment #8689631 - Flags: review?(bugmail.mozilla)
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/
Attachment #8689151 - Attachment description: MozReview Request: Bug 1168263 - Propagate the scroll-clip of a descendant of a layer with a perspective transform up to the layer itself → 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
Attachment #8689151 - Flags: review?(bugmail.mozilla)
Attachment #8682124 - Flags: review?(bugmail.mozilla) → review+
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 on attachment 8689631 [details]
MozReview Request: Bug 1168263 - Introduce a helper function IntersectMaybeRects(). r=kats

https://reviewboard.mozilla.org/r/25681/#review23117
Attachment #8689631 - Flags: review?(bugmail.mozilla) → review+
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?
Attachment #8689151 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #8689151 - Flags: review?(mstange)
(Assignee)

Comment 43

3 years ago
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.
Attachment #8682123 - Flags: review?(matt.woodrow)
(Assignee)

Comment 44

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

https://reviewboard.mozilla.org/r/23975/#review23145
Attachment #8682123 - Flags: review+
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 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 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 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/
Attachment #8689151 - Attachment description: 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 → 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
(Reporter)

Comment 49

3 years ago
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
Attachment #8689151 - Flags: review?(mstange) → review+
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/
(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.
Matt, I'll leave it up to you to land the combined patch series.
(Assignee)

Comment 53

3 years ago
Created attachment 8692238 [details] [diff] [review]
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.
Attachment #8689330 - Attachment is obsolete: true
Attachment #8692238 - Flags: review?(roc)
(Assignee)

Comment 54

3 years ago
Created attachment 8692247 [details] [diff] [review]
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.
Attachment #8692247 - Flags: review?(roc)
(Assignee)

Comment 55

3 years ago
(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.
(Assignee)

Comment 56

3 years ago
Created attachment 8692250 [details] [diff] [review]
Part 4: Add nsDisplayPerspective v4
Attachment #8692250 - Flags: review?(roc)
(Assignee)

Comment 57

3 years ago
(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 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.
Attachment #8692250 - Flags: review?(roc) → review+
(Assignee)

Comment 59

3 years ago
(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.
(Reporter)

Comment 62

3 years ago
No tests? :(
Depends on: 1230693
Depends on: 1230780
Depends on: 1230779

Updated

3 years ago
Depends on: 1231242

Updated

3 years ago
Blocks: 1229321
No longer depends on: 1231242

Updated

3 years ago
Depends on: 1234768

Updated

2 years ago
Depends on: 1237952
Depends on: 1237982
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?

Updated

2 years ago
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 64

2 years ago
I have replied in bug 1232060.
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

2 years ago
Depends on: 1238928
No longer depends on: 1237952

Updated

2 years ago
Blocks: 1240783

Updated

2 years ago
Depends on: 1241443
This was backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d
status-firefox41: affected → ---
status-firefox45: fixed → disabled
status-firefox46: --- → fixed

Updated

2 years ago
Depends on: 1242158

Updated

2 years ago
Depends on: 1245450
Flags: qe-verify+
Depends on: 1258313
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.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified

Updated

2 years ago
Depends on: 1269321

Updated

2 years ago
Depends on: 1274495

Updated

2 years ago
Depends on: 1275504

Updated

5 months ago
Depends on: 1424591

Updated

5 months ago
Depends on: 1429373
You need to log in before you can comment on or make changes to this bug.