Bug 186129

Summary: REGRESSION (r232186): Hardware-accelerated CSS animations using steps() timing function no longer work
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: AnimationsAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dino, graouts, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 170784, 186127    
Attachments:
Description Flags
Testcase
none
Testcase
none
Testcase
none
Patch
none
Patch for landing none

Description Frédéric Wang (:fredw) 2018-05-31 01:15:37 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2018-05-31 01:15:54 PDT
Created attachment 341648 [details]
Testcase
Comment 2 Alexey Proskuryakov 2018-05-31 11:15:27 PDT
Started with https://trac.webkit.org/r232186. Perhaps an intentional change?
Comment 3 Antoine Quint 2018-05-31 11:52:47 PDT
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.
Comment 4 Antoine Quint 2018-05-31 11:54:10 PDT
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.
Comment 5 Radar WebKit Bug Importer 2018-05-31 12:00:56 PDT
<rdar://problem/40694333>
Comment 6 Alexey Proskuryakov 2018-05-31 17:03:04 PDT
To be clear, I positively bisected this regression to r232186. That's not a guess.
Comment 7 Frédéric Wang (:fredw) 2018-06-01 01:02:17 PDT
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.
Comment 8 Antoine Quint 2018-06-01 01:51:23 PDT
Animating margin-left instead of transform behaves as expected. This is an issue with hardware-accelerated animations only.
Comment 9 Frédéric Wang (:fredw) 2018-06-01 02:09:19 PDT
(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?
Comment 10 Frédéric Wang (:fredw) 2018-06-18 09:09:26 PDT
Created attachment 342943 [details]
Testcase

Testcase, including a CSS propertie that cannot be accelerated.
Comment 11 Frédéric Wang (:fredw) 2018-06-18 10:25:02 PDT
(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.
Comment 12 Frédéric Wang (:fredw) 2018-07-03 03:16:46 PDT
(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;
Comment 13 Frédéric Wang (:fredw) 2018-07-03 05:49:23 PDT
Created attachment 344178 [details]
Patch
Comment 14 Antoine Quint 2018-07-03 07:09:32 PDT
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.
Comment 15 Frédéric Wang (:fredw) 2018-07-03 07:15:06 PDT
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.
Comment 16 Frédéric Wang (:fredw) 2018-07-03 07:22:08 PDT
Created attachment 344186 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2018-07-03 08:02:01 PDT
Comment on attachment 344186 [details]
Patch for landing

Clearing flags on attachment: 344186

Committed r233462: <https://trac.webkit.org/changeset/233462>
Comment 18 WebKit Commit Bot 2018-07-03 08:02:03 PDT
All reviewed patches have been landed.  Closing bug.