Last Comment Bug 768440 - Animate CSS Transitions on the compositor
: Animate CSS Transitions on the compositor
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
: Jet Villegas (:jet)
Depends on: 706179
  Show dependency treegraph
Reported: 2012-06-26 06:38 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-07-31 19:18 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (29.49 KB, patch)
2012-06-27 10:14 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (29.09 KB, patch)
2012-06-28 05:48 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (29.50 KB, patch)
2012-07-09 16:08 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (34.70 KB, patch)
2012-07-10 23:57 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch, not quite complete (35.55 KB, patch)
2012-07-16 13:37 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Fixed patch (39.43 KB, patch)
2012-07-16 15:45 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Splinter Review
Transitions patch (40.61 KB, patch)
2012-07-17 09:59 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Splinter Review
Transitions patch (38.38 KB, patch)
2012-07-22 22:26 PDT, David Zbarsky (:dzbarsky)
dbaron: review+
Details | Diff | Splinter Review
Move elementTransitions to the header (8.79 KB, patch)
2012-07-23 12:58 PDT, David Zbarsky (:dzbarsky)
dbaron: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-06-26 06:38:34 PDT
I will fall back to the current code if the transition is cancelled.
Comment 1 David Zbarsky (:dzbarsky) 2012-06-27 10:14:43 PDT
Created attachment 637175 [details] [diff] [review]

Dbaron, can you take a look at my reversing logic?  I know it's not quite right, but I don't understand the mStartForReverseTest logic enough to see how to fix
Comment 2 David Baron :dbaron: ⌚️UTC-8 2012-06-27 13:40:49 PDT
I'd think you should be able to avoid duplicating any of the reversing logic.  Why do you think you need to?
Comment 3 David Zbarsky (:dzbarsky) 2012-06-27 13:47:41 PDT
So currently the transition code animates by reresolving the style on every refresh tick, so when the transition is reversed, it has a current state to reverse from.  When I animate transitions on the compositor, I don't resolve style on the main thread until the transition finishes, so when it is reversed, nsTransitionManager thinks the "current state" is the start state, and I need to somehow trick it into getting the correct state to set as the start of the reverse transition.
Comment 4 David Baron :dbaron: ⌚️UTC-8 2012-06-27 14:15:48 PDT
mStartForReversingTest and mReversePortion are only relevant for starting a transition, which should only be done on the main thread.  It's not relevant to the code that interpolates a particular step of the transition.

When we start the transition, they influence the calculation of duration and delay, which in turn influence pt.mStartTime and pt.mDuration, which do influence the steps of the transition.  But pt.mDuration and pt.mStartTime should have all the information you need from them that would need to be sent to the compositor thread.
Comment 5 David Zbarsky (:dzbarsky) 2012-06-27 14:22:52 PDT
I know this.  The issue is that when starting a reverse transition, if I set its start to mStartForReversingTest of the old transition, it begins at the right place.  However, reversing the reverse does not work - it goes through the whole transition.
Comment 6 David Baron :dbaron: ⌚️UTC-8 2012-06-27 15:22:21 PDT
Comment on attachment 637175 [details] [diff] [review]

