Closed
Bug 768440
Opened 12 years ago
Closed 12 years ago
Animate CSS Transitions on the compositor
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: dzbarsky, Assigned: dzbarsky)
References
Details
Attachments
(2 files, 7 obsolete files)
38.38 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
8.79 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
I will fall back to the current code if the transition is cancelled.
Assignee | ||
Updated•12 years ago
|
Assignee: dzbarsky → nobody
Component: Graphics: Layers → Layout
QA Contact: graphics-layers → layout
Assignee | ||
Comment 1•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #637474 -
Attachment is obsolete: true
Attachment #637474 -
Flags: feedback?(dbaron)
Attachment #640403 -
Flags: review?(dbaron)
Assignee | ||
Comment 10•12 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•12 years ago
|
Attachment #640403 -
Flags: review?(roc)
Assignee | ||
Comment 11•12 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•12 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•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
HasOpacity and IsTransformed are in this patch. I added the check to the next iteration.
Assignee | ||
Comment 20•12 years ago
|
||
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•12 years ago
|
||
Fixed the test failure. This is now green on try.
Attachment #642709 -
Attachment is obsolete: true
Attachment #642765 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 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•12 years ago
|
||
Attachment #642765 -
Attachment is obsolete: true
Attachment #642765 -
Flags: review?(dbaron)
Attachment #643012 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5078933d6954 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e0260c45de9
Target Milestone: --- → mozilla17
Comment 32•12 years ago
|
||
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•12 years ago
|
||
Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/697f5b87eae9 https://hg.mozilla.org/integration/mozilla-inbound/rev/8548e016d4a9
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → mozilla17
Comment 34•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/39550ed860e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/2194a2dd66b2
Comment 36•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39550ed860e6 https://hg.mozilla.org/mozilla-central/rev/2194a2dd66b2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 37•12 years ago
|
||
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Comment 38•12 years ago
|
||
Backed out: http://hg.mozilla.org/mozilla-central/rev/9d2a7a8ca1c7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 39•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/948b8629c7bb https://hg.mozilla.org/integration/mozilla-inbound/rev/8652f9d5ec7d
Comment 40•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/948b8629c7bb https://hg.mozilla.org/mozilla-central/rev/8652f9d5ec7d
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•