Closed Bug 779598 Opened 12 years ago Closed 4 years ago

Consider doing animations of transforms with preserve-3d off the main thread

Categories

(Core :: Graphics: Layers, defect, P3)

Product:
Core
Component:
Graphics: Layers
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla73
Tracking Flags:
Tracking Status
firefox73 --- fixed

People

(Reporter: dzbarsky, Assigned: boris)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [layout:backlog:2020q1])

Attachments

(4 files, 7 obsolete files)

Bug 779598 - Call GetVisualOverflowRectRelativeToSelf() only in the case of NoPrerender.
47 bytes, text/x-phabricator-request
Details | Review
Bug 779598 - Don't try to send transform animations to the compositor if the display item is a transform separator.
47 bytes, text/x-phabricator-request
Details | Review
Bug 779598 - Do animations of transforms with preserve-3d at compositor.
47 bytes, text/x-phabricator-request
Details | Review
Test for preserve-3d on pseudo elements
868 bytes, text/html
Details 
      No description provided.
Had a chat with roc about how to do this.

Currently nsDisplayTransform exports the transition/animation functions and the list of transform functions to the compositor. The simplest approach would be to change this to export a set of these, one for each ancestor in the preserve-3d chain as well as the one for the current frame.

A better approach might be to move preserve-3d handling into the layer system. Currently we flatten preserve-3d transforms together in the display list and only build a single nsDisplayTransform for each leaf frame in the preserve-3d chain. WrapPreserve3D in nsFrame.cpp handles this.

We could instead build nsDisplayTransform/Layers for each element in the preserve-3d chain and make the compositor deal with the remaining problems.

This would avoid having to share the transforms/animations multiple times for shared preserve-3d ancestors, and is probably cleaner overall.

preserve-3d sorting in the compositor currently assumes that all potential children exist as consecutive children, this would need to be updated to handle the new tree structure.
Attached patch wip.diff (obsolete) — — Splinter Review 
Stop to call WrapPreserve3DList().  Create a transform item for each frame with transform and preserves3d.
Depends on: 1097464
What's the status of this now that bug 1097464 is fixed?  (Did attachment 8529068 [details] [diff] [review] land as part of bug 1097464?  It looks like at least significant parts did.)

I know that part of fixing this would be removing this test:
https://hg.mozilla.org/mozilla-central/file/37c7812ce0e6/layout/style/AnimationCommon.cpp#l540
   540     if (frame->Preserves3D() ||
   541         frame->Preserves3DChildren()) {
   542       if (shouldLog) {
   543         nsCString message;
   544         message.AppendLiteral("Gecko bug: Async animation of 'preserve-3d' transforms is not supported.  See bug 779598");
   545         LogAsyncAnimationFailure(message, aElement);
   546       }
   547       return false;
   548     }

Is that all that should be needed now?
Flags: needinfo?(tlee)
That's the only place I can think of, but I haven't looked in detail.
Comment on attachment 8529068 [details] [diff] [review]
wip.diff

Bug 1097474 has already finished what attachment 8529068 [details] [diff] [review] intents to do.
Is there any one trying to work on this bugs?  If no, I would find some one to do this bug.
Attachment #8529068 - Attachment is obsolete: true
Flags: needinfo?(tlee)
I don't know of anyone working on this.  It might just be removing the above test (and then checking that things work!).
Flags: needinfo?(tlee)
I have done something at bug 1208646.  I will move it to this bug.
Flags: needinfo?(tlee)
Assignee: nobody → tlee
What's the status of this?
IIRC, this bug was once stuck by bug 1248828 to extend reftest harness.  But, I heard some others had fixed the problem for WebRenderer.  I will check it, to see if I can move forward.
sinker, was this still blocked on any reftest harness issues?
I think we should land the change even before we set up test infrastructure for it. All the other off-main-thread animations already have the problem that we can't test compositor-side invalidation for them; enabling preserve3d OMTA doesn't make that problem worse.

Matt, do you want to pick this bug up?
Flags: needinfo?(matt.woodrow)
Unfortunately, preserve-3d transform animation does not work as expected.

