Bug 93146 – Coordinated Graphics: Enable threaded/IPC animations

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

Currently animations work nicely for requestAnimationFrame, however, CSS transitions/animations are blocked when there is a lengthy rendering, layout or JavaScript execution.
By allowing animation frames to be rendered in parallel to normal rendering, we can achieve smoother transitions in some cases.

[Noam Rosenthal]

Created attachment 157349 [details]
WIP patch (not for review)

This is turning out pretty tricky, but possible.
The idea is to allow animations to continue flowing while the main thread in the web process is busy with javascript or software rendering, and to continue to vsync-based message passing, by signaling to the animation thread directly from the CoreIPC::Connection's work queue.

[Dongseong Hwang]

Looks good! This patch makes CSS animation smoother.

However, I have few questions.

1. Why did you decide Web process makes progress animations instead of UI process. If we pass a GraphicsLayer::addAnimation message to UI process, LayerTreeRenderer can make progress animations like PageClientQWidget::syncLayers() or AcceleratedCompositingContext::renderLayersToWindow(). I think the later approach can reuse existing code and reduce animation IPC messages and may make css animations slightly faster than threaded animations.

2. I think you may decide threaded animations in Web process because of accurate callback for requestAnimationFrame. Can't Messages::LayerTreeCoordinator::RenderNextFrame() satisfy the requirements for requestAnimationFrame?

I'm looking forward Coordinated Graphics running animations smoothly regardless of Web process's main thread :)

[Noam Rosenthal]

(In reply to comment #2)
> Looks good! This patch makes CSS animation smoother.
> 
> However, I have few questions.
> 
> 1. Why did you decide Web process makes progress animations instead of UI process. If we pass a GraphicsLayer::addAnimation message to UI process, LayerTreeRenderer can make progress animations like PageClientQWidget::syncLayers() or AcceleratedCompositingContext::renderLayersToWindow(). I think the later approach can reuse existing code and reduce animation IPC messages and may make css animations slightly faster than threaded animations.

Yes, that's they way it was before. The problem (which is solvable, but tricky) is in synchronization.
We've had cases where, for example, an animated layer stops being composited, and then it gets repainted with the rest of the "non-composited" content.
The issue is that if painting the non-composited contents takes too much time, there might be a few extra animation frames in the UI process, which would go beyond the end point of the animation. If we can fix that, we can bring back UI-side animations...

[Dongseong Hwang]

(In reply to comment #3)
> Yes, that's they way it was before. The problem (which is solvable, but tricky) is in synchronization.

Thanks for explanation! I understand.

> We've had cases where, for example, an animated layer stops being composited, and then it gets repainted with the rest of the "non-composited" content.
> The issue is that if painting the non-composited contents takes too much time, there might be a few extra animation frames in the UI process, which would go beyond the end point of the animation. If we can fix that, we can bring back UI-side animations...

I agree that we can get succinct code and relatively fast css animations by threaded animations.

I also understand why chromium compositor has duplicated code to make progress animations in both main thread and impl thread after reading your explanation.
Chromium Compositor(a.k.a CC) delegates impl thread to make progress animations in default (like UI-side animations). The code is in CCLayerTreeHostImpl::animateLayers()
However, When CC must synchronize animations between both thread, main thread makes progress animaitions (like threaded animations). The code is in CCLayerTreeHost::animateLayers()

I prefer threaded animations in now.

[Noam Rosenthal]

> I prefer threaded animations in now.

btw I change my mind often on this, and am not 100% certain myself. I'm working on those two tracks in parallel right now - I might have a solution to the sync problem but I'm not sure it's completely great yet.

[Noam Rosenthal]

Created attachment 169706 [details]
Patch

[Rafael Brandao]

Comment on attachment 169706 [details]
Patch

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

Just giving a quick look and informal review. But I've liked what I've seen so far.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:396
> +    m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, anim, WTF::currentTime() - timeOffset, listsMatch));

