Bug 188730 – [Web Animations] Animation with a single keyframe is not accelerated

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/43481113>

[Antoine Quint]

Created attachment 399484 [details]
Patch

[Simon Fraser (smfr)]

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

[Antoine Quint]

Created attachment 399492 [details]
Patch

[Antoine Quint]

(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

[Dean Jackson]

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.

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 399501 [details]
Patch

[Antoine Quint]

Committed r261756: <https://trac.webkit.org/changeset/261756>