Bug 1240228 - Don't update an effect's timing when tweaking its animation's hold time; r=heycam
authorBrian Birtles <birtles@gmail.com>
Tue, 19 Jan 2016 08:05:08 +0900
changeset 280418 dd8bbf8f4e4abfc5ebc36da3b06594c0a9de1aa9
parent 280417 9f6f26cefebee13339fffd1668d1e318666035da
child 280419 76b4de2ccd6ca11a8121d17558aee62caf943c9a
push id70441
push userbbirtles@mozilla.com
push dateMon, 18 Jan 2016 23:05:44 +0000
treeherdermozilla-inbound@dd8bbf8f4e4a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1240228
milestone46.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1240228 - Don't update an effect's timing when tweaking its animation's hold time; r=heycam In some circumstances when composing style, we tweak the time of the animation before telling the effect to compose style. This is to avoid visual flicker in certain situations where the main thread progress is being synchronized with an animation running on the compositor. In the past, effects would store their latest sample time locally so when tweaking the animation time, we would need to call UpdateEffect() after tweaking the time, and then again after restoring it as otherwise the style composed by the effect would not reflect the adjusted time. Now, however, effect's always query their animation for the time so this is no longer necessary. Furthermore, the actions triggered by UpdateEffect are not desirable in this case because they can, amongst other things, cause the associated EffectSet to be destroyed and recreated. Specifically, Animation::UpdateEffect() calls KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() which: * Calls UpdateTargetRegistration which can trigger EffectSet destruction/creation which is undesirable in this case because we intend to restore whatever changes we make to the Animation's state and deleting and recreating the EffectSet will cause any pointers to it to dangle. * Cause us to possibly reset the "is running on compositor" status. This too is undesirable since we intend to restore the state of the Animation immediately after tweaking the hold time so we don't want to act as if any state has changed. * Similarly for marking the cascade as possibly needing an update or requesting a restyle. In summary, all the actions performed by NotifyAnimationTimingUpdated are unnecessary and undesirable in this situation where we are temporarily tweaking an Animation's current time only to restore it immediately afterwards since the actions are all involved with recognizing actual changes in state.
dom/animation/Animation.cpp
dom/animation/EffectCompositor.cpp
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -791,47 +791,38 @@ Animation::ComposeStyle(RefPtr<AnimValue
   //     that case the compositor might have been ahead of the main thread so we
   //     should use the current wallclock time to ensure the animation doesn't
   //     temporarily jump backwards.
   //
   // To address each of these cases we temporarily tweak the hold time
   // immediately before updating the style rule and then restore it immediately
   // afterwards. This is purely to prevent visual flicker. Other behavior
   // such as dispatching events continues to rely on the regular timeline time.
-  bool updatedHoldTime = false;
   AnimationPlayState playState = PlayState();
   {
     AutoRestore<Nullable<TimeDuration>> restoreHoldTime(mHoldTime);
 
     if (playState == AnimationPlayState::Pending &&
         mHoldTime.IsNull() &&
         !mStartTime.IsNull()) {
       Nullable<TimeDuration> timeToUse = mPendingReadyTime;
       if (timeToUse.IsNull() &&
           mTimeline &&
           mTimeline->TracksWallclockTime()) {
         timeToUse = mTimeline->ToTimelineTime(TimeStamp::Now());
       }
       if (!timeToUse.IsNull()) {
         mHoldTime.SetValue((timeToUse.Value() - mStartTime.Value())
                             .MultDouble(mPlaybackRate));
-        // Push the change down to the effect
-        UpdateEffect();
-        updatedHoldTime = true;
       }
     }
 
     mEffect->ComposeStyle(aStyleRule, aSetProperties);
   }
 
-  // Now that the hold time has been restored, update the effect
-  if (updatedHoldTime) {
-    UpdateEffect();
-  }
-
   MOZ_ASSERT(playState == PlayState(),
              "Play state should not change during the course of compositing");
   mFinishedAtLastComposeStyle = (playState == AnimationPlayState::Finished);
 }
 
 void
 Animation::NotifyEffectTimingUpdated()
 {
--- a/dom/animation/EffectCompositor.cpp
+++ b/dom/animation/EffectCompositor.cpp
@@ -560,16 +560,19 @@ EffectCompositor::ComposeAnimationRule(d
   // As a result, we iterate from last animation to first and, if a
   // property has already been set, we don't change it.
   nsCSSPropertySet properties;
 
   for (KeyframeEffectReadOnly* effect : Reversed(sortedEffectList)) {
     effect->GetAnimation()->ComposeStyle(animationRule, properties);
   }
 
+  MOZ_ASSERT(effects == EffectSet::GetEffectSet(aElement, aPseudoType),
+             "EffectSet should not change while composing style");
+
   effects->UpdateAnimationRuleRefreshTime(aCascadeLevel, aRefreshTime);
 }
 
 /* static */ void
 EffectCompositor::GetOverriddenProperties(nsStyleContext* aStyleContext,
                                           EffectSet& aEffectSet,
                                           nsCSSPropertySet&
                                             aPropertiesOverridden)