I think you should rename "timeOffset" to "startTime" to make it clear (like the other change).

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:519
> +    switch (type)    {

Style: extra spaces between ) and {.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:522
> +    case TimingFunction::CubicBezierFunction: {

Style: I don't think we should use brackets for each case. The same for the other places.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:601
> +    for (size_t i = 0 ; i < keyframes.size(); ++i) {

Style: extra space between 0 and ;.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.cpp:147
> +void LayerTreeCoordinatorProxy::setLayerAnimations(uint32_t id, const GraphicsLayerAnimations& animations)

Instead of uint32_t, you should use WebLayerID (to match its header).

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:417
> +    // in a layer with opacity 0.

Sounds great, but I was wondering if this optimization is a real win (or how bad we get if we just skip it).

[Noam Rosenthal]

Comment on attachment 169706 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:396
>> +    m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, anim, WTF::currentTime() - timeOffset, listsMatch));
> 
> I think you should rename "timeOffset" to "startTime" to make it clear (like the other change).

In this case, though, it is the timeOffset :)

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:522
>> +    case TimingFunction::CubicBezierFunction: {
> 
> Style: I don't think we should use brackets for each case. The same for the other places.

We have to, because we declare new variables.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:417
>> +    // in a layer with opacity 0.
> 
> Sounds great, but I was wondering if this optimization is a real win (or how bad we get if we just skip it).

It's a real win. Issue is the flush itself is where most of the time goes - software rendering. So if we lock animations during software rendering, we don't achieve much...

[Noam Rosenthal]

Created attachment 169716 [details]
Patch

[Dongseong Hwang]

(In reply to comment #8)
> (From update of attachment 169706 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169706&action=review
> >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:417
> >> +    // in a layer with opacity 0.
> > 
> > Sounds great, but I was wondering if this optimization is a real win (or how bad we get if we just skip it).
> 
> It's a real win. Issue is the flush itself is where most of the time goes - software rendering. So if we lock animations during software rendering, we don't achieve much...

I like this approach! I could not imagine this synchronization idea. I have few questions :)

canUnlockBeforeFlush seems to be true for only opacity animation. Can canUnlockBeforeFlush be true for transform and filter animations? I think both animations is regardless of layout.
IMHO we can unlock if layout did not occur.


If we do not unlock here, LayerTreeRenderer::flushLayerChanges() will unlock by itself. There is very small time interval between unlock in LayerTreeRenderer::flushLayerChanges() and lock in next LayerTreeCoordinator::performScheduledLayerFlush().
This means if canUnlockBeforeFlush discourages unlocking for transform animations, this patch can not make transform animations be smoother than previous implementation. Is my understanding right?

[Noam Rosenthal]

> canUnlockBeforeFlush seems to be true for only opacity animation. Can canUnlockBeforeFlush be true for transform and filter animations? I think both animations is regardless of layout.

canUnlockBeforeFlush has nothing to do with the type of animation. It has to do with the current pending changes in the frame - if all the current pending changes in the frame are invisible (are inside a 0-opacity layer, are in a layer that's outside the screen, etc.), it's ok to continue animation while rendering.

> IMHO we can unlock if layout did not occur.
> 
> 
> If we do not unlock here, LayerTreeRenderer::flushLayerChanges() will unlock by itself. There is very small time interval between unlock in LayerTreeRenderer::flushLayerChanges() and lock in next LayerTreeCoordinator::performScheduledLayerFlush().
> This means if canUnlockBeforeFlush discourages unlocking for transform animations, this patch can not make transform animations be smoother than previous implementation. Is my understanding right?

See above :)
This will lock only when there are actual frame changes, not during every transform animation.

[Dongseong Hwang]

(In reply to comment #11)
> canUnlockBeforeFlush has nothing to do with the type of animation. It has to do with the current pending changes in the frame - if all the current pending changes in the frame are invisible (are inside a 0-opacity layer, are in a layer that's outside the screen, etc.), it's ok to continue animation while rendering.

Oops, I misunderstood. Thanks for answering my foolish question :)

[Noam Rosenthal]

(In reply to comment #12)
> (In reply to comment #11)
> > canUnlockBeforeFlush has nothing to do with the type of animation. It has to do with the current pending changes in the frame - if all the current pending changes in the frame are invisible (are inside a 0-opacity layer, are in a layer that's outside the screen, etc.), it's ok to continue animation while rendering.
> 
> Oops, I misunderstood. Thanks for answering my foolish question :)
Always feel free to ask! Your feedback is highly valued and very far from foolish :)

[Rafael Brandao]

Comment on attachment 169716 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:396
> +    m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, anim, WTF::currentTime() - timeOffset, listsMatch));

Also asked on IRC: is it intentional the calc of currentTime() - offset only on addAnimation? What about pauseAnimation and others that use offset? Should them remain unchanged? I'm still a bit confused with the meaning of this "offset" we're passing on each of those functions.

[Noam Rosenthal]

(In reply to comment #14)
> (From update of attachment 169716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169716&action=review
> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:396
> > +    m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, anim, WTF::currentTime() - timeOffset, listsMatch));
> 
> Also asked on IRC: is it intentional the calc of currentTime() - offset only on addAnimation? What about pauseAnimation and others that use offset? Should them remain unchanged? I'm still a bit confused with the meaning of this "offset" we're passing on each of those functions.

In this patch it's just a copy of what was happening before wrt offsets. I think we should fix the readability issues around offsets and pause in another patch :)

[Kenneth Rohde Christiansen]

Comment on attachment 169716 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:48
> +    void setState(AnimationState s, double pauseTime = 0)

pauseTime? when to pause?

> Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:61
> +    bool listsMatch() const { return m_listsMatch; }

I dont understand what this represents

[Noam Rosenthal]

(In reply to comment #16)
> (From update of attachment 169716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169716&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:48
> > +    void setState(AnimationState s, double pauseTime = 0)
> 
> pauseTime? when to pause?
Time when the animation was paused.

> 
> > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:61
> > +    bool listsMatch() const { return m_listsMatch; }
> 
> I dont understand what this represents

This was there before... it's about whether the transform-operation lists for the keyframes match. I can rename in a separate patch.

[Noam Rosenthal]

Should I rename those two things before commiting? I can't think of better names right now...

[Kenneth Rohde Christiansen]

Rename later then

[WebKit Review Bot]

Comment on attachment 169716 [details]
Patch

Clearing flags on attachment: 169716

Committed r132275: <http://trac.webkit.org/changeset/132275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dongseong Hwang]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 169716 [details] [details])
> > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:61
> > > +    bool listsMatch() const { return m_listsMatch; }
> > 
> > I dont understand what this represents
> 
> This was there before... it's about whether the transform-operation lists for the keyframes match. I can rename in a separate patch.

I think listsMatch is better than listIndex :)

bool GraphicsLayerCA::createTransformAnimationsFromKeyframes(const KeyframeValueList& valueList, const Animation* animation, const String& animationName, double timeOffset, const IntSize& boxSize)
{
    ...
    int listIndex = validateTransformOperations(valueList, hasBigRotation);
    ...
}

How about validationFailed?

Anyway, it is landed! very good :)

[Noam Rosenthal]

I think this made the bots cry... trying to fix

[Noam Rosenthal]

Reopening, build breakage from mid-air collision

[Noam Rosenthal]

Created attachment 170279 [details]
Patch for landing

[Noam Rosenthal]

Created attachment 170283 [details]
Followup patch: animations stay locked when they're not supposed to

[WebKit Review Bot]

Comment on attachment 170279 [details]
Patch for landing

Clearing flags on attachment: 170279

Committed r132296: <http://trac.webkit.org/changeset/132296>

[Csaba Osztrogonác]

(In reply to comment #27)
> (From update of attachment 170279 [details])
> Clearing flags on attachment: 170279
> 
> Committed r132296: <http://trac.webkit.org/changeset/132296>

Could you check the tree after landing please next time to avoid
similar breakages? The build was broken for 6 hours, 20 commits and
40 patch stucked in the EWS queue. To find newer and newer regressions
during build breakage isn't a good joke. Fortunately there isn't a new
regression now, but it could have been.

[Noam Rosenthal]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 170279 [details] [details])
> > Clearing flags on attachment: 170279
> > 
> > Committed r132296: <http://trac.webkit.org/changeset/132296>
> 
> Could you check the tree after landing please next time to avoid
> similar breakages? The build was broken for 6 hours, 20 commits and
> 40 patch stucked in the EWS queue. To find newer and newer regressions
> during build breakage isn't a good joke. Fortunately there isn't a new
> regression now, but it could have been.

I checked the tree right away, took an hour to fix it, and it took the cq 4 hours to process the fix. Usually cq is a bit faster but I had an issue with manual landing.

[WebKit Review Bot]

Comment on attachment 170283 [details]
Followup patch: animations stay locked when they're not supposed to

Clearing flags on attachment: 170283

Committed r132392: <http://trac.webkit.org/changeset/132392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jocelyn Turcotte]

The leaves demo is jerky as hell since this patch.
There seem to be a loop where serviceScriptedAnimations ultimately causes a scheduleLayerFlush and this locks/unlocks the animations on every frame.

There could be a way to break the loop if no requestAnimationFrame callback was registered, but that would leave pages using both RAF and CSS animations with jerkiness, which goes against the principle of RAF.

[Noam Rosenthal]

(In reply to comment #32)
> The leaves demo is jerky as hell since this patch.
Hmm.., I definitely tested that one and it looks far from jerky.

> There seem to be a loop where serviceScriptedAnimations ultimately causes a scheduleLayerFlush and this locks/unlocks the animations on every frame.
> 
> There could be a way to break the loop if no requestAnimationFrame callback was registered, but that would leave pages using both RAF and CSS animations with jerkiness, which goes against the principle of RAF.

Thank you, created bug https://bugs.webkit.org/show_bug.cgi?id=100536 for the jerkiness issue.