Bug 65282 - SMILTimeContainer should use monotonicallyIncreasingTime() rather than currentTime() for animations
: SMILTimeContainer should use monotonicallyIncreasingTime() rather than curren...
Status: UNCONFIRMED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-07-27 13:59 PST by
Modified: 2012-02-13 11:22 PST (History)


Attachments
use monotonicallyIncreasingTime rather than currentTime to update animations (2.28 KB, patch)
2011-07-27 14:06 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
use monotonicallyIncreasingTime rather than currentTime to update animations (2.29 KB, patch)
2011-07-27 14:12 PST, Scott Graham
krit: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-27 13:59:54 PST
It appears it should be using monotonicallyIncreasingTime to handle changing system clocks while animating.
------- Comment #1 From 2011-07-27 14:06:39 PST -------
Created an attachment (id=102181) [details]
use monotonicallyIncreasingTime rather than currentTime to update animations
------- Comment #2 From 2011-07-27 14:09:54 PST -------
Attachment 102181 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2011-07-27 14:12:08 PST -------
Created an attachment (id=102183) [details]
use monotonicallyIncreasingTime rather than currentTime to update animations
------- Comment #4 From 2011-07-27 23:46:13 PST -------
(From update of attachment 102183 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=102183&action=review

The timing code is similar to the code in AnimationControler and AnimationBase. But these classes use currentTime() as well. Shouldn't they be affected as well some how?

r- because of the changelog.

> Source/WebCore/ChangeLog:9
> +        SMILTimeContainer using currentTime for animations
> +        https://bugs.webkit.org/show_bug.cgi?id=65282
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests, existing tests should play correctly when system clock
> +        changed.

This needs a detailed description. What did you change? Why did you change it? And it is not only important that do not break thinks: It is important how it affects the behavior. You need tests to verify the new behavior, especially what works now but didn't work before. Does your patch affect bug 31256 as well?

> Source/WebCore/ChangeLog:11
> +        * svg/animation/SMILTimeContainer.cpp:

Is SMILTimeContainer the only file in WebCore/svg that uses currentTime()?
------- Comment #5 From 2011-07-27 23:49:38 PST -------
Adding Dean and Simon to the bug report. I'd like to hear their opinion to the changes. Like I wrote before, the SMILTimeContainer code is similar to the animation stuff in WebCore/page/animation. But the AnimationBase and AnimationController use currentTime() as well.
------- Comment #6 From 2011-07-28 11:20:04 PST -------
(In reply to comment #4)
> (From update of attachment 102183 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102183&action=review
> 
> The timing code is similar to the code in AnimationControler and AnimationBase. But these classes use currentTime() as well. Shouldn't they be affected as well some how?

I suspect they should be, but maybe someone else who knows better can comment. From my understanding, currentTime() is correct when you want to know what time the user sees on his clock. currentTime() can jump forward or backwards (suspend, change system time), so for calculating a delta between two events monotonicallyIncreasingTime() would be preferred.

> r- because of the changelog.

My apologies for forgetting this, that was dumb.

> 
> > Source/WebCore/ChangeLog:9
> > +        SMILTimeContainer using currentTime for animations
> > +        https://bugs.webkit.org/show_bug.cgi?id=65282
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        No new tests, existing tests should play correctly when system clock
> > +        changed.
> 
> This needs a detailed description. What did you change? Why did you change it? And it is not only important that do not break thinks: It is important how it affects the behavior. You need tests to verify the new behavior, especially what works now but didn't work before. Does your patch affect bug 31256 as well?

I wasn't aware of that bug, but no, it does not appear to be sufficient to fix that bug. This bug should probably be merged with that one. This change is probably not worth pursuing and I'll investigate the root cause of 31256 instead.

I don't have a good idea on how to write an automated test for this behaviour. All of the currentTime() implementations appear to go directly to the system clock. I could try to add support for custom control of currentTime to DRT perhaps? But it seems a bit tortured as the test code will be an implementation of monotonicallyIncreasingTime.

> 
> > Source/WebCore/ChangeLog:11
> > +        * svg/animation/SMILTimeContainer.cpp:
> 
> Is SMILTimeContainer the only file in WebCore/svg that uses currentTime()?

Yes, I believe so.
------- Comment #7 From 2011-07-28 14:42:20 PST -------
(In reply to comment #6)

> > Does your patch affect bug 31256 as well?

> I wasn't aware of that bug, but no, it does not appear to be sufficient to fix that bug.

Scratch that, it *does* appear to fix bug 31256 as tested with http://upload.wikimedia.org/wikipedia/commons/4/4f/Soccer_ball_animated.svg I believe I confused my build system when changing the clock when I first tried.

When using currentTime() when the clock is set back a little bit (~30s) normally the animation will pause until the previous (farther in the future time) is reached. Sometimes though, it will just "glitch" and restart. I'm not sure of the cause of that difference yet. But, when using monotonicallyIncreasingTime() I do not see either of these artifacts when setting the clock back a bit.

If you agree Dirk, I will mark this bug as duplicate of 31256 and upload an improved patch there. And if you have suggestions on how to test I would be grateful.

We should perhaps continue the discussion on AnimationController and AnimationBase in a new bug.
------- Comment #8 From 2011-07-28 17:52:09 PST -------
(In reply to comment #7)

> We should perhaps continue the discussion on AnimationController and AnimationBase in a new bug.

Yeah. AnimationController and Base should move to the monotonic time as well, but that doesn't need to happen here.
------- Comment #9 From 2011-07-28 22:32:42 PST -------
Ok, can you upload a new patch with better change log and a test case?
------- Comment #10 From 2012-02-13 10:45:40 PST -------
Any updates here Scott?
------- Comment #11 From 2012-02-13 11:22:12 PST -------
(In reply to comment #10)
> Any updates here Scott?

No, sorry. I guess to write a test that shows the current code is broken, it would first be necessary to virtualize all the port's currentTime() implementations so that a test could then set it arbitrarily like a user can set system time.