728758 - SVG SMIL: Removing a stacked animation fails to update

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Reftest to reproduce problem

Thanks to Cameron for reporting this--there's a bug in the way we detect changes within the compositor stack.

If we have two animations stacked on top of one another (i.e. applying to the same target attribute) and we remove the top one then sometimes we fail to detect the change and hence we continue to apply the top effect even after the corresponding animation element is no longer in the tree.

In the attached test case the operation looks something like this:
1. Run the green animation
    (nsSMILAnimationFunction::UpdateCachedTarget stores the target)
2. Run the red animation
3. Remove red animation
4. On the next sample we'll think nothing has changed because
- the green animation function hasn't changed
- when we update its target it's the same as in step 1 so we don't detect any change there
- the base value hasn't changed

Note that the second point there about the target of the animation function is important. If the two animations in question start at the same time (or sufficiently close that there's no sample in between) we'll be ok since the animation on the bottom won't get a chance to cache the target value (it will get filtered out because the top animation overwrites its value). Then, when the top animation is removed the bottom animation will have no target value and UpdateCachedTarget will detect a change, return true, and we'll composite.


Possible remedies:

We already pass state between prev and next compositors to detect changes to the base value that should force a recomposite (specifically this is nsSMILCompositor::StealCachedBaseValue). We could just extend this to include the number of animation functions in effect. Any change in number should force a recomposite. Or, perhaps better still, store and compare the index returned by GetFirstFuncToAffectSandwich?

I wonder if there's any situation where that wouldn't work--i.e. where you could remove one function, add another and not trigger any of the other conditions that force recomposing in the process.

Another approach might be, for example, in nsSMILCompositor::GetFirstFuncToAffectSandwich, go through the unapplied animation functions and remove their cached targets. That way, if they later become active, they'll return true from UpdateCachedTarget. That might be simpler and I don't know if it leads to a lot more busy work since we're already going to the trouble of adding such functions to the compositor anyway. Also, we could skip doing it altogether if mForceCompositing is already true.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a

Patch to implement the second remedy outlined in the description.

Comment 2

[Daniel Holbert [:dholbert]]

Hmm... It feels a bit hacky to pretend that the target has changed.  I'd rather we handle this explicitly, rather than overloading an existing piece of non-trivial state.

Could we just add a boolean flag called e.g. "mWasReplacedInPrevSample" to nsSMILAnimationFunction (with getter/setter), documented to say "In the previous sample, our value was replaced / ignored by another animation that stacked on top of us in the animation sandwich."

Then we could just check that bit alongside HasChanged() here, in GetFirstFuncToAffectSandwich():
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCompositor.cpp?mark=177-177#171

That way even if HasChanged() is false, we'd still turn on mForceCompositing if WasReplacedInPrevSample() returned true for a given animation function that's going to affect our sandwich.

(And we'd set that bit where your existing patch adds a call to "ClearCachedTarget()", I think)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Then we could just check that bit alongside HasChanged() here, in
> GetFirstFuncToAffectSandwich():
> http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCompositor.
> cpp?mark=177-177#171

(NOTE: I'm pretty sure we *won't* need to modify the *other* place where we call HasChanged(), here:
 http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationController.cpp#748
In that chunk, making sure we detect animations that have _just_ changed from active --> inactive, so that we can sample the now-inactive state.  For the purposes of that check, it doesn't really matter whether the animation was overridden in the previous sample.  (If anything, we care less about it if it was overridden in the previous sample.))

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1b

Updated patch. I'm not completely convinced that this approach is less hacky but I don't mind overly.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 599499 [details] [diff] [review]
Patch v1b

>+    // In the following, the 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();


In the comment, I think you mean to say "the short-circuit behavior of ||", not "|="

>+++ b/layout/reftests/svg/smil/anim-remove-9.svg
>@@ -0,0 +1,29 @@
>+<svg xmlns="http://www.w3.org/2000/svg"
[...]
>+  <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>
>+  <script>
>+  </script>
>+</svg>

Nix the empty <script> at the end of the test here.

r=me with that.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #4)
> Updated patch. I'm not completely convinced that this approach is less hacky
> but I don't mind overly.

(Thanks for updating it -- this way seems better/cleaner to me, at least. :))

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Comment on attachment 599499 [details] [diff] [review]
> Patch v1b
> 
> >+    // In the following, the 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();
> 
> 
> In the comment, I think you mean to say "the short-circuit behavior of ||",
> not "|="

Thanks for the review!

I'm pretty sure the comment is correct. I think to a lot of people's minds the following are equivalent:

  mForceCompositing = mForceCompositing || testFunc();

and:

  mForceCompositing |= testFunc();

But they're different. In the first case, if mForceCompositing is true, testFunc won't get run but in the second case it will.

That's what I was trying to explain (hence the "even if mForceCompositing is true").

Maybe I should drop the "short-circuit" word to avoid confusion?

Comment 8

[Daniel Holbert [:dholbert]]

Ah, I see -- I thougth you were referring to the fact that the first entry in the 
  a() || b() || c()
would always get evaluated -- but you are indeed talking about the |= in the case where the lvalue is already truthy.

So -- yeah, I'd drop the "short-circuiting" term, since you're in fact saying that _no_ short circuiting will happen, IIUC. :) (or you could refer to something like "the lack of short-circuiting in |=")

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f972541b274

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/7f972541b274