Animate CSS Transitions on the compositor

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

5 years ago
I will fall back to the current code if the transition is cancelled.
(Assignee)

Updated

5 years ago
Assignee: dzbarsky → nobody
Component: Graphics: Layers → Layout
QA Contact: graphics-layers → layout
(Assignee)

Comment 1

5 years ago
Created attachment 637175 [details] [diff] [review]
Patch

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

Comment 3

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

Comment 5

5 years ago
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]
Patch

>+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]
Patch

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

Comment 8

5 years ago
Created attachment 637474 [details] [diff] [review]
Patch

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

Comment 9

5 years ago
Created attachment 640403 [details] [diff] [review]
Patch
Attachment #637474 - Attachment is obsolete: true
Attachment #637474 - Flags: feedback?(dbaron)
Attachment #640403 - Flags: review?(dbaron)
(Assignee)

Comment 10

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

Updated

5 years ago
Attachment #640403 - Flags: review?(roc)
(Assignee)

Comment 11

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

Comment 13

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

Comment 15

5 years ago
Created attachment 640955 [details] [diff] [review]
Patch

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

Updated

5 years ago
Attachment #640955 - Flags: review?(dbaron)
Comment on attachment 640955 [details] [diff] [review]
Patch

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

Comment 17

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

Comment 19

5 years ago
HasOpacity and IsTransformed are in this patch.  I added the check to the next iteration.
(Assignee)

Comment 20

5 years ago
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.
Attachment #640955 - Attachment is obsolete: true
Attachment #640955 - Flags: review?(roc)
Attachment #640955 - Flags: review?(dbaron)
(Assignee)

Comment 21

5 years ago
Created attachment 642765 [details] [diff] [review]
Fixed patch

Fixed the test failure.  This is now green on try.
Attachment #642709 - Attachment is obsolete: true
Attachment #642765 - Flags: review?(dbaron)
(Assignee)

Updated

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

Comment 23

5 years ago
Created attachment 643012 [details] [diff] [review]
Transitions patch
Attachment #642765 - Attachment is obsolete: true
Attachment #642765 - Flags: review?(dbaron)
Attachment #643012 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 25

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

Comment 27

5 years ago
Created attachment 644840 [details] [diff] [review]
Transitions patch

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

Comment 28

5 years ago
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.
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.

nsTransitionManager.h:


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

Comment 31

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5078933d6954
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0260c45de9
Target Milestone: --- → mozilla17
Sorry, I backed this out (along with two other bugs from the same push) because of reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/442e36401b38

https://tbpl.mozilla.org/php/getParsedLog.php?id=13791557&tree=Mozilla-Inbound
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 → ---
(Assignee)

Comment 33

5 years ago
Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/697f5b87eae9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8548e016d4a9
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla17
Push backed out for xul android R3 failures (tried asking on IRC which specific csets should be backed out, but presume you were afk):
https://tbpl.mozilla.org/php/getParsedLog.php?id=13839408&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13834332&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13837514&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad3878167c1
Target Milestone: mozilla17 → ---
(Assignee)

Comment 35

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/39550ed860e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2194a2dd66b2
https://hg.mozilla.org/mozilla-central/rev/39550ed860e6
https://hg.mozilla.org/mozilla-central/rev/2194a2dd66b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Backed out:
http://hg.mozilla.org/mozilla-central/rev/9d2a7a8ca1c7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 39

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/948b8629c7bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8652f9d5ec7d
https://hg.mozilla.org/mozilla-central/rev/948b8629c7bb
https://hg.mozilla.org/mozilla-central/rev/8652f9d5ec7d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.