If an element has transform animations and preserve-3d transform-style, then the animations don't run on the compositor since
mAllowAsyncAnimation for wrapped nsDisplayTransform item is set to false in nsIFrame::BuildDisplayListForStackingContext
[1].

And as for backface-visibility:hidden, there is a pre-existing test case, layout/reftests/transform-3d/animate-backface-hidden.html, and the test doesn't pass on WebRender unfortunately.

[1] https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#3290-3307
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Unfortunately, preserve-3d transform animation does not work as expected.
> 
> If an element has transform animations and preserve-3d transform-style, then
> the animations don't run on the compositor since
> mAllowAsyncAnimation for wrapped nsDisplayTransform item is set to false in
> nsIFrame::BuildDisplayListForStackingContext
> [1].

These wrapper items are just to separate content that doesn't participate in the 3d context. mAllowAsyncAnimation is expected to be false for them, but we also shouldn't have the actual animated transform on them either.

I'd expect mAllowAsyncAnimation to be true for the 'real' transform items.

> 
> And as for backface-visibility:hidden, there is a pre-existing test case,
> layout/reftests/transform-3d/animate-backface-hidden.html, and the test
> doesn't pass on WebRender unfortunately.
> 
> [1]
> https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#3290-
> 3307
Flags: needinfo?(matt.woodrow)
Thank you mattwoodrow for clarification!  So there is a problem that we call AddAnimationsAndTransitionsToLayer() for the wrapper item, thus we set nsIFrame::RefusedAsyncAnimationProperty property for the target frame in the function because the wrapper item's mAllowAsyncAnimation is false, after that we can't send the transform animation.

So, mattwoodrow told me that we can distinguish wrapper items by using IsTransformSeparator(), it works fine;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16ef1e4c6196810e17765258c3c271c05d4cdda8

animate-backface-hidden.html failures on the try are expected as I mentioned in comment 14, but still backface-visibility:hidden does not yet work on the compositor properly, when I open the reftest file directly, a blue box should be disappeared soon, but it actually doesn't, it's updated by clicking content (i.e. need flush).

So, I'd suggest to land preserve-3d case here and defer backface-visibility case to bug 1432961.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> So, I'd suggest to land preserve-3d case here and defer backface-visibility
> case to bug 1432961.

I tend to reopen bug 1186204.
Comment on attachment 8983702 [details]
Bug 779598 - Prerender descendant frames in prerendered preserve-3d frame.

https://reviewboard.mozilla.org/r/249526/#review256010

As long as the tests still pass this is fine. See also https://bugzilla.mozilla.org/show_bug.cgi?id=1461412#c24 I guess.
Attachment #8983702 - Flags: review?(bugmail) → review+
Comment on attachment 8983699 [details]
Bug 779598 - Call GetVisualOverflowRectRelativeToSelf() only in the case of NoPrerender.

https://reviewboard.mozilla.org/r/249520/#review256024
Attachment #8983699 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8983700 [details]
Bug 779598 - Don't try to send transform animations to the compositor if the display item is a transform separator.

https://reviewboard.mozilla.org/r/249522/#review256026

::: commit-message-e2c73:1
(Diff revision 1)
> +Bug 779598 - Don't try to send transform animations to the compositor if the display item isn't transform separator. r?mattwoodrow

If the display item is a transform separator?

::: layout/painting/nsDisplayList.cpp:8659
(Diff revision 1)
>    }
>  
>    // If it looks like we're animated, we should rasterize in local space
>    // (disabling subpixel-aa and global pixel snapping)
> -  bool rasterizeLocally = ActiveLayerTracker::IsStyleMaybeAnimated(
> -    Frame(), eCSSProperty_transform);
> +  bool rasterizeLocally =
> +    mIsTransformSeparator &&

Should this be !mTransformSeparator?

We want to rasterize in local coord space if we're animated, which is the case for non-separators?
Attachment #8983700 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8983701 [details]
Bug 779598 - Do animations of transforms with preserve-3d at compositor.

