Bug 218655 - Reversed transform animation not applied alongside other transform animations
Summary: Reversed transform animation not applied alongside other transform animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-06 05:05 PST by Antoine Quint
Modified: 2021-04-01 11:56 PDT (History)
3 users (show)

See Also:


Attachments
Test (305 bytes, text/html)
2020-11-06 05:05 PST, Antoine Quint
no flags Details
Patch (26.91 KB, patch)
2021-01-14 08:55 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (28.19 KB, patch)
2021-01-15 00:14 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (29.14 KB, patch)
2021-01-15 02:38 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-11-06 05:05:56 PST
Created attachment 413425 [details]
Test

Consider this example:

div {
    width: 100px;
    height: 100px;
    margin-left: 100px;
    margin-top: 100px;
    background-color: black;

    animation: scale 2s forwards, rotate 500ms 1s reverse;
}

@keyframes scale {
    50% { scale: 2 }
}

@keyframes rotate {
    100% { rotate: 90deg }
}

In Safari, the rotate animation won't be performed because it's reversed and we disable accelerated animations when the playback rate is not 1 (see bug 211839). We should ensure that we only run accelerated transform animations if all of the accelerated transform animations can be accelerated.
Comment 1 Radar WebKit Bug Importer 2020-11-06 05:06:16 PST
<rdar://problem/71116284>
Comment 2 Antoine Quint 2020-11-06 05:16:57 PST
We should also test we switch to software mode if already accelerated and a non-accelerated animation is added.
Comment 3 Antoine Quint 2021-01-14 07:22:14 PST
I have the case where we try to add an animation that we think should be accelerated but fails to run accelerated due to the internals of GraphicsLayerCA working fine.

I also need to handle the case where we determine the animation cannot be run accelerated before even involving GraphicsLayerCA, such as a steps() timing function.
Comment 4 Antoine Quint 2021-01-14 08:55:46 PST
Created attachment 417627 [details]
Patch
Comment 5 Simon Fraser (smfr) 2021-01-14 09:29:20 PST
Comment on attachment 417627 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:443
> +    m_isApplyingPendingAcceleratedAnimations = true;

SetForScope<>

> Source/WebCore/animation/DocumentTimeline.cpp:466
> +        if (keyframeEffect.failedToRunAcceleratedTransformRelatedAnimation()) {

It's a bit weird to have the very specific failedToRunAcceleratedTransformRelatedAnimation() function. Could applyPendingAcceleratedActions() instead return an OptionSet<> of what it did?

> Source/WebCore/animation/KeyframeEffect.cpp:1773
> +    if (downcast<DocumentTimeline>(animation()->timeline())->isApplyingPendingAcceleratedAnimations()) {

Not a fan of code like this that needs to know what's on its call stack.
Comment 6 Antoine Quint 2021-01-14 12:53:39 PST
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 417627 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417627&action=review
> 
> > Source/WebCore/animation/DocumentTimeline.cpp:466
> > +        if (keyframeEffect.failedToRunAcceleratedTransformRelatedAnimation()) {
> 
> It's a bit weird to have the very specific
> failedToRunAcceleratedTransformRelatedAnimation() function. Could
> applyPendingAcceleratedActions() instead return an OptionSet<> of what it
> did?

Yes, I'll do this.

> > Source/WebCore/animation/KeyframeEffect.cpp:1773
> > +    if (downcast<DocumentTimeline>(animation()->timeline())->isApplyingPendingAcceleratedAnimations()) {
> 
> Not a fan of code like this that needs to know what's on its call stack.

Me neither actually, but was a bit lazy there I confess. I'll make it so that a parameter is passed down to effectFailedToRunAcceleratedTransformRelatedAnimation() to indicate whether the change should be made synchronously or as part of an accelerated action.
Comment 7 Antoine Quint 2021-01-15 00:14:12 PST
Created attachment 417682 [details]
Patch
Comment 8 Antoine Quint 2021-01-15 02:38:54 PST
Created attachment 417690 [details]
Patch
Comment 9 EWS 2021-01-15 09:47:23 PST
Committed r271524: <https://trac.webkit.org/changeset/271524>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417690 [details].