614879 - SVG SMIL: Indefinite to-animation should just set the base value

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

There's an inconsistency in the way we handle to-animation when the simple duration is indefinite. In that case we should basically just stick to the base value, much in the same way as we do for by-animation with indefinite duration. However, instead we jump straight to the to value.

The problem comes about because if we have an indefinite simple duration we just go ahead and set the first value in our list:
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#259

That works fine for all cases except to-animation where the first value in the list is the 'to' value.

See attached test case. In WebKit and Opera the box remains fixed to the left but in our implementation it jumps to the right.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Patch v1a

This patch means we just leave the underlying value untouched for the case of an indefinite to-animation.

Comment 2

[Brian Birtles (:birtles)]

Comment on attachment 493321 [details] [diff] [review]
Patch v1a

Cancelling review. I think if we have a discrete to-animation (or a to-animation that falls back to discrete mode because the property isn't interpolatable) we want to set the value for the entire duration. As Jonathan has mentioned in bug 544855 comment 12 this is really inconsistent but that's SMIL.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch v1b

This patch now detects if we're going to interpolate the property or not and sets the value accordingly.

SMIL doesn't really talk about indefinite to animation, although it does talk about indefinite duration and interpolation here: http://www.w3.org/TR/smil-animation/#InterpolationAndIndefSimpleDur

Basically it says to apply the first value f(0) which is a little ambiguous for to-animation.

My understanding is that indefinite duration behaves like a really really really long duration. So we just stick to whatever value we'd otherwise set at t=0.

For to animation this is:
a) the to-value when calcMode=discrete OR when we fall back to discrete calcMode because the animation values aren't interpolatable
b) the base value otherwise

Comment 4

[Brian Birtles (:birtles)]

Attachment: Patch v1c

Fix bitrot.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 493883 [details] [diff] [review]
Patch v1c

A few notes just on the reftests here:

>+     class="reftest-wait"
>+     onload="setTimeAndSnapshot(2, true)">
>+  <script xlink:href="smil-util.js" type="text/javascript"/>

Looks like all of the reftests here share the three lines above, but none of them need them, AFAIK, since there's no time-dependence at all -- right?  (Every animation begins at time 0 by default, and has an indefinite duration, so there's no need to do any seeking / pausing)

>+++ b/layout/reftests/svg/smil/anim-indefinite-to-1.svg
>+  <!-- Test that an indefinite to-animation just sticks to the base value -->

You should probably add "... for interpolatable attributes." at the end of that comment. (since we have the opposite behavior for *non*-interpolatable attributes, as your second reftest shows.)

>+++ b/layout/reftests/svg/smil/anim-indefinite-to-4.svg
>+  <!-- Not really a to-animation, but check that set animation isn't incorrectly
>+       treated as to-animation when we have an indefinite simply duration. -->
>+  <rect x="15" y="15" width="200" height="100" fill="blue">
>+    <set attributeName="height" to="200"/>

s/simply duration/duration/, I think.

This comment needs a little clarification -- it took me a minute to figure out what this meant (since, for non-interpolatable attributes, <set to> *does* behave exactly like <animate to>).

Maybe replace with something like the following:
>  Check that 'set' animation sets the *to* value, even with interpolatable
>  attributes. This is different from the 'animate' element's to-animation - there, for
>  interpolatable attributes, we'd apply the *base* value (and never change, since the
>  duration is indefinite).  But 'set' always  sets the to value -- never the base value.

Also: It might be worth explicitly specifying |dur="indefinite"| in these tests' animations, just for clarity's sake, but I'll leave that up to you. :)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 493883 [details] [diff] [review]
Patch v1c

