Bug 123170 – Keyframe animations do multiple style recalcs before starting

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Animations

[Ralph T]

I noticed on a slow ARM machine that starting a keyframe animation takes longer than starting a transition. Upon inspection it appears that:

 1. Transitions emit an animation to the compositor/GraphicsLayer in the first style recalc (a small recalc caused by the style mutation).
 2. The compositor manages to flush pending layer changes before the animation timer fires and causes other (full) style recalcs.

(1) is caused by ImplicitAnimation::animate calling AnimationBase::fireAnimationEventsIfNeeded after doing its work (instead of before like KeyframeAnimation::animate does). This causes the animation state machine to transition to AnimationStateStartWaitStyleAvailable inside the first small "mutate" style recalc. If I make KeyframeAnimation::animate call fireAnimationEventsIfNeeded at the end (and on the early returns too) then a keyframe animation will emit an animation to the compositor on the first "mutate" recalc too.

(2) is caused by ImplicitAnimation::endAnimation calling RenderLayerBacking::transitionFinished which calls GraphicsLayer::removeAnimation. On my port, this causes the layer flush timer to be enqueued, even if no animation was actually removed. This happens early (when the animation is created by the first recalc) so this timer gets serviced before any of the animation timers, which cause more slow recalcs. If I make KeyframeAnimation do something to schedule a layer flush then my animation gets started by the UIProcess before the slow recalcs.

I'll add a patch in a minute that does both (1) and (2) but I feel like it would be nice if (2) was more explicit (i.e.: we force a layer flush before doing the animation timer, or we have something more structured than posting timers so we can enforce a more efficient ordering). 

Independently I noticed that if I don't make the AnimationController perform style invals then everything still works except the animation/transition events. I'm not sure why the events need style recalcs, since the StyleResolver already knows to ask the AnimationController for computed styles on animated elements. We might have an opportunity to optimize away a lot of these full inval/recalcs!

[Ralph T]

Created attachment 214906 [details]
Patch

[Ralph T]

I don't like the way a compositor layer flush is scheduled in this change -- for transitions it happens by calling transitionFinished when the animation is created (the state transitions from New -> New). I'd love to hear alternative suggestions :).

[Simon Fraser (smfr)]

Comment on attachment 214906 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        (WebCore::KeyframeAnimation::animate):
> +        Reorder advancing the state machine and calling AnimationBase::fireAnimationEventsIfNeeded. This allows AnimationBase's state machine to advance past WaitForStyle after the first recalc. This order matches what ImplicitAnimation::animate does for transitions.

If this is a behavior change, you should be able to make a testcase for it.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:69
> +#if USE(ACCELERATED_COMPOSITING)
> +    // Force the compositor to schedule a layer flush here so that it occurs before the
> +    // animation timer, which will be scheduled later (but after the animation has been
> +    // added to the layer in the composited case).
> +    if (m_object && toRenderBoxModelObject(m_object)->layer())
> +        toRenderBoxModelObject(m_object)->layer()->compositor().scheduleLayerFlush(false);
> +#endif

You can't just do this for all animations, and this seems wrong. We'll schedule a layer flush when we push the animations to the GraphicsLayerCA, which probably happens in the same runloop anyway.

[Ralph T]

(In reply to comment #3)

Thanks for the review, Simon!

> > Source/WebCore/ChangeLog:16
> > +        (WebCore::KeyframeAnimation::animate):
> > +        Reorder advancing the state machine and calling AnimationBase::fireAnimationEventsIfNeeded. This allows AnimationBase's state machine to advance past WaitForStyle after the first recalc. This order matches what ImplicitAnimation::animate does for transitions.
> 
> If this is a behavior change, you should be able to make a testcase for it.

I didn't expect there to be any page-visible behavior change as a result -- I'm not sure if I can observe style recalcs in a layout test.

You mentioned that it might break accel. and non-accel animations started at the same time. I didn't realize that WebKit waited to start non-accelerated animations until we had received the notification that an accelerated animation had been started (I assume that's what you meant). I'll do a test locally where I delay the compositor sending the animation started signal and see what happens with and without the change.

> > Source/WebCore/page/animation/KeyframeAnimation.cpp:69
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    // Force the compositor to schedule a layer flush here so that it occurs before the
> > +    // animation timer, which will be scheduled later (but after the animation has been
> > +    // added to the layer in the composited case).
> > +    if (m_object && toRenderBoxModelObject(m_object)->layer())
> > +        toRenderBoxModelObject(m_object)->layer()->compositor().scheduleLayerFlush(false);
> > +#endif
> 
> You can't just do this for all animations, and this seems wrong. We'll schedule a layer flush when we push the animations to the GraphicsLayerCA, which probably happens in the same runloop anyway.

