Bug 188730 - [Web Animations] Animation with a single keyframe is not accelerated
Summary: [Web Animations] Animation with a single keyframe is not accelerated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-19 07:59 PDT by Antoine Quint
Modified: 2020-07-08 10:22 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.08 KB, patch)
2020-05-15 09:13 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2020-05-15 10:34 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (13.58 KB, patch)
2020-05-15 12:14 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-08-19 07:59:07 PDT
const animation = document.body.appendChild(document.createElement("div")).animate({
    opacity: 0
}, 5000);

The following animation fails because in KeyframeEffectReadOnly::applyPendingAcceleratedActions() the keyframes we pass to RenderLayerBacking::startAnimation() and eventually to GraphicsLayerCA::animationCanBeAccelerated() (via GraphicsLayerCA::addAnimation()) only have a single value and we need at least two values to have a qualifying accelerated animation.
Comment 1 Radar WebKit Bug Importer 2018-08-19 07:59:28 PDT
<rdar://problem/43481113>
Comment 2 Antoine Quint 2020-05-15 09:13:25 PDT
Created attachment 399484 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-05-15 09:31:40 PDT
Comment on attachment 399484 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1583
> +        keyframes = WTFMove(explicitKeyframes);

I think this leaves you with a ref to garbage data.

> Source/WebCore/rendering/style/KeyframeList.cpp:97
> +    int initialSize = size();

auto. size is size_t.

> Source/WebCore/rendering/style/KeyframeList.cpp:102
> +        static StyleRuleKeyframe* zeroPercentKeyframe;
> +        if (!zeroPercentKeyframe) {
> +            zeroPercentKeyframe = &StyleRuleKeyframe::create(MutableStyleProperties::create()).leakRef();
> +            zeroPercentKeyframe->setKey(0);

NeverDestroyed?

> Source/WebCore/rendering/style/KeyframeList.cpp:115
> +        static StyleRuleKeyframe* hundredPercentKeyframe;
> +        if (!hundredPercentKeyframe) {
> +            hundredPercentKeyframe = &StyleRuleKeyframe::create(MutableStyleProperties::create()).leakRef();
> +            hundredPercentKeyframe->setKey(1);
> +        }

Ditto
Comment 4 Antoine Quint 2020-05-15 10:34:05 PDT
Created attachment 399492 [details]
Patch
Comment 5 Antoine Quint 2020-05-15 10:35:10 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 399484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399484&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:1583
> > +        keyframes = WTFMove(explicitKeyframes);
> 
> I think this leaves you with a ref to garbage data.

It was actually setting m_blendingKeyframes, which is not what I intended. I now use a lambda to generate a new KeyframeList only when needed.

> > Source/WebCore/rendering/style/KeyframeList.cpp:97
> > +    int initialSize = size();
> 
> auto. size is size_t.

Fixed in updated patch.

> > Source/WebCore/rendering/style/KeyframeList.cpp:102
> > +        static StyleRuleKeyframe* zeroPercentKeyframe;
> > +        if (!zeroPercentKeyframe) {
> > +            zeroPercentKeyframe = &StyleRuleKeyframe::create(MutableStyleProperties::create()).leakRef();
> > +            zeroPercentKeyframe->setKey(0);
> 
> NeverDestroyed?
> 
> > Source/WebCore/rendering/style/KeyframeList.cpp:115
> > +        static StyleRuleKeyframe* hundredPercentKeyframe;
> > +        if (!hundredPercentKeyframe) {
> > +            hundredPercentKeyframe = &StyleRuleKeyframe::create(MutableStyleProperties::create()).leakRef();
> > +            hundredPercentKeyframe->setKey(1);
> > +        }
> 
> Ditto
Comment 6 Dean Jackson 2020-05-15 10:54:28 PDT
Comment on attachment 399492 [details]
Patch

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

> Source/WebCore/rendering/style/KeyframeList.cpp:81
> +    return size() && (m_keyframes[0].key() || m_keyframes[size() - 1].key() != 1);

Are you sure m_keyframes has values?

> Source/WebCore/rendering/style/KeyframeList.cpp:103
> +        static StyleRuleKeyframe* zeroPercentKeyframe;
> +        if (!zeroPercentKeyframe) {
> +            zeroPercentKeyframe = &StyleRuleKeyframe::create(MutableStyleProperties::create()).leakRef();
> +            zeroPercentKeyframe->setKey(0);
> +        }

Should do the NeverDestroyed thing that smfr suggested.
Comment 7 Antoine Quint 2020-05-15 10:56:07 PDT
(In reply to Dean Jackson from comment #6)
> Comment on attachment 399492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399492&action=review
> 
> > Source/WebCore/rendering/style/KeyframeList.cpp:81
> > +    return size() && (m_keyframes[0].key() || m_keyframes[size() - 1].key() != 1);
> 
> Are you sure m_keyframes has values?

Yes, given size() is true.

> > Source/WebCore/rendering/style/KeyframeList.cpp:103
> > +        static StyleRuleKeyframe* zeroPercentKeyframe;
> > +        if (!zeroPercentKeyframe) {
> > +            zeroPercentKeyframe = &StyleRuleKeyframe::create(MutableStyleProperties::create()).leakRef();
> > +            zeroPercentKeyframe->setKey(0);
> > +        }
> 
> Should do the NeverDestroyed thing that smfr suggested.

Let's try it.
Comment 8 Antoine Quint 2020-05-15 12:14:23 PDT
Created attachment 399501 [details]
Patch
Comment 9 Antoine Quint 2020-05-15 13:07:32 PDT
Committed r261756: <https://trac.webkit.org/changeset/261756>