SVG Animation setCurrentTime fails (seeks to wrong time), when called after pausing & waiting

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

9 years ago
Currently, setCurrentTime ends up seeking to the wrong time, if it's called significantly after "pauseAnimations" was called.

In fact, if "n" seconds have elapsed since pauseAnimations was called, then setCurrentTime will be off by about "n" seconds.

STEPS TO REPRODUCE:
 1. Enable the "svg.smil.enabled" pref, in a mozilla-central nightly
 2. Load testcase

EXPECTED RESULTS:
 "Success" alert should pop up (indicating that getCurrentTime gave us back the value that we set with setCurrentTime)

ACTUAL RESULTS:
 "Failure" alert pops up (indicating that getCurrentTime gave us back an unexpected value)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090507 Minefield/3.6a1pre

Opera v9.64 shows expected results.
(Assignee)

Updated

9 years ago
Summary: SVG Animation setCurrentTime fails (sets wrong time), when called after pausing & waiting → SVG Animation setCurrentTime fails (seeks to wrong time), when called after pausing & waiting
(Assignee)

Comment 1

9 years ago
Created attachment 376447 [details]
testcase 1: wait 200 ms
(Assignee)

Comment 2

9 years ago
Created attachment 376449 [details]
testcase 2: wait 1 sec

(In reply to comment #0)
> In fact, if "n" seconds have elapsed since pauseAnimations was called, then
> setCurrentTime will be off by about "n" seconds.

This testcase demonstrates this aspect -- it waits for 1 sec before calling setCurrentTime, and the result is that we're off by ~1 sec.
(Assignee)

Updated

9 years ago
Attachment #376447 - Attachment description: testcase 1 → testcase 1: wait 200 ms
(Assignee)

Comment 3

9 years ago
Created attachment 376453 [details] [diff] [review]
fix

I think this fixes it.  Basically, the situation with current mozilla-central is this:
 * mParentOffset is effectively the unix time of our document start time.
 * When we're paused, GetCurrentTime effectively returns mPauseStart - mParentOffset (which is cached as mCurrentTime in UpdateCurrentTime())
 * In setCurrentTime(), we adjust mParentOffset (using an updated value of PR_Now() via GetParentTime()), but we **don't** update mPauseStart

As a result, if we wait longer to call SetCurrentTime, then mParentOffset gets larger (whereas mPauseStart doesn't change), and so our effective "current time" shrinks.

This patch simply makes us update mPauseStart with mParentOffset in SetCurrentTime, keeping it exactly "aSeekTo" milliseconds beyond mParentOffset.

I still need to run this through reftests & mochitests, but I'm pretty sure this is correct.
(Assignee)

Updated

9 years ago
Attachment #376449 - Attachment description: testcase 1: wait 1 sec → testcase 2: wait 1 sec
(Assignee)

Comment 4

9 years ago
Created attachment 376480 [details] [diff] [review]
fix with mochitest

Yup, this passes the SMIL mochitests & reftests.

Here's the same fix, with a mochitest attached.  I've confirmed that the mochitest fails[1] pre-patching and passes post-patching.

[1] By "fails", I mean it fails on all the checks after setTimeout calls, *except* for the one check with setCurrentTime(10000000), as that value is apparently large enough to crop the (relatively small) accumulated wait time from its float representation.
Attachment #376480 - Flags: superreview?(roc)
Attachment #376480 - Flags: review?(roc)
Attachment #376480 - Flags: superreview?(roc)
Attachment #376480 - Flags: superreview+
Attachment #376480 - Flags: review?(roc)
Attachment #376480 - Flags: review+
(Assignee)

Comment 5

9 years ago
Fix pushed: http://hg.mozilla.org/mozilla-central/rev/a3a08801bd70
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.