Closed
Bug 681645
Opened 13 years ago
Closed 13 years ago
SVG SMIL : Animation skips 30th and 58th frame
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: rakssvg, Assigned: birtles)
Details
Attachments
(5 files, 3 obsolete files)
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•13 years ago
|
||
0.29;0.307;0.31 Shouldn't the middle number be 0.30
Updated•13 years ago
|
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
I was just playing with various values and attached the file with 0.307, reattaching the file having the value as 0.30
Updated•13 years ago
|
Attachment #555404 -
Attachment mime type: text/plain → image/svg+xml
Updated•13 years ago
|
Attachment #555399 -
Attachment is obsolete: true
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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.
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Version: 6 Branch → Trunk
Comment 6•13 years ago
|
||
Just as an absolute sanity-check: I verified that my self-describing testcase shows correct/expected results on Opera 11.50 (fill == stroke always).
Assignee | ||
Comment 7•13 years ago
|
||
I've yet to look into this but I'm happy to take it for now.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•13 years ago
|
||
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.
Attachment #555911 -
Flags: review?(dholbert)
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
Comment 11•13 years ago
|
||
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•13 years ago
|
||
(Note that both reduced testcases render correctly (blue) in Opera.)
Assignee | ||
Comment 13•13 years ago
|
||
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).
Assignee | ||
Comment 14•13 years ago
|
||
Here's a patch that incorporates and fixes the second test case (i.e. attachment 556153 [details])
Attachment #556743 -
Flags: review?(dholbert)
Reporter | ||
Comment 15•13 years ago
|
||
Well I don't know the internals, but I expect that after the fix setCurrentTime() displays the correct frame
Comment 16•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.)
Assignee | ||
Comment 20•13 years ago
|
||
(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. :)
Attachment #555911 -
Attachment is obsolete: true
Attachment #556743 -
Attachment is obsolete: true
Attachment #555911 -
Flags: review?(dholbert)
Attachment #556743 -
Flags: review?(dholbert)
Attachment #557046 -
Flags: review?
Comment 21•13 years ago
|
||
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!
Attachment #557046 -
Flags: review? → review+
Assignee | ||
Comment 22•13 years ago
|
||
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/bff12708b423
Whiteboard: [inbound]
Comment 23•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bff12708b423
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•