681645 - SVG SMIL : Animation skips 30th and 58th frame

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Raks]

Attachment: SVG file

While using setCurrentTime we found a BUG which apparently seems to be a bug with the player.
We created a animation which toggles the display of a black rectangle every 0.5 seconds. The duration of the animation is  50 seconds hence there are 100 frames. As per the animation the rectangle is not to be displayed at the 30th frame but still it remains displayed. It seems as if the keyTimes value at that instant is ignored. Same is with the 58th frame. Increasing the duration of the animtion does not solve the problem.

Attached is the svg file

Comment 1

[Robert Longson [:longsonr]]

0.29;0.307;0.31

Shouldn't the middle number be 0.30

Comment 2

[Raks]

I was just playing with various values and attached the file with 0.307, reattaching the file having the value as 0.30

Comment 3

[Raks]

Attachment: SVG File Correct One

Ignore the earlier file, use this one

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: self-describing testcase

Here's a somewhat easier-to-see-the-bug testcase.  I changed it to animate 'fill' instead of 'display', and I added a simpler parallel animation on the 'stroke' (which does not show the bug*, and hence makes it more obvious when the main 'fill' animation breaks).

I also added a countdown to the time at which I see the bug.

* (the 'stroke' animation presumably doesn't hit the bug because it uses a simpler repeating animation, without any keyTimes etc.)

Comment 5

[Daniel Holbert [:dholbert]]

I can reproduce, with both testcases, in today's nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1

Confirming.

Comment 6

[Daniel Holbert [:dholbert]]

Just as an absolute sanity-check: I verified that my self-describing testcase shows correct/expected results on Opera 11.50 (fill == stroke always).

Comment 7

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

I've yet to look into this but I'm happy to take it for now.

Comment 8

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

Attachment: Patch v1a

Proposed patch for this.

Basically, what was happening is we'd get to this line in nsSMILAnimationFunction::ScaleSimpleProgress:

  return (double)i / numTimes;

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#664

When i=29, we'd get a result of 0.2899999999999999

Then back in nsSMILAnimationFunction::InterpolateResult we'd hit:

  PRUint32 index = (PRUint32)floor(scaledSimpleProgress * aValues.Length());

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#489

Which would take the index to 28 instead of 29.

I think the floor behaviour is right so I've adjusted ScaleSimpleProgress to put the scaled time in the middle of the keyTimes interval so we don't hit these boundary conditions.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1 (with keyTimes)

(review comments coming up; first, I'm posting a non-reftest version of the reduced testcase from Brian's patch, and then a variant testcase, to illustrate one comment)

Here's the reduced testcase from Brian's patch.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 2 (*without* keyTimes)

Comment 11

[Daniel Holbert [:dholbert]]

Reduced testcase 2 (just attached) has no keyTimes -- just the 100 values -- so it skips nsSMILAnimationFunction::ScaleSimpleProgress() altogether.

We still end up with the value 0.2899999999999999, though, from this line:
> 423       simpleProgress = (double)mSampleTime / dur;

I think the fix for this bug will need to address this case as well.  I suspect we need a NS_Round() somewhere, but I'm not immediately sure where.

Comment 12

[Daniel Holbert [:dholbert]]

(Note that both reduced testcases render correctly (blue) in Opera.)

Comment 13

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

The second test case is a different kind of bug. If you play the animation (rather than just snapshotting) you won't be able to see anything amiss. But with the original bug the interval is dropped.

In the second test case, if you set the snapshot time to 29.01 you'll get blue. But in the first test case you'll still get red.

In order to fix it I suggest just adding a fudge factor of 0.0000000000000001 to the value before we perform a floor().

A fudge factor this small would mean that even for:
mSampleTime = 2899999999999
dur = 10000000000000
we'd should still end up in the 28-29 interval

I'm suggesting we do this at each point before we floor for consistency (including linear/spline although it will only really be noticeable for discrete).

Comment 14

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

Attachment: Patch part 2 v1a

Here's a patch that incorporates and fixes the second test case (i.e. attachment 556153 [details])

Comment 15

[Raks]

Well I don't know the internals, but I expect that after the fix setCurrentTime() displays the correct frame

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #13)
> In order to fix it I suggest just adding a fudge factor of
> 0.0000000000000001 to the value before we perform a floor().

That sounds reasonable to me.
(Darned float<-->integer interconversion errors. :))

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 556743 [details] [diff] [review]
Patch part 2 v1a

