Bug 65274 – Different elapsed times used during update of animation

• Component: SVG
• Status: RESOLVED FIXED

[Scott Graham]

In some places during SMILTimeContainer and SVGSMILElement, the elapsed time from the fired timer is used to update animations, and in others, elapsed() is recalled. This eventually recalls the client's currentTime() so the animation will be updated with disparate times.

Some more information: https://lists.webkit.org/pipermail/webkit-dev/2011-July/017649.html

[Scott Graham]

Created attachment 102192 [details]
use one consistent elapsed time for animation update

[Simon Fraser (smfr)]

I wonder if we should plug in the same timebase that requestAnimationFrame etc use? Maybe not in this patch, but soon.

[James Robinson]

Yeah, I'd really like to tie in the timebase and the scheduling for this in with requestAnimationFrame.  That would mean changing the 'current time' notion from something internal to SMILTimeContainer to something passed in from the outside on each animation update. I also think it's a good thing for a different patch than this one.

I'm not super familiar with SMIL but doesn't it allow applying transformations in time space (for example, making a container in which time travels twice as fast)?  Is that handled at this level or somewhere else?

[Scott Graham]

I've only just started cozying up with the spec, so I'm not entirely sure if there's sub time warping requirements or not. FWIW, there's a comment in the time interval calculations claiming:

           // No nested time containers in SVG, no need for crazy time space conversions. Phew!

but will need to investigate more for a future requestAnimationFrame patch.

Is there anyone else who should review this patch as is?

[Dirk Schulze]

(In reply to comment #3)
> Yeah, I'd really like to tie in the timebase and the scheduling for this in with requestAnimationFrame.  That would mean changing the 'current time' notion from something internal to SMILTimeContainer to something passed in from the outside on each animation update. I also think it's a good thing for a different patch than this one.
SVG has a time container per SVGSVGElement. And every time container can be paused and continued independent of the other time container. You can set the time of every container independent of the other containers. So we can end up with different time containers with different status per document. We could still use a global timer. But this needs to get considered.

> 
> I'm not super familiar with SMIL but doesn't it allow applying transformations in time space (for example, making a container in which time travels twice as fast)?  Is that handled at this level or somewhere else?

Not sure if SMIL supports that, but SVG Animation doesn't. And we do not need a complete SMIL implementation.

[Eric Seidel (no email)]

Comment on attachment 102192 [details]
use one consistent elapsed time for animation update

View in context: https://bugs.webkit.org/attachment.cgi?id=102192&action=review

> Source/WebCore/ChangeLog:13
> +        No new tests, but existing svg animations should continue to work
> +        correctly, and elapsed() is stable when stepping in debugger.

Is this observable from the web?  Can we test this?

[Scott Graham]

(In reply to comment #6)
> (From update of attachment 102192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102192&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests, but existing svg animations should continue to work
> > +        correctly, and elapsed() is stable when stepping in debugger.
> 
> Is this observable from the web?  Can we test this?

I guess it would be on a heavily loaded machine. I can't think of good way to test automatically in a layout test though.

My primary concern was really debugger single-stepping, but it just seemed wrong to update parts of the same animation with different times.

[Eric Seidel (no email)]

Comment on attachment 102192 [details]
use one consistent elapsed time for animation update

Without some way to tst this, I'm sure this will break again. :(  The change seems OK to me though.  We do this all on the main thread (including running JavaScript) so there is no way this is observable from the web.  Unless there are JS events which are fired synchronously from animations?  But I doubt that.

I'm not sure this change is worth it, for just debugging.

[Nikolas Zimmermann]

This is fixed since r158405, by properly reporting a fixed time, if the animation is paused on a SMILTimeContainer.