557885 - SVG/SMIL: 'keyTimes' attribute should be honored when calcMode="discrete"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Currently, when we have an animation with calcMode="discrete", we don't honor the |keyTimes| attribute at all.

We should be honoring keyTimes there, though -- according to the spec, keyTimes should behave as follows when calcMode="discrete":
==
For discrete animation, the first time value in the list must be 0. The time associated with each value defines when the value is set; the animation function uses that value until the next time defined in keyTimes.
==
http://www.w3.org/TR/2001/REC-smil-animation-20010904/#AnimFuncCalcMode

See attached testcase, with two discrete-calcMode animations that are identical except for the presence of |keyTimes|.

Comment 1

[Daniel Holbert [:dholbert]]

When we fix this up, we should consider being stricter about the |keyTimes| values we accept, in either the same patch or a related patch.

In particular: For linear and spline calcMode, we're currently "forgiving" in that we allow the final |keyTimes| entry to be less than 1.0, despite the letter of the spec.[1]  This is because of an error in an example in the spec, as Brian describes in a blog post.[2]

However, as Olaf says in his comment on Brian's post, SMIL 3 has removed/replaced the broken example, and it reiterates the value restriction.[3]  Since we're already doing some reworking our handling of keyTimes (in this bug here), it seems to me that we should align with the intention of the spec on keyTimes-strictness -- ignore the broken example, and not accept the broken syntax.

(Side note: in the special case of calcMode="discrete", the final value of keyTimes does NOT have to be 1 -- but the first value is still supposed to be 0. See the chunk of spec linked in comment 0.)

[1] http://www.w3.org/TR/2001/REC-smil-animation-20010904/#AnimFuncCalcMode
[2] http://brian.sol1.net/svg/range-for-keytimes/
[3] http://www.w3.org/TR/SMIL/smil-animation.html#adef-keyTimes

Comment 2

[Daniel Holbert [:dholbert]]

Jeff pointed out to me that we currently fail (i.e. have the wrong timing on) the SVG test http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-14-t.html

That test uses calcMode="discrete" & keyTimes, so it's basically failing due to this bug.

Comment 3

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

This seems to me like something we should fix before we ship SMIL as I imagine a lot of content will get tested against our implementation. Better to be strict now and relax it later on than vice versa. I'm happy to take this.

Comment 4

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

See previous comment for my reason for nominating this.

Comment 5

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

This shouldn't be hard to implement as we previously implemented the more strict behaviour and so it should be just a matter of (a) reviving the old code, (b) updating our tests to match the new behaviour, and (c) adding new tests. 0.5 days.

Comment 6

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

Attachment: Patch v1a

This patch makes us honour keyTimes with calcMode=discrete as well as making us more strict on bad keyTimes as proposed in comment 1.

It took a little longer than anticipated because when I estimated the time I only had the latter part (making the keyTime checks more strict) in mind. I didn't realise this was a compound bug.

I've also added further parsing tests for keyTimes (as per bug 474742).

Comment 7

[Daniel Holbert [:dholbert]]

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

Looks good -- thanks for fixing this up!  Comments below:

>diff --git a/content/smil/nsSMILAnimationFunction.cpp b/content/smil/nsSMILAnimationFunction.cpp
>-    double intervalProgress;
>+    double intervalProgress = -1.f;

