REGRESSION (r232186): Hardware-accelerated CSS animations using steps() timing function no longer work

[Frédéric Wang (:fredw)]

See the attached testcase. Top square uses Bézier timing function (linear) while bottom square uses steps.

On the GTK port, the two squares are animated. On the iOS/macOS ports on trunk with the latest Xcode update (9.4) the bottom square does not move at all. It used to work a few days ago, so probably it's a recent regression in trunk or in XCode 9.4.

[Frédéric Wang (:fredw)]

Created attachment 341648 [details]
Testcase

[Alexey Proskuryakov]

Started with https://trac.webkit.org/r232186. Perhaps an intentional change?

[Antoine Quint]

r232186 should not affect WebKit unless you have DEFAULT_EXPERIMENTAL_FEATURES_ENABLED set to TRUE. Also, the timing function code is not affected by the Web Animations flags.

[Antoine Quint]

Looks like a bug with the new CSS Animations implementation on top of Web Animations. This does not reproduce with the Web Animations CSS Integration flag off.

[Radar WebKit Bug Importer]

<rdar://problem/40694333>

[Alexey Proskuryakov]

To be clear, I positively bisected this regression to r232186. That's not a guess.

[Frédéric Wang (:fredw)]

Created attachment 341744 [details]
Testcase

This is an extended testcase that logs the getComputedStyle() of the square, showing that the transform is correctly updated in both cases.

[Antoine Quint]

Animating margin-left instead of transform behaves as expected. This is an issue with hardware-accelerated animations only.

[Frédéric Wang (:fredw)]

(In reply to Antoine Quint from comment #8)
> Animating margin-left instead of transform behaves as expected. This is an
> issue with hardware-accelerated animations only.

Also, note that GraphicsLayerCA::animationCanBeAccelerated returns false for the steps() timing function (which is the case failing in the attached testcase). So maybe the implementation with webAnimationCSSIntegration enabled does not handle transform for non-accelerated animations?

[Frédéric Wang (:fredw)]

Created attachment 342943 [details]
Testcase

Testcase, including a CSS propertie that cannot be accelerated.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #9)
> (In reply to Antoine Quint from comment #8)
> > Animating margin-left instead of transform behaves as expected. This is an
> > issue with hardware-accelerated animations only.
> 
> Also, note that GraphicsLayerCA::animationCanBeAccelerated returns false for
> the steps() timing function (which is the case failing in the attached
> testcase). So maybe the implementation with webAnimationCSSIntegration
> enabled does not handle transform for non-accelerated animations?

Comparing the stack traces, webAnimationCSSIntegration disabled gives:

WebCore::GraphicsLayerCA::animationCanBeAccelerated
WebCore::GraphicsLayerCA::addAnimation
WebCore::RenderLayerBacking::startAnimation
WebCore::RenderBoxModelObject::startAnimation
WebCore::KeyframeAnimation::startAnimation
WebCore::AnimationBase::updateStateMachine
...

while webAnimationCSSIntegration enabled gives:

WebCore::GraphicsLayerCA::animationCanBeAccelerated
WebCore::GraphicsLayerCA::addAnimation
WebCore::RenderLayerBacking::startAnimation
WebCore::RenderBoxModelObject::startAnimation
WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions
WebCore::WebAnimation::applyPendingAcceleratedActions
...

Note that in the former case, the AnimationBase becomes non-accelerated when startAnimation returns false (e.g. when animationCanBeAccelerated returns false) but in the latter case, the WebAnimation just ignores the boolean returned by startAnimation. It looks like it should somewhat fallback to non-accelerated mode too.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #11)
> Note that in the former case, the AnimationBase becomes non-accelerated when
> startAnimation returns false (e.g. when animationCanBeAccelerated returns
> false) but in the latter case, the WebAnimation just ignores the boolean
> returned by startAnimation. It looks like it should somewhat fallback to
> non-accelerated mode too.

OK, just checked again this morning and indeed this seems to be enough:

--- a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
+++ b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
@@ -1256,3 +1256,6 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
         case AcceleratedAction::Play:
-            compositedRenderer->startAnimation(timeOffset, backingAnimationForCompositedRenderer().ptr(), m_blendingKeyframes);
+            if (!compositedRenderer->startAnimation(timeOffset, backingAnimationForCompositedRenderer().ptr(), m_blendingKeyframes)) {
+                m_shouldRunAccelerated = false;
+                m_lastRecordedAcceleratedAction = AcceleratedAction::Stop;
+            }
             break;

[Frédéric Wang (:fredw)]

Created attachment 344178 [details]
Patch

[Antoine Quint]

Comment on attachment 344178 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1259
> +                m_lastRecordedAcceleratedAction = AcceleratedAction::Stop;

I think we should clear the list of pending accelerated actions and return here since we already know we can't actually run accelerated.

[Frédéric Wang (:fredw)]

Comment on attachment 344178 [details]
Patch

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

>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1259
>> +                m_lastRecordedAcceleratedAction = AcceleratedAction::Stop;
> 
> I think we should clear the list of pending accelerated actions and return here since we already know we can't actually run accelerated.

Right, we can just return immediately. m_pendingAcceleratedActions.clear(); is already called at the beginning of the function.

[Frédéric Wang (:fredw)]

Created attachment 344186 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344186 [details]
Patch for landing

Clearing flags on attachment: 344186

Committed r233462: <https://trac.webkit.org/changeset/233462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.