Yeah, this is a bad hack. The layer flush is scheduled when the animation is added to the underlying GraphicsLayer implementation. In CoordinatedGraphics this uses a zero-delay timer, just like AnimationController and everything else, so the flush happens after the big nasty recalcs. I see that CA uses a higher priority native timer mechanism to perform layer flushes -- so maybe I can just use a higher priority glib timeout and jump the queue -- then this part of the patch is totally unnecessary!

I'll do the accel. vs non-accel test and update the patch without the layer flush assuming using a higher priority timer doesn't break anything.

[Ralph T]

Created attachment 215010 [details]
Patch

[Ralph T]

I switched to using a layer flush mechanism based on my runloop (Glib for me), more like mac, and now I don't need an explicit flush to get animations to the UIProcess early.

The remaining change causes a new keyframe animation to be emitted to the GraphicsLayer on the first style recalc rather than the second.

I validated that it doesn't break accelerated and unaccelerated animations by commenting out the call from the compositor to GraphicsLayerClient::notifyAnimationStarted -- the consequence of this was that the unaccelerated animation never started as expected and this behavior was the same with or without the patch (and neither generated a webkitAnimationStart event).

I don't believe there is any behavior change caused by this patch except that the animation is emitted earlier. All of the slow animation LayoutTests pass. What else can I test/validate?

[Darin Adler]

Comment on attachment 215010 [details]
Patch

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

Looks like a good change to me.

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

Is a test possible? If not, please replace this with an explanation of why not. If a test is possible, please include it in the patch.

Can’t land a patch with this OOPS in it; the Subversion pre-commit hook will reject it.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:39
> +#include "RenderLayer.h"
> +#include "RenderLayerCompositor.h"

Should not add these includes.

[Ralph T]

Created attachment 215083 [details]
Patch

[Ralph T]

(In reply to comment #7)
Thanks for the review, Darin!

> > Source/WebCore/ChangeLog:13
> > +        No new tests (OOPS!).
> 
> Is a test possible? If not, please replace this with an explanation of why not. If a test is possible, please include it in the patch.

Ok, updated -- there's no change in behavior that can be observed from a test.

> > Source/WebCore/page/animation/KeyframeAnimation.cpp:39
> > +#include "RenderLayer.h"
> > +#include "RenderLayerCompositor.h"
> 
> Should not add these includes.

Oops! Removed.

If this is good for an r+, then please also cq+ since I can't commit myself.

[Simon Fraser (smfr)]

Comment on attachment 215083 [details]
Patch

Pretty sure that the later synthetic recalc was to give time for hardware animations to start (which you can only test on Mac).

[Ralph T]

Actually, please don't cq+. I chatted with Simon on IRC who said I should test on mac before this change gets committed. I'll have a mac later this week or early next week and I'll test then and report back.

[Andreas Kling]

(In reply to comment #11)
> Actually, please don't cq+. I chatted with Simon on IRC who said I should test on mac before this change gets committed. I'll have a mac later this week or early next week and I'll test then and report back.

Any updates on this?

[Ralph T]

Apologies for not following up. I tested this patch on mac and manually went through all of the animations tests and validated that they looked the same before and after (they all have the expected results with the patch).

I wanted to prevent the compositor from calling GraphicsLayerClient::notifyAnimationStarted (break it so it never gets called) and validate that when an accelerated animation and an unaccelerated animation are discovered in the same style recalc that the unaccelerated one doesn't start -- I think this would satisfy Simon's concerns about accelerated vs. unaccelerated start times being coordinated. I did do this test on my own port, which has a compositor that supports accelerated animations in the same way mac does and it was fine (the unaccelerated animation didn't start).

I'll try to build against trunk on mac today and perform the above test. We could make this behavior testable by letting internals enable/disable the compositor calling nofifyAnimationStarted (or making it a no-op internally). Would this be a good contribution?

[Ralph T]

I built against trunk and validated that when GraphicsLayerCA::notifyAnimationStarted is a no-op, unaccelerated animations that were introduced in the same style recalc as the accelerated animation don't start.

This means that the synchronized starting of accelerated and unaccelerated animations is unaffected by this patch.

Simon, are there any other tests I can do to ensure I'm not regressing anything?

Also, do you think it'd be worth exposing disabling notifyAnimationStarted to internals so that we can test this synchronized start behavior? (I think that would be unrelated to this patch, however).

[Brady Eidson]

Comment on attachment 215083 [details]
Patch

Assuming that patches for review since 2013 are stale, r-

[Ahmad Saleem]

It does not have any test to confirm whether this issue exist in newer animation code.

The code this patch was modifying is gone: https://github.com/WebKit/WebKit/commit/15433766d180265ef8953942e2ec0d23eb7d8f52

@Antoine - anything to salvage here or we can close this?

[Antoine Quint]

We can close this.