I think I'd prefer to leave this uninitialized, since I don't think this -1 value ever gets used.  It looks like every case below this will set |intervalProgress|.  (with the possible exception of the ComputePacedPosition call if it fails... but in that case, we don't end up using |intervalProgress| at all.)

>   for (; i < numTimes - 2 && aProgress >= mKeyTimes[i+1]; ++i);
> 
>+  if (aCalcMode == CALC_DISCRETE) {
>+    // discrete calcMode behaviour differs in that each keyTime defines the time
>+    // from when the corresponding value is set, and therefore the last value
>+    // needn't be 1. So check if we're in the last 'interval', that is, the
>+    // space between the final value and 1.0.
>+    if (aProgress >= mKeyTimes[i+1]) ++i;
>+    return (double)i / numTimes;

Two things about the added "if (aProgress >= mKeyTimes[i+1]) ++i;" line:
 1) Put ++i on its own line.
 2) I think we can only satisfy this condition and hit the "++i" for one i-value: numTimes - 2. Could you assert that i=numTimes-2, just before the "++i" line, just for code-understandability purposes?  (At first glance, the current code reads as if it applies to many values of "i".  It takes some code inspection to realize that it's really only applicable for one particular i-value).

>-  aProgress = (i + (aProgress - intervalStart) / intervalLength) *
>+  return (i + (aProgress - intervalStart) / intervalLength) *
>          1.0 / double(numTimes - 1);

As long as you're tweaking this line: we might as well get rid of that "1.0", right? (It's simpler to directly divide by double(numTimes-1) rather than multiplying by 1.0-over-that.)

>+nsSMILAnimationFunction::ScaleIntervalProgress(double aProgress,
>+                                               PRUint32 aIntervalIndex,
>+                                               PRUint32 aNumIntervals)

It looks like this method doesn't use |aNumIntervals| anymore, except to assert that it's >=1. Can we just get rid of that param?


>+  NS_ABORT_IF_FALSE(aIntervalIndex < (PRUint32)mKeySplines.Length(),
>+                    "Invalid interval index");

Get rid of the PRUint32 cast -- nsTArray::Length already returns an unsigned value.

> nsSMILAnimationFunction::CheckKeyTimes(PRUint32 aNumValues)
[...]
>+  // For to-animation the number of values is considered to be 2 unless it's
>+  // discrete to-animation in which case either 1 or 2 is acceptable.
>+  PRBool matchingNumOfValues = IsToAnimation() ?
>+      calcMode == CALC_DISCRETE ? numKeyTimes <= 2 : numKeyTimes == 2 :
>+      aNumValues == numKeyTimes;

Extreme nit: could you flip that last line around -- numKeyTimes == aNumValues -- to match the ordering of the checks right before it? :)

>   // special handling if there is only one keyTime. The spec doesn't say what to
>   // do in this case so we allow the keyTime to be either 0 or 1.
>-  if (mKeyTimes.Length() == 1) {
>+  if (numKeyTimes == 1) {
>     double time = mKeyTimes[0];
>     SetKeyTimesErrorFlag(!(time == 0.0 || time == 1.0));
>     return;
>   }

Wait... Is this (numKeyTimes==1) supposed to be allowed? Per comment 0, the first keyTime must be 0, and per comment 1, the last keyTime must be 1. (unless calcMode="discrete")  So if the user specifies keyTimes, I'd think that two keyTimes would be required, right? (for non-discrete calcMode)

In the calcMode="discrete" case, I'd expect that we'd allow a singleton-keyTimes-list, but its singleton value would need to be 0, per the side note in comment 1.

So, I'd imagine we should just delete the above-quoted clause entirely, and add tests to make sure that single-keyTime-with-calcMode="linear" fails.  (Does that break anything?)

r=dholbert with the above addressed

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> So, I'd imagine we should just delete the above-quoted clause entirely, and add
> tests to make sure that single-keyTime-with-calcMode="linear" fails.  (Does
> that break anything?)

(Bonus points for tests to verify that single-keytime-with-calcMode="discrete" fails IFF the single-keytime is > 0)

Comment 9

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

Attachment: Patch v1b, r=dholbert

(In reply to comment #7)
> >diff --git a/content/smil/nsSMILAnimationFunction.cpp b/content/smil/nsSMILAnimationFunction.cpp
> >-    double intervalProgress;
> >+    double intervalProgress = -1.f;
> 
> I think I'd prefer to leave this uninitialized, since I don't think this -1
> value ever gets used.  It looks like every case below this will set
> |intervalProgress|.  (with the possible exception of the ComputePacedPosition
> call if it fails... but in that case, we don't end up using |intervalProgress|
> at all.)

Yeah, the reason I initialised it to -1 is that I want to be sure that if someone later changes the following code and the intervalProgress doesn't get set, then we will be sure to fail the assertion:

NS_ABORT_IF_FALSE(0.0f <= intervalProgress && intervalProgress < 1.0f,
                  "Interval progress should be in the range [0, 1)");
                  
As it currently stands, that assertion may just happen to pass even if intervalProgress was not set (which would be a bug) if by a stroke of (bad) luck intervalProgress ends up with a value in the range [0, 1).

I want to make sure that if there is a bug we fail consistently and early. Is that ok?

> Two things about the added "if (aProgress >= mKeyTimes[i+1]) ++i;" line:
>  1) Put ++i on its own line.
>  2) I think we can only satisfy this condition and hit the "++i" for one
> i-value: numTimes - 2. Could you assert that i=numTimes-2, just before the
> "++i" line, just for code-understandability purposes?  (At first glance, the
> current code reads as if it applies to many values of "i".  It takes some code
> inspection to realize that it's really only applicable for one particular
> i-value).