>@@ -449,18 +461,25 @@ nsSMILAnimationFunction::InterpolateResu
>                                 intervalProgress, from, to);
[...]
>     } else { // calcMode == CALC_LINEAR or calcMode == CALC_SPLINE
>       double scaledSimpleProgress =
>         ScaleSimpleProgress(simpleProgress, calcMode);
>-      PRUint32 index = (PRUint32)floor(scaledSimpleProgress *
>-                                       (aValues.Length() - 1));
>+      PRUint32 index =
>+        (PRUint32)floor((scaledSimpleProgress + floatingPointFudgeFactor) *
>+                        (aValues.Length() - 1));
>+      // Clamp upper-bound (since the floating-point fudge factor might push us
>+      // just out of range)
>+      index = NS_MIN(index, aValues.Length() - 2);
>+      // Check lower-bound
>+      NS_ABORT_IF_FALSE(index >= 0,
>+        "Got negative index, scaled simple progress out of range");
>       from = &aValues[index];
>       to = &aValues[index + 1];
>       intervalProgress =
>         scaledSimpleProgress * (aValues.Length() - 1) - index;
>       intervalProgress = ScaleIntervalProgress(intervalProgress, index);

I don't think we need or want this change for the LINEAR/SPLINE cases. (the above chunk)  Those should already be handled correctly (or very nearly correctly) by the |intervalProgress| variable, which accounts for the portion that's dropped by the floor() call.

IIUC, the above chunk will just mean the difference between doing an interpolation ~0.9999999999999 of the way between values[28] and values[29] vs. doing an interpolation ~0.00000000001 of the way between values[29] and values[30].  Both of those calculations will end up with a result equal to (or very nearly equal to) values[29].  And it's likely that the existing (unpatched) computation will be closer to the "correct" value, I think.

So unless you object, I think I'd suggest dropping the above chunk.

>@@ -478,20 +497,26 @@ nsSMILAnimationFunction::InterpolateResu
>   if (calcMode == CALC_DISCRETE || NS_FAILED(rv)) {
>     double scaledSimpleProgress =
>       ScaleSimpleProgress(simpleProgress, CALC_DISCRETE);
>     if (IsToAnimation()) {
[...]
>-      PRUint32 index = (PRUint32)floor(scaledSimpleProgress * 2);
>+      PRUint32 index =
>+        (PRUint32)floor((scaledSimpleProgress + floatingPointFudgeFactor) * 2);
>       aResult = index == 0 ? aBaseValue : aValues[0];
>     } else {
>-      PRUint32 index = (PRUint32)floor(scaledSimpleProgress * aValues.Length());
>+      PRUint32 index =
>+        (PRUint32)floor((scaledSimpleProgress + floatingPointFudgeFactor) *
>+                        aValues.Length());

I'd prefer we just added floatingPointFudgeFactor to scaledSimpleProgress up-front, rather than adding it inline each place scaledSimpleProgress is used.

>+      // Clamp upper-bound (since the floating-point fudge factor might push us
>+      // just out of range)
>+      index = NS_MIN(index, aValues.Length() - 1);

So this can only be a problem if scaledSimpleProgress ends up being >= 1.0, which hasn't been possible before but now might be possible if our fudge factor pushes us over.

I think it'd be better to preserve the "progress must be in the range [0, 1)" invariant, so that we don't hit this problem in the first place. Perhaps something like this:
>   if (calcMode == CALC_DISCRETE || NS_FAILED(rv)) {
>     double scaledSimpleProgress =
>       ScaleSimpleProgress(simpleProgress, CALC_DISCRETE);
>+     // Apply fudge factor, but don't let it move progress out of the [0, 1) range
>+     if (scaledSimpleProgress + floatingPointFudgeFactor <= 1.0) {
>+       scaledSimpleProgress += floatingPointFudgeFactor;
>+     }

Comment 18

[Daniel Holbert [:dholbert]]

Two more notes, on floatingPointFudgeFactor:
 - I think general Mozilla style for constant variables is to use a "k" prefix[1] (even for local variables[2]).  So, consider naming this "kFloatingPointFudgeFactor"

 - Assuming you agree with my suggestions in comment 17, you might as well move the fudge-factor declaration down to the only place it'll actually be used now (just after the "// Apply fudge factor" comment in the suggested code-chunk @ end of comment 17)

[1] https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes
[2] There are many examples of this in e.g. nsCSSParser.cpp (though there are some counterexamples too) -- e.g. in CSSParserImpl::ParseImageRect and in most of the CSSParserImpl::ParseBorderXXX functions.

Comment 19

[Daniel Holbert [:dholbert]]

Also, RE the attachment being named "Patch part 2 v1a" (and apparently stacking on top of the earlier patch):  I _think_ the only code-change that we actually need here is the chunk suggested at the end of comment 17.

So, this should end up being a 1-part fix -- is that right?

(also: it looks like "Patch part 2 v1a" adds an entry for "anim-discrete-values-3.svg" to reftest.list, but it's missing that test file.)

Comment 20

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

Attachment: Patch v1b

(In reply to Daniel Holbert [:dholbert] from comment #17)
> I don't think we need or want this change for the LINEAR/SPLINE cases. (the
> above chunk)  Those should already be handled correctly (or very nearly
> correctly) by the |intervalProgress| variable, which accounts for the
> portion that's dropped by the floor() call.

Yeah, I mentioned doing this at the end of comment 13 just for consistency. But I'm fine with not doing it so I've removed it now.

> I think it'd be better to preserve the "progress must be in the range [0,
> 1)" invariant, so that we don't hit this problem in the first place.

Ok, sounds good.

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Also, RE the attachment being named "Patch part 2 v1a" (and apparently
> stacking on top of the earlier patch):  I _think_ the only code-change that
> we actually need here is the chunk suggested at the end of comment 17.

That's right, we only need the second code change but I was thinking of keeping the first anyway. That is, when we're applying keyTimes to discrete timing, scale the time to the middle of the appropriate interval to avoid any odd behaviour at interval boundaries. I'll leave it out for now anyway.

> (also: it looks like "Patch part 2 v1a" adds an entry for
> "anim-discrete-values-3.svg" to reftest.list, but it's missing that test
> file.)

Yeah, I forgot to hg add. Explains the Try server failures. :)

Comment 21

[Daniel Holbert [:dholbert]]

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

(In reply to Brian Birtles (:birtles) from comment #20)
> That's right, we only need the second code change but I was thinking of
> keeping the first anyway. That is, when we're applying keyTimes to discrete
> timing, scale the time to the middle of the appropriate interval to avoid
> any odd behaviour at interval boundaries. I'll leave it out for now anyway.

(Cool -- I'm willing to believe that original change might be something that we'd want, but I don't have a great handle on what the significance of that change would actually be & what real-world effects it'd have.  In any case, it'd probably be worth another bug, and it'd be best if we had a separate testcase that was still broken even after this bug's 1.0e-16 fudge-factor fix).

r=me. Thanks for fixing this!

Comment 22

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

Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bff12708b423

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bff12708b423