| Summary: | [Web Animations] Animation with a single keyframe is not accelerated | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||
| Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | changseok, dino, esprehn+autocc, ews-watchlist, glenn, graouts, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | Safari Technology Preview | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=214088 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 399484 [details]
Patch
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 Created attachment 399492 [details]
Patch
(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 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. (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. Created attachment 399501 [details]
Patch
Committed r261756: <https://trac.webkit.org/changeset/261756> |
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.