Fixed.

> >-  aProgress = (i + (aProgress - intervalStart) / intervalLength) *
> >+  return (i + (aProgress - intervalStart) / intervalLength) *
> >          1.0 / double(numTimes - 1);
> 
> As long as you're tweaking this line: we might as well get rid of that "1.0",
> right? (It's simpler to directly divide by double(numTimes-1) rather than
> multiplying by 1.0-over-that.)

Fixed.

> >+nsSMILAnimationFunction::ScaleIntervalProgress(double aProgress,
> >+                                               PRUint32 aIntervalIndex,
> >+                                               PRUint32 aNumIntervals)
> 
> It looks like this method doesn't use |aNumIntervals| anymore, except to assert
> that it's >=1. Can we just get rid of that param?

Fixed.

> >+  NS_ABORT_IF_FALSE(aIntervalIndex < (PRUint32)mKeySplines.Length(),
> >+                    "Invalid interval index");
> 
> Get rid of the PRUint32 cast -- nsTArray::Length already returns an unsigned
> value.

Fixed -- this and the above have been there a while.

> > nsSMILAnimationFunction::CheckKeyTimes(PRUint32 aNumValues)
> [...]
> >+  // For to-animation the number of values is considered to be 2 unless it's
> >+  // discrete to-animation in which case either 1 or 2 is acceptable.
> >+  PRBool matchingNumOfValues = IsToAnimation() ?
> >+      calcMode == CALC_DISCRETE ? numKeyTimes <= 2 : numKeyTimes == 2 :
> >+      aNumValues == numKeyTimes;
> 
> Extreme nit: could you flip that last line around -- numKeyTimes == aNumValues
> -- to match the ordering of the checks right before it? :)

Fixed.

> >   // special handling if there is only one keyTime. The spec doesn't say what to
> >   // do in this case so we allow the keyTime to be either 0 or 1.
> >-  if (mKeyTimes.Length() == 1) {
> >+  if (numKeyTimes == 1) {
> >     double time = mKeyTimes[0];
> >     SetKeyTimesErrorFlag(!(time == 0.0 || time == 1.0));
> >     return;
> >   }
> 
> Wait... Is this (numKeyTimes==1) supposed to be allowed? Per comment 0, the
> first keyTime must be 0, and per comment 1, the last keyTime must be 1. (unless
> calcMode="discrete")  So if the user specifies keyTimes, I'd think that two
> keyTimes would be required, right? (for non-discrete calcMode)

Yeah, technically I guess you can do something like <animate values="10" keyTimes="0" calcMode="linear" ...> so it's more than just non-discrete calcMode. I've removed the clause above and updated the following check so that if we only have one value and one keyTime it can be 0.

Comment 10

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

Attachment: Patch v1c, r=dholbert

Fixed the test case. Previously the added test said it was using calcMode=discrete when it wasn't. Also, checked a few other variations on the theme of one keyTime.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> Yeah, the reason I initialised it to -1 is that I want to be sure that if
> someone later changes the following code and the intervalProgress doesn't get
> set, then we will be sure to fail the assertion:
> 
> NS_ABORT_IF_FALSE(0.0f <= intervalProgress && intervalProgress < 1.0f,
>                   "Interval progress should be in the range [0, 1)");

Ah -- ok, that makes more sense, then. :)

