Bug 81484 - [chromium] Animation events should only be used for synchronizing animation start times
Summary: [chromium] Animation events should only be used for synchronizing animation s...
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: 81106
Blocks: 79536
  Show dependency treegraph
 
Reported: 2012-03-18 20:25 PDT by vollick
Modified: 2012-03-21 05:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch (26.37 KB, patch)
2012-03-18 20:29 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (29.38 KB, patch)
2012-03-20 08:14 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (31.13 KB, patch)
2012-03-20 19:44 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-03-18 20:25:39 PDT
It is important that animations on the main and impl thread have the same start time. With 81106, we will be ticking animations on the main thread and it is important that the same value be interpolated at time t on either thread.

It is no longer important to set the final values of animations in response to an animation finishing -- animations on the main thread will finish naturally and set their final values.
Comment 1 vollick 2012-03-18 20:29:40 PDT
Created attachment 132534 [details]
Patch

Will post another CL with the ChangeLog changes when 81106 lands.
Comment 2 vollick 2012-03-20 08:14:34 PDT
Created attachment 132827 [details]
Patch

Eliminated animation finished events. Animations will finish naturally on the
main thread and the final values will be set.

Animation started events are now also used to synchronize the main thread
animations' start times with their impl thread equivalents, preventing skew.
Until main thread animations receive their synchronized start times, they will
apply their initial values (i.e., they will be paused at time zero). This
guarantees that we will not jump to the animation's final value on the main
thread while we wait for the synchronized start time.
Comment 3 Nat Duca 2012-03-20 18:35:11 PDT
Comment on attachment 132827 [details]
Patch

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

A thing of beauty. @Enne.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationEvents.cpp:-33
> -CCAnimationEvent::CCAnimationEvent(int layerId, CCActiveAnimation::TargetProperty targetProperty)

No need for the CPP anymore, methinks? Just put the constructor in the .h, c.f. CCLayerTreeHost.h's CCSettings
Comment 4 Adrienne Walker 2012-03-20 19:11:20 PDT
Comment on attachment 132827 [details]
Patch

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

Drive by language question: is monotonic time a synonym for wall clock time here?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:391
> +        if (m_activeAnimations[i]->runState() == CCActiveAnimation::Running && !m_activeAnimations[i]->needsSynchronizedStartTime()) {
>              double trimmed = m_activeAnimations[i]->trimTimeToCurrentIteration(monotonicTime);
>  
> +            // Animation assumes its initial value until it gets the synchronized start time
> +            // from the impl thread and can start ticking.
> +            if (m_activeAnimations[i]->needsSynchronizedStartTime())
> +                trimmed = 0;

I don't understand this second conditional here.  It looks like it will never be true, based on the first conditional.
Comment 5 vollick 2012-03-20 19:44:08 PDT
Created attachment 132950 [details]
Patch

(In reply to comment #3)
CCAnimationEvents.cpp is toast.

(In reply to comment #4)
> (From update of attachment 132827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132827&action=review
>
> Drive by language question: is monotonic time a synonym for wall clock time here?
>
I've been using monotonicTime to refer to times obtained via monotonicallyIncreasingTime() and wallClockTime for times obtained using currentTime()

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:391
> > +        if (m_activeAnimations[i]->runState() == CCActiveAnimation::Running && !m_activeAnimations[i]->needsSynchronizedStartTime()) {
> >              double trimmed = m_activeAnimations[i]->trimTimeToCurrentIteration(monotonicTime);
> >
> > +            // Animation assumes its initial value until it gets the synchronized start time
> > +            // from the impl thread and can start ticking.
> > +            if (m_activeAnimations[i]->needsSynchronizedStartTime())
> > +                trimmed = 0;
>
> I don't understand this second conditional here.  It looks like it will never be true, based on the first conditional.
Good catch. The first conditional was meant to be deleted. Fixed.
Comment 6 Adrienne Walker 2012-03-20 20:59:47 PDT
Comment on attachment 132950 [details]
Patch

Thanks for the language clarification.  I must have misread some code to think that monotonic and wall clock were being interchangeably used.

R=me, mostly due to Nat's review.
Comment 7 WebKit Review Bot 2012-03-21 05:13:51 PDT
Comment on attachment 132950 [details]
Patch

Clearing flags on attachment: 132950

Committed r111525: <http://trac.webkit.org/changeset/111525>
Comment 8 WebKit Review Bot 2012-03-21 05:13:55 PDT
All reviewed patches have been landed.  Closing bug.