Bug 65282 - SMILTimeContainer should use monotonicallyIncreasingTime() rather than currentTime() for animations
: SMILTimeContainer should use monotonicallyIncreasingTime() rather than curren...
Status: UNCONFIRMED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Scott Graham
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-27 13:59 PDT by Scott Graham
Modified: 2012-02-13 11:22 PST (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Graham 2011-07-27 13:59:54 PDT
It appears it should be using monotonicallyIncreasingTime to handle changing system clocks while animating.
Comment 1 Scott Graham 2011-07-27 14:06:39 PDT
Created attachment 102181 [details]
use monotonicallyIncreasingTime rather than currentTime to update animations
Comment 2 WebKit Review Bot 2011-07-27 14:09:54 PDT
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 Scott Graham 2011-07-27 14:12:08 PDT
Created attachment 102183 [details]
use monotonicallyIncreasingTime rather than currentTime to update animations
Comment 4 Dirk Schulze 2011-07-27 23:46:13 PDT
Comment on attachment 102183 [details]
use monotonicallyIncreasingTime rather than currentTime to update animations

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 Dirk Schulze 2011-07-27 23:49:38 PDT
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 Scott Graham 2011-07-28 11:20:04 PDT
(In reply to comment #4)
> (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?

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 Scott Graham 2011-07-28 14:42:20 PDT
(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 Dean Jackson 2011-07-28 17:52:09 PDT
(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 Dirk Schulze 2011-07-28 22:32:42 PDT
Ok, can you upload a new patch with better change log and a test case?
Comment 10 Nikolas Zimmermann 2012-02-13 10:45:40 PST
Any updates here Scott?
Comment 11 Scott Graham 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.