>+AddOpacityTransitions(ElementTransitions* et, Layer* aLayer) {
>+  for (PRUint32 propIdx = 0; propIdx < et->mPropertyTransitions.Length(); propIdx++) {
>+    ElementPropertyTransition* property = &et->mPropertyTransitions[propIdx];
>+    if (property->mProperty != eCSSProperty_opacity) {
>+      continue;
>+    }
>+    if (property->IsRemovedSentinel()) {
>+      continue;
>+    }
>+    InfallibleTArray<AnimationSegment> segments;
>+    segments.AppendElement(AnimationSegment(Opacity(property->mStartForReversingTest.GetFloatValue()),
>+                                            Opacity(property->mEndValue.GetFloatValue()),
>+                                            0,
>+                                            1,
>+                                            ToTimingFunction(property->mTimingFunction)));

For a start, this should look at property->mStartValue rather than property->mStartForReversingTest.  (AddTransformTransitions looks ok in this regard, though.)
Comment 7 David Baron :dbaron: ⌚️UTC-8 2012-06-27 15:34:25 PDT
Comment on attachment 637175 [details] [diff] [review]

>-      pt.mStartForReversingTest = oldPT.mEndValue;
>+      if (haveOMTA) {
>+        printf("\n trying to interpolate to right start\n");
>+        nsStyleAnimation::Interpolate(aProperty, oldPT.mStartForReversingTest,
>+                                      oldPT.mEndValue, valuePortion,
>+                                      pt.mStartForReversingTest);
>+      } else {
>+        pt.mStartForReversingTest = oldPT.mEndValue;
>+      }

This change in ConsiderStartingTransition is also wrong.  It should be fine unmodified.

(I'm also a bit suspicious of some of the other haveOMTA checks in ConsiderStartingTransition, but I haven't looked into them as closely.)
Comment 8 David Zbarsky (:dzbarsky) 2012-06-28 05:48:48 PDT
Created attachment 637474 [details] [diff] [review]

So this undoes all the hacks from the previous patch.  With this patch, we can transition all the way forwards and then all the way back, but reversing in the middle isn't right.

For opacity, the reversal simply doesn't register, which means we aren't hitting the code in 

459     // If the new transition reverses the old one, we'll need to handle         
460     // the timing differently.                                                  
461     if (!oldPT.IsRemovedSentinel() &&                                           
462         oldPT.mStartForReversingTest == pt.mEndValue) {

Going to debug some more and try to figure out what's going on.

The strange thing is, transforms are reverseable - (but there is an occasional segfault, possibly a race)
Comment 9 David Zbarsky (:dzbarsky) 2012-07-09 16:08:55 PDT
Created attachment 640403 [details] [diff] [review]
Comment 10 David Zbarsky (:dzbarsky) 2012-07-09 16:11:47 PDT
The patch looks a little weird because it's on top of my animations repo, which has a patch that moves ElementTransitions to the header.
Comment 11 David Zbarsky (:dzbarsky) 2012-07-09 20:55:45 PDT
I really don't like the split between Add[Transform/Opacity]Animations and Add[Transform/Opacity]Transitions.  The code duplication will get even worse as I implement transform-origin, perspective, and perspective-origin.  Can either of you think of some way to common those up, maybe using templates?
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-09 21:37:07 PDT
I don't understand why we have to treat animations and transitions separately in the first place. From the point of view of callers of nsAnimationManager::GetAnimationsForCompositor, there should just be one set of things to deal with --- let's call them animations. The fact that they're triggered in different ways should not affect FrameLayerBuilder etc. (And in the future we might have other ways of triggering them, e.g. SMIL.)
Comment 13 David Zbarsky (:dzbarsky) 2012-07-09 22:23:22 PDT
Animations and transitions have different representations and different features (animations have iterations and directions, transitions are reverseable).  Perhaps it would be cleaner to rewrite both ElementAnimations and ElementTransitions as one class, and then just use that one class here, but that is beyond the scope of this bug.  We would need to do that anyways if/when we implement the WebAnimations stuff.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-09 23:06:59 PDT
I think unifying ElementAnimations and ElementTransitions should be in scope for this bug, because we're proposing to add a lot of duplicated code and data to work around the split.
Comment 15 David Zbarsky (:dzbarsky) 2012-07-10 23:57:43 PDT
Created attachment 640955 [details] [diff] [review]

Now with less code duplication
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-11 02:18:01 PDT
Comment on attachment 640955 [details] [diff] [review]

Review of attachment 640955 [details] [diff] [review]:

::: layout/generic/nsFrame.cpp
@@ +948,5 @@
>    if (mContent) {
>      ea = nsAnimationManager::GetAnimationsForCompositor(mContent, eCSSProperty_opacity);
> +    et = nsTransitionManager::GetTransitionsForCompositor(mContent, eCSSProperty_opacity);
> +  }
> +  return GetStyleDisplay()->mOpacity < 1.0f || ea || et;

This is better, but I'm still very not-pleased with duplicating code in layout where we use property values for each different source of animation :-(.

::: layout/style/nsTransitionManager.cpp
@@ +114,5 @@
> +  for (PRUint32 i = 0; i < mPropertyTransitions.Length(); ++i) {
> +    ElementPropertyTransition& pt = mPropertyTransitions[i];
> +    if (!pt.mHaveInvalidatedForTransitionStart && !pt.IsRemovedSentinel() &&
> +        TimeStamp::Now() > pt.mStartTime) {
> +   printf("\n will invalidate, transition started\n");

You probably don't want to land this printf

::: layout/style/nsTransitionManager.h
@@ +114,5 @@
>    }
> +  static ElementTransitions* GetTransitions(nsIContent* aContent) {
> +    return static_cast<ElementTransitions*>
> +      (aContent->GetProperty(nsGkAtoms::transitionsProperty));

Can you set the node state bit so we don't have to do this property fetch for every single call to HasOpacity() or IsTransformed()?
Comment 17 David Zbarsky (:dzbarsky) 2012-07-11 15:58:02 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Can you set the node state bit so we don't have to do this property fetch
> for every single call to HasOpacity() or IsTransformed()?

HasOpacity() and IsTranformed() call GetTransitionsForCompositor now.
I added a check of the node state bit to GetTransitionsForCompositor, so it will now be almost free.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-11 19:44:34 PDT
In the next iteration of the patch, you mean?

I'm trying to decide how hard-line to be about the code duplication. It's the usual tradeoff between expedience and technical debt :-(
Comment 19 David Zbarsky (:dzbarsky) 2012-07-11 20:31:08 PDT
HasOpacity and IsTransformed are in this patch.  I added the check to the next iteration.
Comment 20 David Zbarsky (:dzbarsky) 2012-07-16 13:37:29 PDT
Created attachment 642709 [details] [diff] [review]
Patch, not quite complete

So this is the patch that cause the test failure.  Now to figure out why.
Comment 21 David Zbarsky (:dzbarsky) 2012-07-16 15:45:45 PDT
Created attachment 642765 [details] [diff] [review]
Fixed patch

Fixed the test failure.  This is now green on try.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-16 20:16:52 PDT
Comment on attachment 642765 [details] [diff] [review]
Fixed patch

Review of attachment 642765 [details] [diff] [review]:

r+ on the non-style-system parts

::: content/base/public/nsIContent.h
@@ +309,5 @@
>      return mNodeInfo->Equals(aTag, kNameSpaceID_MathML);
>    }
> +
> +  bool HasAnimationsForCompositor(nsCSSProperty aProperty);

Document this method.
Comment 23 David Zbarsky (:dzbarsky) 2012-07-17 09:59:48 PDT
Created attachment 643012 [details] [diff] [review]
Transitions patch
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-17 11:15:16 PDT
Comment on attachment 643012 [details] [diff] [review]
Transitions patch

Review of attachment 643012 [details] [diff] [review]:

::: layout/base/nsDisplayList.cpp
@@ +291,5 @@
>          list = segment->mToValue.GetCSSValueListValue();
>          InfallibleTArray<TransformFunction> toFunctions;
>          AddTransformFunctions(list, frame->GetStyleContext(),
>                                frame->PresContext(), bounds,
>                                scale, toFunctions);

please hoist these calls to GetStyleContext() and PresContext().
Comment 25 David Zbarsky (:dzbarsky) 2012-07-17 11:25:52 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)

> please hoist these calls to GetStyleContext() and PresContext().

OK, done.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-17 12:09:02 PDT
Comment on attachment 643012 [details] [diff] [review]
Transitions patch

Review of attachment 643012 [details] [diff] [review]:

OK, r+ with that.
Comment 27 David Zbarsky (:dzbarsky) 2012-07-22 22:26:16 PDT
Created attachment 644840 [details] [diff] [review]
Transitions patch

Rebased after addressing comments on previous patches, and no longer invalidating frames.
Comment 28 David Zbarsky (:dzbarsky) 2012-07-23 12:58:38 PDT
Created attachment 645033 [details] [diff] [review]
Move elementTransitions to the header

Sorry, forgot to post this for review earlier because it was part of the animations patch.
Comment 29 David Baron :dbaron: ⌚️UTC-8 2012-07-23 14:34:11 PDT
Comment on attachment 645033 [details] [diff] [review]
Move elementTransitions to the header

Please give a useful commit message, and please indent the } of the ElementTransitions constructor correctly (i.e., not at all).

r=dbaron with that

(It would be good if you got in the habit of including the commit message in patches you post for review.)
Comment 30 David Baron :dbaron: ⌚️UTC-8 2012-07-23 15:31:44 PDT
Comment on attachment 644840 [details] [diff] [review]
Transitions patch

>+nsIContent::HasAnimationsForCompositor(nsCSSProperty aProperty)

Maybe this belongs in nsLayoutUtils instead?  It's really not very
content-related, so I'd prefer it being in layout.  Also, please brace
the single-line if body at the start.

I think AddAnimationsForProperty should take the frame as the argument
rather than the content node.  The caller has both (the frame first,
from which it gets the content), and AddAnimationsForProperty only uses
the frame.  (Otherwise it's confusing because a content node need not
have a frame, yet you use the frame without null-checking it.)

>+    InfallibleTArray<AnimationSegment> segments;
>+    if (property->mProperty != aProperty) {
>       continue;
>     }

Switch the order.

>+  if (aLayer->GetAnimations().Length() > 0) {
>+    aLayer->ClearAnimations();
>+  }

It would be simpler to just call ClearAnimations without the test.
And it seems like it ought to be cheaper too.  If it's not, maybe it
should be?

>+  nsIDocument* doc = aContent->GetCurrentDoc();
>+  if (!doc) {
>+    return;
>+  }
>+  nsIPresShell* presShell = doc->GetShell();
>+  if (!presShell) {
>+    return;
>+  }
>+  mozilla::TimeStamp currentTime =
>+    presShell->GetPresContext()->RefreshDriver()->MostRecentRefresh();

Instead of all this, please just do:
+  mozilla::TimeStamp currentTime =
+    frame->PresContext()->RefreshDriver()->MostRecentRefresh();

ElementTransitions::HasStartedTransitionSinceLastTick is unused; please
remove it.  (I didn't review it.)  Same for
mHaveInvalidatedForTransitionStart and the code that sets it.

In ElementPropertyTransition::CanPerformOnCompositor:
>+    aTime > mStartTime && aTime < mStartTime + mDuration;
this is clearer to read with all < or <= signs, i.e., as:
+    mStartTime < aTime && aTime < mStartTime + mDuration;

>-  for (PRUint32 i = 0, i_end = mPropertyTransitions.Length(); i < i_end; ++i) {
>+  for (PRUint32 i = 0; i < mPropertyTransitions.Length(); ++i) {

Probably better to leave this.

In nsTransitionManager::ConsiderStartingTransition:

>+  ElementTransitions* et = nsTransitionManager::GetTransitions(aElement);
>+  if (et) {
>+    haveOMTA = et->CanPerformOnCompositorThread();
>+  }

This should all be conditioned on !aNewStyleContext->GetPseudoType() so
that you don't do something weird when there are transitions on :before
or :after and OMTC transitions on the element itself.

Remove the "bool transitionFinished" that you introduce in
nsTransitionManager::WillRefresh; it's unused.

Also remove the XXXdz comment; I don't think it makes sense there, since
WillRefresh isn't going to be called off the main thread.


>+    if (!transitions)
>+      return nsnull;
>+    bool propertyMatches = transitions->HasTransitionOfProperty(aProperty);
>+    return (propertyMatches && transitions->CanPerformOnCompositorThread()) ?
>+      transitions : nsnull;

Using ?: here is unnecessary.  I'd prefer:

+    if (!transitions ||
+        !transitions->HasTransitionOfProperty(aProperty) ||
+        !transitions->CanPerformOnCompositorThread()) {
+      return nsnull;
+    }
+    return transitions;

(Note {} around single-line if body.)

r=dbaron with that
Comment 32 Matt Brubeck (:mbrubeck) 2012-07-23 20:42:38 PDT
Sorry, I backed this out (along with two other bugs from the same push) because of reftest failures:
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/transform-3d/perspective-origin-4a.html | image comparison (==)
Comment 37 Aaron Train [:aaronmt] 2012-07-30 08:53:01 PDT
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Comment 38 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-30 11:39:08 PDT
Backed out:

Note You need to log in before you can comment on or make changes to this bug.