>+++ b/content/smil/nsSMILAnimationFunction.cpp
>   if (mSimpleDuration.IsIndefinite() ||
>-      (values.Length() == 1 && TreatSingleValueAsStatic())) {
>-    // Indefinite duration or only one value set: Always set the first value
>+      (values.Length() == 1 && !IsToAnimation())) {
>+
>+    // If we have indefinite to-animation then our behaviour is either:
>+    // a) calcMode = discrete, or property not interpolatable: set to-value
>+    // b) interpolating: leave base value untouched
>+    if (IsToAnimation()) {
>+      // Check if we'd succeed interpolating
>+      if (GetCalcMode() != CALC_DISCRETE &&
>+          !aResult.IsNull() &&
>+          NS_SUCCEEDED(aResult.Interpolate(values[0], 0.0, result)))
>+        return;
>+    }
>+
>+    // Otherwise if we have an indefinite duration or just one value set (e.g.
>+    // 'values="8"') just set the first value.
>     result = values[0];

I don't love the logic structure here -- it's a bit confusing.
(For example, the "!IsToAnimation()" check followed immediately by an "if (IsToAnimation())" -- that strikes me as being jumbled and sub-optimal.)

I think we should:
 (a) Replace this clause entirely with just
     if (values.Length() == 1 && !IsToAnimation()) {
       result = values[0];
     }
  (That handles the <set> and singleton-values-list case, but does *not* handle indefinite duration)

 (b) Handle indefinite duration in InterpolateResult instead of here.  Just add a check for indefinite duration at the beginning of that function, and set mSimpleProgress to 0 in that case. This is just a direct application of your "indefinite duration behaves like a really really really long duration" philosophy from Comment 3, and it lets us commandeer all our existing functionality for making non-interpolable animations fall into discrete mode etc. (rather than needing to add an extra add-hock Interpolate call)

>+++ b/content/smil/nsSMILSetAnimationFunction.h
>+  // Although <set> animation might look like to-animation it doesn't behave
>+  // like it.
>+  NS_OVERRIDE virtual PRBool IsToAnimation() const {
>+    return PR_FALSE;

This comment is a little confusing (particularly "it doesn't behave like it"), because <set> animation *does* actually behave like to-animation a lot of the time.  Maybe replace with:

  // Although <set> animation can look like to-animation,
  // it behaves differently in some circumstances (particularly
  // with interpolatable attributes).

Comment 7

[Brian Birtles (:birtles)]

Attachment: Patch v1d

(In reply to comment #5)
> >+     class="reftest-wait"
> >+     onload="setTimeAndSnapshot(2, true)">
> >+  <script xlink:href="smil-util.js" type="text/javascript"/>
> 
> Looks like all of the reftests here share the three lines above, but none of
> them need them, AFAIK, since there's no time-dependence at all -- right? 
> (Every animation begins at time 0 by default, and has an indefinite duration,
> so there's no need to do any seeking / pausing)

Right you are, thanks for catching that.

> >+++ b/layout/reftests/svg/smil/anim-indefinite-to-1.svg
> >+  <!-- Test that an indefinite to-animation just sticks to the base value -->
> 
> You should probably add "... for interpolatable attributes." at the end of that
> comment. (since we have the opposite behavior for *non*-interpolatable
> attributes, as your second reftest shows.)

Right, fixed.

> >+++ b/layout/reftests/svg/smil/anim-indefinite-to-4.svg
> >+  <!-- Not really a to-animation, but check that set animation isn't incorrectly
> >+       treated as to-animation when we have an indefinite simply duration. -->
> >+  <rect x="15" y="15" width="200" height="100" fill="blue">
> >+    <set attributeName="height" to="200"/>
> 
> s/simply duration/duration/, I think.

Yeah, it should be simple duration. (Since indefinite active duration is also possible but behaves differently.)

> This comment needs a little clarification -- it took me a minute to figure out
> what this meant (since, for non-interpolatable attributes, <set to> *does*
> behave exactly like <animate to>).

Ok, I've added that clarification.

> Also: It might be worth explicitly specifying |dur="indefinite"| in these
> tests' animations, just for clarity's sake, but I'll leave that up to you. :)

Definitely. I set it for the first three but not the last which is an oversight. Fixed now.

(In reply to comment #6)
> I don't love the logic structure here -- it's a bit confusing.
> (For example, the "!IsToAnimation()" check followed immediately by an "if
> (IsToAnimation())" -- that strikes me as being jumbled and sub-optimal.)
> 
> I think we should...

That's great. Fixed.

I've reworked some of the logic 

> >+++ b/content/smil/nsSMILSetAnimationFunction.h
> >+  // Although <set> animation might look like to-animation it doesn't behave
> >+  // like it.
> >+  NS_OVERRIDE virtual PRBool IsToAnimation() const {
> >+    return PR_FALSE;
> 
> This comment is a little confusing (particularly "it doesn't behave like it"),
> because <set> animation *does* actually behave like to-animation a lot of the
> time.

Ok I've adjusted the comment a bit but not quite as suggested so you might want to check it. (Basically my understanding is that to-animation aligning with set-animation in the case of a non-interpolatable attribute is more the exception than the rule since to-animation seems to be designed with interpolation in mind, or at least that's the case the SMIL specs spend most energy on.)

Cheers Daniel, thanks for all your help on this.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 494905 [details] [diff] [review]
Patch v1d

>+  // If we have an indefinite simple duration, just set the progress to be
>+  // 0 which will give us the expected behaviour of the animation being fixed at
>+  // it its start point.

s/it its/its/

>+    simpleProgress = dur > 0 ? (double)mSampleTime / dur : 0.0;

No real need for the " : 0.0;" case here, since simpleProgress was already initialized to 0.0 above.

Just do something like:
  if (dur > 0) {
    simpleProgress = (double)mSampleTime / dur;
  } // else (unexpected, per ABORT_IF_FALSE above): leave progress at 0

Otherwise looks great! r=dholbert. Thanks for fixing this. :)

Comment 9

[Brian Birtles (:birtles)]

Attachment: Patch v1e; r=dholbert

(In reply to comment #8)
> Comment on attachment 494905 [details] [diff] [review]
> Patch v1d
> 
> >+  // If we have an indefinite simple duration, just set the progress to be
> >+  // 0 which will give us the expected behaviour of the animation being fixed at
> >+  // it its start point.
> 
> s/it its/its/
Fixed, thanks.

> >+    simpleProgress = dur > 0 ? (double)mSampleTime / dur : 0.0;
> 
> No real need for the " : 0.0;" case here, since simpleProgress was already
> initialized to 0.0 above.
> 
> Just do something like:
>   if (dur > 0) {
>     simpleProgress = (double)mSampleTime / dur;
>   } // else (unexpected, per ABORT_IF_FALSE above): leave progress at 0

As discussed on IRC, this case is valid when mSampleTime == dur == 0 so I've applied the above suggestion but updated the comment accordingly.

Comment 10

[Brian Birtles (:birtles)]

Requesting approval to land. Fixes incorrect behaviour. Includes tests. Doesn't add complexity but rather simplifies the code.

Comment 11

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/566bac81d5fe