Bug 65274

Summary: Different elapsed times used during update of animation
Product: WebKit Reporter: Scott Graham <scottmg>
Component: SVGAssignee: Scott Graham <scottmg>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, eric, jamesr, koivisto, krit, simon.fraser, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 41761    
Attachments:
Description Flags
use one consistent elapsed time for animation update eric: review-

Description Scott Graham 2011-07-27 12:26:32 PDT
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
Comment 1 Scott Graham 2011-07-27 15:16:13 PDT
Created attachment 102192 [details]
use one consistent elapsed time for animation update
Comment 2 Simon Fraser (smfr) 2011-07-27 17:52:31 PDT
I wonder if we should plug in the same timebase that requestAnimationFrame etc use? Maybe not in this patch, but soon.
Comment 3 James Robinson 2011-07-27 18:35:10 PDT
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?
Comment 4 Scott Graham 2011-07-27 21:07:26 PDT
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?
Comment 5 Dirk Schulze 2011-07-28 00:04:28 PDT
(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.
Comment 6 Eric Seidel (no email) 2011-12-19 10:51:26 PST
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?
Comment 7 Scott Graham 2011-12-19 14:16:24 PST
(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.
Comment 8 Eric Seidel (no email) 2011-12-21 11:48:26 PST
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.
Comment 9 Nikolas Zimmermann 2019-09-02 02:39:10 PDT
This is fixed since r158405, by properly reporting a fixed time, if the animation is paused on a SMILTimeContainer.