Bug 65282 – SMILTimeContainer should use monotonicallyIncreasingTime() rather than currentTime() for animations

• Component: SVG
• Status: RESOLVED FIXED

[Scott Graham]

It appears it should be using monotonicallyIncreasingTime to handle changing system clocks while animating.

[Scott Graham]

Created attachment 102181 [details]
use monotonicallyIncreasingTime rather than currentTime to update animations

[WebKit Review Bot]

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.

[Scott Graham]

Created attachment 102183 [details]
use monotonicallyIncreasingTime rather than currentTime to update animations

[Dirk Schulze]

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()?

[Dirk Schulze]

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.

[Scott Graham]

(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.

[Scott Graham]

(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.

[Dean Jackson]

(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.

[Dirk Schulze]

Ok, can you upload a new patch with better change log and a test case?

[Nikolas Zimmermann]

Any updates here Scott?

[Scott Graham]

(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.

[Nikolas Zimmermann]

SMILTimeContainer and the CSS animation/transition code path uses MonoticTime::now(), which superseded monotonicallyIncreasingTime() since 27.08.2013 (r154706).