Closed Bug 768440 Opened 8 years ago Closed 8 years ago

Animate CSS Transitions on the compositor


(Core :: Layout, defect)

Not set





(Reporter: dzbarsky, Assigned: dzbarsky)




(2 files, 7 obsolete files)

I will fall back to the current code if the transition is cancelled.
Assignee: dzbarsky → nobody
Component: Graphics: Layers → Layout
QA Contact: graphics-layers → layout
Attached patch Patch (obsolete) — Splinter 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
Assignee: nobody → dzbarsky
Attachment #637175 - Flags: feedback?(dbaron)
I'd think you should be able to avoid duplicating any of the reversing logic.  Why do you think you need to?
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.
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.
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.
OS: Mac OS X → All
Hardware: x86 → All
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 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.)
Attached patch Patch (obsolete) — Splinter 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)
Attachment #637175 - Attachment is obsolete: true
Attachment #637175 - Flags: feedback?(dbaron)
Attachment #637474 - Flags: feedback?(dbaron)
Attached patch Patch (obsolete) — Splinter Review
Attachment #637474 - Attachment is obsolete: true
Attachment #637474 - Flags: feedback?(dbaron)
Attachment #640403 - Flags: review?(dbaron)
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.
Attachment #640403 - Flags: review?(roc)
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?
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.)
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.
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.
Attached patch Patch (obsolete) — Splinter Review
Now with less code duplication
Attachment #640403 - Attachment is obsolete: true
Attachment #640403 - Flags: review?(roc)
Attachment #640403 - Flags: review?(dbaron)
Attachment #640955 - Flags: review?(roc)
Attachment #640955 - Flags: review?(dbaron)
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()?
(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.
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 :-(
HasOpacity and IsTransformed are in this patch.  I added the check to the next iteration.
Attached patch Patch, not quite complete (obsolete) — Splinter Review
So this is the patch that cause the test failure.  Now to figure out why.
Attachment #640955 - Attachment is obsolete: true
Attachment #640955 - Flags: review?(roc)
Attachment #640955 - Flags: review?(dbaron)
Attached patch Fixed patch (obsolete) — Splinter Review
Fixed the test failure.  This is now green on try.
Attachment #642709 - Attachment is obsolete: true
Attachment #642765 - Flags: review?(dbaron)
Attachment #642765 - Flags: review?(roc)
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.
Attachment #642765 - Flags: review?(roc) → review+
Attached patch Transitions patch (obsolete) — Splinter Review
Attachment #642765 - Attachment is obsolete: true
Attachment #642765 - Flags: review?(dbaron)
Attachment #643012 - Flags: review?(roc)
Attachment #643012 - Flags: review?(dbaron)
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().
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)

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

OK, done.
Comment on attachment 643012 [details] [diff] [review]
Transitions patch

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

OK, r+ with that.
Attachment #643012 - Flags: review?(roc) → review+
Rebased after addressing comments on previous patches, and no longer invalidating frames.
Attachment #643012 - Attachment is obsolete: true
Attachment #643012 - Flags: review?(dbaron)
Attachment #644840 - Flags: review?(dbaron)
Sorry, forgot to post this for review earlier because it was part of the animations patch.
Attachment #645033 - Flags: review?(dbaron)
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.)
Attachment #645033 - Flags: review?(dbaron) → review+
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
Attachment #644840 - Flags: review?(dbaron) → review+
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 (==)
Target Milestone: mozilla17 → ---
Target Milestone: --- → mozilla17
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Backed out:
Resolution: FIXED → ---
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.