https://reviewboard.mozilla.org/r/249524/#review256028

::: layout/painting/ActiveLayerTracker.cpp:437
(Diff revision 1)
>      if (CheckScrollInducedActivity(layerActivity, activityIndex, aBuilder)) {
>        return true;
>      }
>    }
> -  if (aProperty == eCSSProperty_transform && aFrame->Combines3DTransformWithAncestors()) {
> +  if (aProperty == eCSSProperty_transform &&
> +      aFrame->In3DContextAndBackfaceIsHidden()) {

I don't really understand this code before or after the change.

Previously it looks like we'd have recursed to the root of the preserve-3d context and checked there, but why would we ignore animations on the current frame?

I think we want to check this frame and the preserve-3d ancestors.

::: layout/painting/nsDisplayList.cpp:8414
(Diff revision 1)
>    if (aDirtyRect->Contains(overflow)) {
>      return FullPrerender;
>    }
>  
> +  if ((aFrame->Extend3DContext() || aFrame->IsPreserve3DLeaf()) &&
> +      nsLayoutUtils::HasEffectiveAnimation(aFrame, eCSSProperty_transform)) {

If we have multiple nested preserve-3d frame, and an animation set on the middle one, does this make sure we prerender everything for the leaves?

It seems like the leaf frame wouldn't hit this path (since the animation isn't on it), and then when we build the leaf items we'd take this path:
https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2916

Which loads the visible rect from the preserve-3d root, and wouldn't take the prerendering of the intermediate into account.
Actually the patch was originally written by Thinker, so I don't fully understand what the patch does, but I am trying to understand.

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> Comment on attachment 8983701 [details]
> Bug 779598 - Do animations of transforms with preserve-3d at compositor.
> 
> https://reviewboard.mozilla.org/r/249524/#review256028
> 
> ::: layout/painting/ActiveLayerTracker.cpp:437
> (Diff revision 1)
> >      if (CheckScrollInducedActivity(layerActivity, activityIndex, aBuilder)) {
> >        return true;
> >      }
> >    }
> > -  if (aProperty == eCSSProperty_transform && aFrame->Combines3DTransformWithAncestors()) {
> > +  if (aProperty == eCSSProperty_transform &&
> > +      aFrame->In3DContextAndBackfaceIsHidden()) {
> 
> I don't really understand this code before or after the change.
> 
> Previously it looks like we'd have recursed to the root of the preserve-3d
> context and checked there, but why would we ignore animations on the current
> frame?
> 
> I think we want to check this frame and the preserve-3d ancestors.

Hmm after some more reading the code, I start thinking we don't necessary modify IsStyleAnimated() at all.
In the case of |this| frame is preserve-3d, we check the frame by HasEffectiveAnimation() or MayHaveEffectiveAnimation() just below this |if| block. 

> ::: layout/painting/nsDisplayList.cpp:8414
> (Diff revision 1)
> >    if (aDirtyRect->Contains(overflow)) {
> >      return FullPrerender;
> >    }
> >  
> > +  if ((aFrame->Extend3DContext() || aFrame->IsPreserve3DLeaf()) &&
> > +      nsLayoutUtils::HasEffectiveAnimation(aFrame, eCSSProperty_transform)) {
> 
> If we have multiple nested preserve-3d frame, and an animation set on the
> middle one, does this make sure we prerender everything for the leaves?
> 
> It seems like the leaf frame wouldn't hit this path (since the animation
> isn't on it),

Oh I see.  

> and then when we build the leaf items we'd take this path:
> https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2916
> 
> Which loads the visible rect from the preserve-3d root, and wouldn't take
> the prerendering of the intermediate into account.

I don't quite understand this part, but in a test case which has a transform animation in the middle of preserve-3d frame (and a descendant is larger than the parent) I could confirm that the check doesn't work well.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> 
> > and then when we build the leaf items we'd take this path:
> > https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2916
> > 
> > Which loads the visible rect from the preserve-3d root, and wouldn't take
> > the prerendering of the intermediate into account.
> 
> I don't quite understand this part, but in a test case which has a transform
> animation in the middle of preserve-3d frame (and a descendant is larger
> than the parent) I could confirm that the check doesn't work well.

The overflow areas of some frames in preserve-3d isn't computed correctly (and can't be, because they're 3d boxes, not 2d rectangles), so this code is working around that.

The pre-transform overflow area of frames within preserve-3d includes the union of all children that aren't participating in the preserve-3d context. For leaves this is all the children, for intermediates (and the root), it's some subset.

The post-transform overflow area for the root is correct, and includes the whole preserve-3d overflow. All other post-transform overflow areas are not valid.

Display list building normally converts the building rectangle into local coordinate space, and then checks if it intersects with the overflow rect for that frame. Since the intermediate frames don't have useful overflow areas, this doesn't work. The transform from root to intermediate might also be singular (in 2d, for example rotateX(90deg)), even though the transform from root to leaf is not. This would give us an empty building rect, even though we still want to build descendants.

Instead, whenever we get to the root of a preserve-3d context, we cache the current building rect. For each descendant in the preserve-3d context we retrieve the cached value, and transform it to the local coord space in a single step, so that we'd convert from root -> leaf directly and avoid the empty rectangle.

I think if we decide to prerender a frame within preserve-3d, we need to set a flag so that all descendants also prerender even if they're not animated.
(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> Comment on attachment 8983701 [details]
> Bug 779598 - Do animations of transforms with preserve-3d at compositor.
> 
> https://reviewboard.mozilla.org/r/249524/#review256028
> 
> ::: layout/painting/ActiveLayerTracker.cpp:437
> (Diff revision 1)
> >      if (CheckScrollInducedActivity(layerActivity, activityIndex, aBuilder)) {
> >        return true;
> >      }
> >    }
> > -  if (aProperty == eCSSProperty_transform && aFrame->Combines3DTransformWithAncestors()) {
> > +  if (aProperty == eCSSProperty_transform &&
> > +      aFrame->In3DContextAndBackfaceIsHidden()) {
> 
> I don't really understand this code before or after the change.
> 
> Previously it looks like we'd have recursed to the root of the preserve-3d
> context and checked there, but why would we ignore animations on the current
> frame?
> 
> I think we want to check this frame and the preserve-3d ancestors.

After some more thought about this.  I think mattwoodrow is right.  The original code attempts to participate the frame which has no transform animation but its parent has transform animations into 3D context in the preserve-3d ancestor.  But this attempt will disturb if the frame itself has transform animations.  I think what we should do there is to move the |if (aProperty == eCSSProperty_transform && aFrame->Combines3DTransformWithAncestors())| after nsLayoutUtils::{MayHave,Has}EffectiveAnimation() calls.

And,

(In reply to Matt Woodrow (:mattwoodrow) from comment #28)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> > 
> > > and then when we build the leaf items we'd take this path:
> > > https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2916
> > > 
> > > Which loads the visible rect from the preserve-3d root, and wouldn't take
> > > the prerendering of the intermediate into account.
> > 
> > I don't quite understand this part, but in a test case which has a transform
> > animation in the middle of preserve-3d frame (and a descendant is larger
> > than the parent) I could confirm that the check doesn't work well.
> 
> The overflow areas of some frames in preserve-3d isn't computed correctly
> (and can't be, because they're 3d boxes, not 2d rectangles), so this code is
> working around that.
> 
> The pre-transform overflow area of frames within preserve-3d includes the
> union of all children that aren't participating in the preserve-3d context.
> For leaves this is all the children, for intermediates (and the root), it's
> some subset.
> 
> The post-transform overflow area for the root is correct, and includes the
> whole preserve-3d overflow. All other post-transform overflow areas are not
> valid.
> 
> Display list building normally converts the building rectangle into local
> coordinate space, and then checks if it intersects with the overflow rect
> for that frame. Since the intermediate frames don't have useful overflow
> areas, this doesn't work. The transform from root to intermediate might also
> be singular (in 2d, for example rotateX(90deg)), even though the transform
> from root to leaf is not. This would give us an empty building rect, even
> though we still want to build descendants.
> 
> Instead, whenever we get to the root of a preserve-3d context, we cache the
> current building rect. For each descendant in the preserve-3d context we
> retrieve the cached value, and transform it to the local coord space in a
> single step, so that we'd convert from root -> leaf directly and avoid the
> empty rectangle.
> 
> I think if we decide to prerender a frame within preserve-3d, we need to set
> a flag so that all descendants also prerender even if they're not animated.

Once we introduce the flag and we probably can use the flag in ActiveLayerTracker::IsStyleAnimated()?
Gah, I thought we can check the situation without introducing the new flag to check nsIFrame::DisplayItems() something like this [1] but DisplayItems is available only on retained display list?

[1] https://hg.mozilla.org/try/rev/10534af91a31c4241d89721ca3a752f3d457c710
I noticed that nsIFrame::DisplayItemData() for the parent frame can be used for checking the situation (i.e. the given frame is a child of prerendered preserve-3d context).
Comment on attachment 8983702 [details]
Bug 779598 - Prerender descendant frames in prerendered preserve-3d frame.

https://reviewboard.mozilla.org/r/249526/#review267164

::: layout/generic/nsFrame.cpp:1661
(Diff revision 2)
> +
> +  nsIFrame* parent = GetInFlowParent();
> +  MOZ_ASSERT(parent && parent->Extend3DContext());
> +
> +  DisplayItemDataArray& array = parent->DisplayItemData();
> +  for (auto data: array) {

const auto\* might be easier here, also needs whitespace after 'data'.
Attachment #8983702 - Flags: review?(matt.woodrow) → review+
Attachment #8983701 - Flags: review?(matt.woodrow)
See Also: → 1487412

Unassign Thinker because he may not keep working on this.

Assignee: thinker.li → nobody
Priority: -- → P3
Attachment #8798367 - Attachment is obsolete: true
Attachment #8983700 - Attachment is obsolete: true
Attachment #8983701 - Attachment is obsolete: true
Attachment #8983702 - Attachment is obsolete: true
Attachment #8983699 - Attachment is obsolete: true
Assignee: nobody → boris.chiou

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)

I noticed that nsIFrame::DisplayItemData() for the parent frame can be used
for checking the situation (i.e. the given frame is a child of prerendered
preserve-3d context).

I'm looking at this patch, and it seems we cannot use nsIFrame::DisplayitemData() of the parent frame. In BuildDisplayListForStackingContext(), We go through the children of the current transformed frame and build display items for the children first (i.e. deep-first), and then build the nsDisplayTransform for the current frame. In other words, we haven't built the parent transform display item yet when calling ShouldPrerenderTransformedContent(), IIRC. Perhaps we still need an extra flag for this.

Attachment #9115025 - Attachment is obsolete: true

Just notice using FullPrerender on pseudo elements for preserve-3d is not enough. We still need to get the correct visible rect and the dirty rect for the pseudo elements. Perhaps we can just apply PartialPrerender for them.

Whiteboard: [layout:backlog:perf]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b3c8573068f5eef9c7586b23aabf1d3d9e41fe9

Looks good now. Thanks for the review and I will land these patches soon.

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c40945f576bc
Call GetVisualOverflowRectRelativeToSelf() only in the case of NoPrerender. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1b0f56b9164e
Don't try to send transform animations to the compositor if the display item is a transform separator. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/50d6aca7fe69
Do animations of transforms with preserve-3d at compositor. r=mattwoodrow

https://hg.mozilla.org/mozilla-central/rev/c40945f576bc
https://hg.mozilla.org/mozilla-central/rev/1b0f56b9164e
https://hg.mozilla.org/mozilla-central/rev/50d6aca7fe69

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox73: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Hooray! Well done Boris!

Whiteboard: [layout:backlog:perf] → [layout:backlog:2020q1]
Regressions: 1621007
Blocks: compositor-animations
Regressions: 1698988
Regressions: 1789053
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: