Bug 728758 - Detect animations removed from top of compositor stack and recomposite; r=dholbert
authorBrian Birtles <birtles@gmail.com>
Fri, 24 Feb 2012 09:45:40 +0900
changeset 87595 7f972541b27412a93507b9789c2c5c2e0f9666b5
parent 87594 16283b69a10ac9f11647e75c18dc790a9bc4b969
child 87596 7d5d8b8121d137e85a9460f45d1da4b9b982660b
push id22133
push usermak77@bonardo.net
push dateFri, 24 Feb 2012 10:23:30 +0000
treeherdermozilla-central@fbcdc2c87df8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs728758
milestone13.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 728758 - Detect animations removed from top of compositor stack and recomposite; r=dholbert
content/smil/nsSMILAnimationFunction.cpp
content/smil/nsSMILAnimationFunction.h
content/smil/nsSMILCompositor.cpp
content/smil/nsSMILCompositor.h
layout/reftests/svg/smil/anim-remove-9.svg
layout/reftests/svg/smil/reftest.list
--- a/content/smil/nsSMILAnimationFunction.cpp
+++ b/content/smil/nsSMILAnimationFunction.cpp
@@ -89,17 +89,18 @@ nsSMILAnimationFunction::nsSMILAnimation
     mBeginTime(LL_MININT),
     mAnimationElement(nsnull),
     mErrorFlags(0),
     mIsActive(false),
     mIsFrozen(false),
     mLastValue(false),
     mHasChanged(true),
     mValueNeedsReparsingEverySample(false),
-    mPrevSampleWasSingleValueAnimation(false)
+    mPrevSampleWasSingleValueAnimation(false),
+    mWasSkippedInPrevSample(false)
 {
 }
 
 void
 nsSMILAnimationFunction::SetAnimationElement(
     nsISMILAnimationElement* aAnimationElement)
 {
   mAnimationElement = aAnimationElement;
@@ -228,16 +229,17 @@ nsSMILAnimationFunction::Inactivate(bool
 }
 
 void
 nsSMILAnimationFunction::ComposeResult(const nsISMILAttr& aSMILAttr,
                                        nsSMILValue& aResult)
 {
   mHasChanged = false;
   mPrevSampleWasSingleValueAnimation = false;
+  mWasSkippedInPrevSample = false;
 
   // Skip animations that are inactive or in error
   if (!IsActiveOrFrozen() || mErrorFlags != 0)
     return;
 
   // Get the animation values
   nsSMILValueArray values;
   nsresult rv = GetValues(aSMILAttr, values);
--- a/content/smil/nsSMILAnimationFunction.h
+++ b/content/smil/nsSMILAnimationFunction.h
@@ -242,16 +242,34 @@ public:
    *
    * @param aNewTarget A nsSMILTargetIdentifier representing the animation
    *                   target of this function for this sample.
    * @return  true if |aNewTarget| is different from the old cached value;
    *          otherwise, false.
    */
   bool UpdateCachedTarget(const nsSMILTargetIdentifier& aNewTarget);
 
+  /**
+   * Returns true if this function was skipped in the previous sample (because
+   * there was a higher-priority non-additive animation). If a skipped animation
+   * function is later used, then the animation sandwich must be recomposited.
+   */
+  bool WasSkippedInPrevSample() const {
+    return mWasSkippedInPrevSample;
+  }
+
+  /**
+   * Mark this animation function as having been skipped. By marking the
+   * function as skipped, if it is used in a subsequent sample we'll know to
+   * recomposite the sandwich.
+   */
+  void SetWasSkipped() {
+    mWasSkippedInPrevSample = true;
+  }
+
   // Comparator utility class, used for sorting nsSMILAnimationFunctions
   class Comparator {
     public:
       bool Equals(const nsSMILAnimationFunction* aElem1,
                     const nsSMILAnimationFunction* aElem2) const {
         return (aElem1->CompareTo(aElem2) == 0);
       }
       bool LessThan(const nsSMILAnimationFunction* aElem1,
@@ -461,17 +479,18 @@ protected:
   nsSMILValue                   mFrozenValue;
 
   // Allows us to check whether an animation function has changed target from
   // sample to sample (because if neither target nor animated value have
   // changed, we don't have to do anything).
   nsSMILWeakTargetIdentifier    mLastTarget;
 
   // Boolean flags
-  bool                          mIsActive:1;
-  bool                          mIsFrozen:1;
-  bool                          mLastValue:1;
-  bool                          mHasChanged:1;
-  bool                          mValueNeedsReparsingEverySample:1;
-  bool                          mPrevSampleWasSingleValueAnimation:1;
+  bool mIsActive:1;
+  bool mIsFrozen:1;
+  bool mLastValue:1;
+  bool mHasChanged:1;
+  bool mValueNeedsReparsingEverySample:1;
+  bool mPrevSampleWasSingleValueAnimation:1;
+  bool mWasSkippedInPrevSample:1;
 };
 
 #endif // NS_SMILANIMATIONFUNCTION_H_
--- a/content/smil/nsSMILCompositor.cpp
+++ b/content/smil/nsSMILCompositor.cpp
@@ -168,26 +168,41 @@ nsSMILCompositor::CreateSMILAttr()
 }
 
 PRUint32
 nsSMILCompositor::GetFirstFuncToAffectSandwich()
 {
   PRUint32 i;
   for (i = mAnimationFunctions.Length(); i > 0; --i) {
     nsSMILAnimationFunction* curAnimFunc = mAnimationFunctions[i-1];
-    if (curAnimFunc->UpdateCachedTarget(mKey) ||
-        (!mForceCompositing && curAnimFunc->HasChanged())) {
-      mForceCompositing = true;
-    }
+    // In the following, the lack of short-circuit behavior of |= means that we
+    // will ALWAYS run UpdateCachedTarget (even if mForceCompositing is true)
+    // but only call HasChanged and WasSkippedInPrevSample if necessary.  This
+    // is important since we need UpdateCachedTarget to run in order to detect
+    // changes to the target in subsequent samples.
+    mForceCompositing |=
+      curAnimFunc->UpdateCachedTarget(mKey) ||
+      curAnimFunc->HasChanged() ||
+      curAnimFunc->WasSkippedInPrevSample();
 
     if (curAnimFunc->WillReplace()) {
       --i;
       break;
     }
   }
+  // Mark remaining animation functions as having been skipped so if we later
+  // use them we'll know to force compositing.
+  // Note that we only really need to do this if something has changed
+  // (otherwise we would have set the flag on a previous sample) and if
+  // something has changed mForceCompositing will be true.
+  if (mForceCompositing) {
+    for (PRUint32 j = i; j > 0; --j) {
+      mAnimationFunctions[j-1]->SetWasSkipped();
+    }
+  }
   return i;
 }
 
 void
 nsSMILCompositor::UpdateCachedBaseValue(const nsSMILValue& aBaseValue)
 {
   if (!mCachedBaseValue) {
     // We don't have last sample's base value cached. Assume it's changed.
--- a/content/smil/nsSMILCompositor.h
+++ b/content/smil/nsSMILCompositor.h
@@ -126,14 +126,14 @@ public:
 
   // Member data for detecting when we need to force-recompose
   // ---------------------------------------------------------
   // Flag for tracking whether we need to compose. Initialized to false, but
   // gets flipped to true if we detect that something has changed.
   bool mForceCompositing;
 
   // Cached base value, so we can detect & force-recompose when it changes
-  // from one sample to the next.  (nsSMILAnimationController copies this
+  // from one sample to the next. (nsSMILAnimationController copies this
   // forward from the previous sample's compositor.)
   nsAutoPtr<nsSMILValue> mCachedBaseValue;
 };
 
 #endif // NS_SMILCOMPOSITOR_H_
new file mode 100644
--- /dev/null
+++ b/layout/reftests/svg/smil/anim-remove-9.svg
@@ -0,0 +1,27 @@
+<svg xmlns="http://www.w3.org/2000/svg"
+  class="reftest-wait"
+  onload="go()">
+  <!-- Bug 728758 - Removing a stacked animation fails to update -->
+  <!-- In this test we check that removing an animation applied on top of
+       another animation causes the underlying animation's result to show. -->
+  <script>
+    function go() {
+      document.documentElement.pauseAnimations();
+      // Force a sample after starting the bottom animation, but before starting
+      // the top animation.
+      document.documentElement.setCurrentTime(0.5);
+      // Sample again after the top animation has started
+      document.documentElement.setCurrentTime(1);
+      // Remove top animation
+      var anim = document.getElementById("anim");
+      anim.parentNode.removeChild(anim);
+      // Sample again
+      document.documentElement.setCurrentTime(1);
+      document.documentElement.removeAttribute("class");
+    }
+  </script>
+  <rect x="15" y="15" width="200" height="200" fill="orange">
+    <set attributeName="fill" to="blue"/>
+    <set attributeName="fill" to="red" begin="1s" id="anim"/>
+  </rect>
+</svg>
--- a/layout/reftests/svg/smil/reftest.list
+++ b/layout/reftests/svg/smil/reftest.list
@@ -166,16 +166,17 @@ fuzzy-if(/^Windows\x20NT\x205\.1/.test(h
 == anim-remove-2.svg anim-standard-ref.svg
 == anim-remove-3.svg anim-standard-ref.svg
 == anim-remove-4.svg anim-standard-ref.svg
 == anim-remove-5.svg anim-standard-ref.svg
 == anim-remove-6.svg anim-standard-ref.svg
 == anim-remove-7.svg anim-standard-ref.svg
 == anim-remove-8css.svg anim-standard-ref.svg
 == anim-remove-8xml.svg anim-standard-ref.svg
+== anim-remove-9.svg anim-standard-ref.svg
 == anim-retarget-1.svg anim-standard-ref.svg
 == anim-retarget-2.svg anim-standard-ref.svg
 == anim-retarget-3.svg anim-standard-ref.svg
 == anim-retarget-4.svg anim-standard-ref.svg
 == anim-retarget-5.svg anim-standard-ref.svg
 == anim-retarget-6.svg anim-standard-ref.svg
 == anim-retarget-7.svg anim-standard-ref.svg
 == anim-retarget-8.svg anim-standard-ref.svg