Bug 81484 – [chromium] Animation events should only be used for synchronizing animation start times

• Component: WebKit Misc.
• Status: RESOLVED FIXED

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.

Created attachment 132534 [details]
Patch

Will post another CL with the ChangeLog changes when 81106 lands.

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.

[Nat Duca]

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

[Adrienne Walker]

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.

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.

[Adrienne Walker]

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.

[WebKit Review Bot]

Comment on attachment 132950 [details]
Patch

Clearing flags on attachment: 132950

Committed r111525: <http://trac.webkit.org/changeset/111525>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.