Bug 84601 - [chromium] Add pause and resume support for accelerated css animations.
Summary: [chromium] Add pause and resume support for accelerated css animations.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 08:46 PDT by vollick
Modified: 2012-04-27 17:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.06 KB, patch)
2012-04-23 08:56 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (15.48 KB, patch)
2012-04-23 10:07 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (18.99 KB, patch)
2012-04-25 08:54 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (31.91 KB, patch)
2012-04-27 13:08 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (31.59 KB, patch)
2012-04-27 13:22 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-04-23 08:46:06 PDT
Need to implement suspendAnimations and resumeAnimations and make sure that the run state changes make it to the impl thread. This will be important for testing.
Comment 1 vollick 2012-04-23 08:56:39 PDT
Created attachment 138357 [details]
Patch
Comment 2 Dana Jansens 2012-04-23 10:01:14 PDT
Comment on attachment 138357 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626
>  void LayerChromium::suspendAnimations(double time)

Can you name the 'time' variable more descriptively, or otherwise make it more clear what the variable's base is? Or maybe the conversion below is more obvious to others
Comment 3 vollick 2012-04-23 10:07:17 PDT
Created attachment 138371 [details]
Patch

(In reply to comment #2)
> (From update of attachment 138357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138357&action=review
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626
> >  void LayerChromium::suspendAnimations(double time)
>
> Can you name the 'time' variable more descriptively, or otherwise make it more clear what the variable's base is? Or maybe the conversion below is more obvious to others

Good point! I've renamed the parameters.
Comment 4 James Robinson 2012-04-24 23:45:02 PDT
Comment on attachment 138371 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626
> +void LayerChromium::suspendAnimations(double wallClockTime)

why does this take a wall clock time? who's passing this in to us?

if this is API we control, it should be using monotonicallyIncreasingTime. if this is being fed in by WebCore, i'd prefer to handle the time rebasing before it gets in to LayerChromium so we don't have to muck with it there

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:137
> +    // Currently, we only push changes due to pausing and resuming animations on the main thread.

could you explain why it's OK to delete the code that used to live here?
Comment 5 vollick 2012-04-25 08:54:44 PDT
Created attachment 138820 [details]
Patch

(In reply to comment #4)
> (From update of attachment 138371 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138371&action=review
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626
> > +void LayerChromium::suspendAnimations(double wallClockTime)
>
> why does this take a wall clock time? who's passing this in to us?
>
> if this is API we control, it should be using monotonicallyIncreasingTime. if this is being fed in by WebCore, i'd prefer to handle the time rebasing before it gets in to LayerChromium so we don't have to muck with it there
>
The time comes from WebCore. I've moved the rebasing out to GraphicsLayerChromium.

> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:137
> > +    // Currently, we only push changes due to pausing and resuming animations on the main thread.
>
> could you explain why it's OK to delete the code that used to live here?
The old code is never called. I've included its removal in several cl's but none of them have landed yet.
Comment 6 vollick 2012-04-27 13:08:40 PDT
Created attachment 139253 [details]
Patch

Incorporates fixes for bugs I've found getting threaded animation in DRT.

Noteworthy changes
 - CCActiveAnimations need to keep track of their time offset. This is used when resuming animations.
 - Suspend and Resume must cause the animation to pause at the given time and prevent any run state changes.
Comment 7 James Robinson 2012-04-27 13:12:57 PDT
Comment on attachment 139253 [details]
Patch

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

Great, R=me.  Thanks for moving the time rebase - I'd like to keep as much of the compositor interface monotonic time as possible and keep WebCore-specific fiddly bits in GLC

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:48
> +#include <wtf/CurrentTime.h>
> +

can kill this now

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:145
> +    // there is absolutely no need for clients of this controller to kmonotonicTime

typo - looks like a bad search->replace of "now"->"monotonicTime"
Comment 8 vollick 2012-04-27 13:22:13 PDT
Created attachment 139257 [details]
Patch

(In reply to comment #7)
> (From update of attachment 139253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139253&action=review
>
> Great, R=me.  Thanks for moving the time rebase - I'd like to keep as much of the compositor interface monotonic time as possible and keep WebCore-specific fiddly bits in GLC
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:48
> > +#include <wtf/CurrentTime.h>
> > +
>
> can kill this now
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:145
> > +    // there is absolutely no need for clients of this controller to kmonotonicTime
>
> typo - looks like a bad search->replace of "now"->"monotonicTime"
D'oh. Fixed.
Comment 9 WebKit Review Bot 2012-04-27 17:07:05 PDT
Comment on attachment 139257 [details]
Patch

Clearing flags on attachment: 139257

Committed r115519: <http://trac.webkit.org/changeset/115519>
Comment 10 WebKit Review Bot 2012-04-27 17:07:13 PDT
All reviewed patches have been landed.  Closing bug.