Bug 77229 – [chromium] Implement keyframed animations for the cc thread.

• Component: New Bugs
• Status: RESOLVED FIXED

Subclasses of CCFloatAnimationCurve and CCTransformAnimationCurve need to be implemented. These curves need to be created in CCLayerAnimationController in response to a call to GraphicsLayer::addAnimation and passed to the cc thread animation kernel.

Created attachment 124333 [details]
Patch

Created attachment 126142 [details]
Patch

[Nat Duca]

Comment on attachment 126142 [details]
Patch

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

Unit Tests?

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:37
> +////////////////////////////////////////////////////////////////////////////////

not sure we do this ///////// stuff in the CC code.

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:42
> +{

Ugh, really? Is there a way for this to be less dynamic? E.g. you construct a curve with all its keyframes instead of piecemeal?

Created attachment 126579 [details]
Patch

Created attachment 126589 [details]
Patch

(In reply to comment #3)
> (From update of attachment 126142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126142&action=review
> 
> Unit Tests?
Added.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:37
> > +////////////////////////////////////////////////////////////////////////////////
> 
> not sure we do this ///////// stuff in the CC code.
Removed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:42
> > +{
> 
> Ugh, really? Is there a way for this to be less dynamic? E.g. you construct a curve with all its keyframes instead of piecemeal?
Fixed. They now take keyframes as constructor arguments.

[James Robinson]

This appears to depend on top of https://bugs.webkit.org/show_bug.cgi?id=75874, marking it as dependent (feel free to undo if I've got the relationship incorrect).

[Nat Duca]

Comment on attachment 126589 [details]
Patch

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

Some more comments, sorry.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Remove this line

> Source/WebCore/ChangeLog:63
> +        No new tests. (OOPS!)

Looks like you've duped the changelog somehow.

> Source/WebKit/chromium/ChangeLog:17
> [phone]  Ian Vollick  <[e-mail]>

duped changelog

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:70
> +    const Vector<CCFloatKeyframe>& keyframes() const { return m_keyframes; }

Lets remove this accessor since its not used

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:88
> +    // True if keyframes are sorted and there is at least one keyframe.

Dumb question, should ::create() just validate the keyframes and return nullptr? That way you can't even get an invalid curve in the first place?

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:102
> +    const Vector<CCTransformKeyframe>& keyframes() const { return m_keyframes; }

remove since unused?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:166
> +    if (animation->isDirectionSet() && animation->direction() == Animation::AnimationDirectionAlternate)

can we get some of the common code between the crateXXX shared?

Would this help?
template <class T> bool prepareKeyframes(vector<T>& keyframes);// return false if preparation failed

This could cover lines 163-190 ish, I think.

A similar technique might work for the code after the curve construction, too.

Or you might even be able to reduce this to do a template <KEYFRAME_TYPE, CURVE_TYPE> createAnimation()

Also, its probalby bad that the create function also appends to active animations. It should either return as a PassOwnPtr, or be changed to be more like activateXAnimation

[James Robinson]

Comment on attachment 126589 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:118
> +    OwnPtr<CCActiveAnimation> toReturn(adoptPtr(new CCActiveAnimation(m_animationCurve->clone(), m_name, m_group, m_targetProperty)));

I'm pretty sure this is buggy - you can't simply copy a WTF::String from one thread to another.  I believe you need to use isolatedCopy().

I'm not really thrilled with the idea of using strings cross-thread.  I believe GraphicsLayer uses strings because they map naturally to CoreAnimation's addAnimation: forKey: / removeAnimation: forKey: APIs: https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CoreAnimation_guide/Articles/AnimatingLayers.html#//apple_ref/doc/uid/TP40006085-SW1.  However I think we should think of staying close to that for our main-thread API (LayerChromium, effectively) and something something less foot-gunny in our implementation.

Created attachment 127686 [details]
Patch

Created attachment 127688 [details]
Patch

Created attachment 127689 [details]
Patch

(In reply to comment #8)
> (From update of attachment 126589 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126589&action=review
> 
> Some more comments, sorry.

> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:70
> > +    const Vector<CCFloatKeyframe>& keyframes() const { return m_keyframes; }
> 
> Lets remove this accessor since its not used
>
Done. 
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:88
> > +    // True if keyframes are sorted and there is at least one keyframe.
> 
> Dumb question, should ::create() just validate the keyframes and return nullptr? That way you can't even get an invalid curve in the first place?
> 
Love it. Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:102
> > +    const Vector<CCTransformKeyframe>& keyframes() const { return m_keyframes; }
> 
> remove since unused?
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:166
> > +    if (animation->isDirectionSet() && animation->direction() == Animation::AnimationDirectionAlternate)
> 
> can we get some of the common code between the crateXXX shared?
> 
> Would this help?
> template <class T> bool prepareKeyframes(vector<T>& keyframes);// return false if preparation failed
> 
> This could cover lines 163-190 ish, I think.
> 
> A similar technique might work for the code after the curve construction, too.
> 
> Or you might even be able to reduce this to do a template <KEYFRAME_TYPE, CURVE_TYPE> createAnimation()
> 
Done.
> Also, its probalby bad that the create function also appends to active animations. It should either return as a PassOwnPtr, or be changed to be more like activateXAnimation
Functions have renamed and are, hopefully, clearer.

(In reply to comment #9)
> (From update of attachment 126589 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126589&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:118
> > +    OwnPtr<CCActiveAnimation> toReturn(adoptPtr(new CCActiveAnimation(m_animationCurve->clone(), m_name, m_group, m_targetProperty)));
> 
> I'm pretty sure this is buggy - you can't simply copy a WTF::String from one thread to another.  I believe you need to use isolatedCopy().
> 
> I'm not really thrilled with the idea of using strings cross-thread.  I believe GraphicsLayer uses strings because they map naturally to CoreAnimation's addAnimation: forKey: / removeAnimation: forKey: APIs: https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CoreAnimation_guide/Articles/AnimatingLayers.html#//apple_ref/doc/uid/TP40006085-SW1.  However I think we should think of staying close to that for our main-thread API (LayerChromium, effectively) and something something less foot-gunny in our implementation.

Thank you for catching this. I now pass ids rather than strings. There will need to be a mapping between names and ids, but the mapping cannot be maintained without plumbing back to GraphicsLayerChromium to tell it when animations finish. This will be coming in 77646.

[James Robinson]

(In reply to comment #14)
> (In reply to comment #9)
> > (From update of attachment 126589 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126589&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:118
> > > +    OwnPtr<CCActiveAnimation> toReturn(adoptPtr(new CCActiveAnimation(m_animationCurve->clone(), m_name, m_group, m_targetProperty)));
> > 
> > I'm pretty sure this is buggy - you can't simply copy a WTF::String from one thread to another.  I believe you need to use isolatedCopy().
> > 
> > I'm not really thrilled with the idea of using strings cross-thread.  I believe GraphicsLayer uses strings because they map naturally to CoreAnimation's addAnimation: forKey: / removeAnimation: forKey: APIs: https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CoreAnimation_guide/Articles/AnimatingLayers.html#//apple_ref/doc/uid/TP40006085-SW1.  However I think we should think of staying close to that for our main-thread API (LayerChromium, effectively) and something something less foot-gunny in our implementation.
> 
> Thank you for catching this. I now pass ids rather than strings. There will need to be a mapping between names and ids, but the mapping cannot be maintained without plumbing back to GraphicsLayerChromium to tell it when animations finish. This will be coming in 77646.

That's a good plan but it unfortunately creates a bad dependency cycle in the current patch order.  This patch doesn't appear to apply cleanly without bug 75874 which still uses strings.  I would suggest breaking the cycle in one of two ways:

1.) Modify this patch so that it can apply and be tested independently of bug 75874 - it appears that there are merge conflicts in some test code, but I think logically the bulk of this patch is independent of the way it's tied in to GraphicsLayerChromium. This seems slightly easier to me

2.) Implement whatever part of 77646 is needed for the string switchover as a bug blocking this one and wait for that before going to land this.  That seems less ideal unless it's very difficult to do one.  In general, I think it's better to land pieces of logic independently if they can be well-tested in isolation and we're pretty confident in how the bits will glue together.

Created attachment 128495 [details]
Patch

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #9)
> > > (From update of attachment 126589 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=126589&action=review
> > > 
> > > > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:118
> > > > +    OwnPtr<CCActiveAnimation> toReturn(adoptPtr(new CCActiveAnimation(m_animationCurve->clone(), m_name, m_group, m_targetProperty)));
> > > 
> > > I'm pretty sure this is buggy - you can't simply copy a WTF::String from one thread to another.  I believe you need to use isolatedCopy().
> > > 
> > > I'm not really thrilled with the idea of using strings cross-thread.  I believe GraphicsLayer uses strings because they map naturally to CoreAnimation's addAnimation: forKey: / removeAnimation: forKey: APIs: https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CoreAnimation_guide/Articles/AnimatingLayers.html#//apple_ref/doc/uid/TP40006085-SW1.  However I think we should think of staying close to that for our main-thread API (LayerChromium, effectively) and something something less foot-gunny in our implementation.
> > 
> > Thank you for catching this. I now pass ids rather than strings. There will need to be a mapping between names and ids, but the mapping cannot be maintained without plumbing back to GraphicsLayerChromium to tell it when animations finish. This will be coming in 77646.
> 
> That's a good plan but it unfortunately creates a bad dependency cycle in the current patch order.  This patch doesn't appear to apply cleanly without bug 75874 which still uses strings.  I would suggest breaking the cycle in one of two ways:
> 
> 1.) Modify this patch so that it can apply and be tested independently of bug 75874 - it appears that there are merge conflicts in some test code, but I think logically the bulk of this patch is independent of the way it's tied in to GraphicsLayerChromium. This seems slightly easier to me
> 
> 2.) Implement whatever part of 77646 is needed for the string switchover as a bug blocking this one and wait for that before going to land this.  That seems less ideal unless it's very difficult to do one.  In general, I think it's better to land pieces of logic independently if they can be well-tested in isolation and we're pretty confident in how the bits will glue together.

I think that this has been sorted out -- this patch can now be applied cleanly on master.

[James Robinson]

Comment on attachment 128495 [details]
Patch

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

Looks great R=me.  A few naming nits to address before landing, just to stay consistent.

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:48
> +    CCTransformKeyframe(double time)

nit: explicit

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:40
> +template <typename KEYFRAME, typename VALUE>

style nit (here and elsewhere): we reserve ALLCAPS style for macros in WebKit and typically just use normal type names for template parameters. So this would be "Keyframe" and "Value"

Created attachment 128547 [details]
Patch

(In reply to comment #18)
> (From update of attachment 128495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128495&action=review
> 
> Looks great R=me.  A few naming nits to address before landing, just to stay consistent.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:48
> > +    CCTransformKeyframe(double time)
> 
> nit: explicit
> 
Fixed.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:40
> > +template <typename KEYFRAME, typename VALUE>
> 
> style nit (here and elsewhere): we reserve ALLCAPS style for macros in WebKit and typically just use normal type names for template parameters. So this would be "Keyframe" and "Value"
Fixed. Affected files: CCKeyframedAnimationCurve.cpp, CCAnimationTestCommon.cpp, CCLayerAnimationController.cpp

[WebKit Review Bot]

Comment on attachment 128547 [details]
Patch

Clearing flags on attachment: 128547

Committed r108727: <http://trac.webkit.org/changeset/108727>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.