Can you add a comment above the -1 setting, then, so -1 is less of a "magic number"?  It could say something like:
  // Init intervalProgress to -1, to make sure that if we ever forget to
  // set it,  we'll fail the NS_ABORT_IF_FALSE about its expected range.

You might want to put the "-1" initialization in an "#ifdef DEBUG" block, too (since NS_ABORT_IF_FALSE is debug-only).  I don't feel too strongly about that, though. :)

> I've removed the clause above and updated the following check so that
> if we only have one value and one keyTime it can be 0.

I'm still not sure the spec allows for one entry in the keyTimes list, though...  This part is pretty explicit:
   For linear and spline animation, the first time value in the list
   must be 0, and the last time value in the list must be 1.
http://www.w3.org/TR/SVG/animate.html#KeyTimesAttribute

To me, that implies that keyTimes *must* have >=2 values (for linear and spline animation).

Even if we wanted to ignore that problem, I don't see how it's useful to honor a singleton keyTimes-list.  Maybe it makes us more forgiving about silly markup (but really, it doesn't even make us *that* much more forgiving, since we'd only allow one possible value for that single-keyTime (0), anyway)).

It might be worth requesting clarification in the spec on this (whether a singleton keyTimes list is allowed for linear/spline).  Without that clarification, I tend to think we shouldn't allow it, but if you feel strongly the other way, I could go along with what you've got.

Comment 12

[Daniel Holbert [:dholbert]]

On the other hand... I guess you could consider calcMode="linear|spline" with a single entry in the values list to be a *form* of "discrete animation", rather than "linear or spline animation", since in a sense no linear/spline action actually happens when there's just one value.  (even though the calcMode is linear|spline)

Also, maybe it's worth supporting singleton keyTimes lists for the simple fact that some authors may get in the habit of always specifying a keyTime for each of their values, and they'd expect this to work when they have just one value in their |values| list (which is of course allowed).  Even though that's kind of silly. :)

Still, I think some clarification about this on the www-svg list would be useful here.

Comment 13

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

Attachment: Patch v1d, r=dholbert

(In reply to comment #11)
> Can you add a comment above the -1 setting, then, so -1 is less of a "magic
> number"?  It could say something like:
>   // Init intervalProgress to -1, to make sure that if we ever forget to
>   // set it,  we'll fail the NS_ABORT_IF_FALSE about its expected range.

Done.

(In reply to comment #12)
> Also, maybe it's worth supporting singleton keyTimes lists for the simple fact
> that some authors may get in the habit of always specifying a keyTime for each
> of their values, and they'd expect this to work when they have just one value
> in their |values| list (which is of course allowed).  Even though that's kind
> of silly. :)

Yeah, that's what I had in mind. Specifically I was thinking of tools/scripts that just spit out keyTimes regardless of whether it makes sense or not. Sometimes it's easier to get the script to just spit out a '0' than having to special-case it to not set the attribute at all.

> Still, I think some clarification about this on the www-svg list would be
> useful here.

Sent: http://lists.w3.org/Archives/Public/www-svg/2010Sep/0113.html

Comment 14

[Daniel Holbert [:dholbert]]

Excellent, thanks!

Comment 15

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

Pushed: http://hg.mozilla.org/mozilla-central/rev/c7a1a954e862