<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 2

•

11 years ago

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.

Comment 3

•

11 years ago

Attached patch wip.diff (obsolete) — Details — Splinter Review

Stop to call WrapPreserve3DList(). Create a transform item for each frame with transform and preserves3d.

David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)

Updated

•

11 years ago

Depends on: 1097464

Comment 4

•

10 years ago

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)

Comment 5

•

10 years ago

That's the only place I can think of, but I haven't looked in detail.

David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)

Comment 6

•

10 years ago

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)

Comment 7

•

10 years ago

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)

Comment 8

•

10 years ago

I have done something at bug 1208646. I will move it to this bug.

Flags: needinfo?(tlee)

Updated

•

10 years ago

Assignee: nobody → tlee

Comment 9

•

9 years ago

Attached patch Do animations of transforms with preserve-3d at compositor (obsolete) — Details — Splinter Review

Markus Stange [:mstange]

Comment 10

•

9 years ago

What's the status of this?

Comment 11

•

9 years ago

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.

Thomas Wisniewski

Comment 12

•

8 years ago

sinker, was this still blocked on any reftest harness issues?

Markus Stange [:mstange]

Comment 13

•

8 years ago

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)

Comment 14

•

8 years ago

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

Comment 15

•

8 years ago

(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)

Comment 16

•

8 years ago

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.

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Comment 17

•

8 years ago

(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.

Updated

•

8 years ago

Blocks: 1461311

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Comment 18

•

7 years ago

A new try with rebased patches. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6675139ee939e9a6613e69f84597dacabe8359d

Comment hidden (mozreview-request)

Comment 23

•

7 years ago

mozreview-review

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 24

•

7 years ago

mozreview-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 25

•

7 years ago

mozreview-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 26

•

7 years ago

mozreview-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.

Comment 27

•

7 years ago

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.

Comment 28

•

7 years ago

(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.

Comment 29

•

7 years ago

(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()?

Comment 30

•

7 years ago

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

Comment 31

•

7 years ago

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 hidden (mozreview-request)

Comment 36

•

7 years ago

mozreview-review

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+