Changeset 231840 in webkit

Timestamp:

May 16, 2018 1:27:54 AM (6 years ago)

Author:

graouts@webkit.org

Message:

REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
​https://bugs.webkit.org/show_bug.cgi?id=185299
<rdar://problem/39630230>

Reviewed by Simon Fraser.

Source/WebCore:

In r230574, the fix for webkit.org/b/184518, we changed the processing order in GraphicsLayerCA::updateAnimations() to first
process m_uncomittedAnimations and then m_animationsToProcess, so we are guaranteed animations exist before we attempt to pause
or seek them. This broke interrupting and resuming hardware animations (such as an interrupted CSS Transition or an animation
running in a non-visible tab) since a pause operation recorded _before_ an animation was added would be paused anyway since
the animation was now first added, and then paused. The fix is simply to clear any pending AnimationProcessingAction for a
newly-uncommitted animation.

Test: transitions/interrupted-transition-hardware.html

• platform/graphics/ca/GraphicsLayerCA.cpp:

(WebCore::GraphicsLayerCA::createAnimationFromKeyframes):
(WebCore::GraphicsLayerCA::appendToUncommittedAnimations):
(WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):

• platform/graphics/ca/GraphicsLayerCA.h:

(WebCore::GraphicsLayerCA::LayerPropertyAnimation::LayerPropertyAnimation):

LayoutTests:

Add a new test where we interrupt a transition and check that upon returning to the original value,
an animated value is still used and not the initial value. This test fails prior to this patch.

• transitions/interrupted-transition-hardware-expected.html: Added.
• transitions/interrupted-transition-hardware.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/transitions/interrupted-transition-hardware-expected.html (added)
• LayoutTests/transitions/interrupted-transition-hardware.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (modified) (3 diffs)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-05-16  Antoine Quint  <graouts@apple.com>
+
+        REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
+        https://bugs.webkit.org/show_bug.cgi?id=185299
+        <rdar://problem/39630230>
+
+        Reviewed by Simon Fraser.
+
+        Add a new test where we interrupt a transition and check that upon returning to the original value,
+        an animated value is still used and not the initial value. This test fails prior to this patch.
+
+        * transitions/interrupted-transition-hardware-expected.html: Added.
+        * transitions/interrupted-transition-hardware.html: Added.
+
 2018-05-15  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-05-16  Antoine Quint  <graouts@apple.com>
+
+        REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
+        https://bugs.webkit.org/show_bug.cgi?id=185299
+        <rdar://problem/39630230>
+
+        Reviewed by Simon Fraser.
+
+        In r230574, the fix for webkit.org/b/184518, we changed the processing order in GraphicsLayerCA::updateAnimations() to first
+        process m_uncomittedAnimations and then m_animationsToProcess, so we are guaranteed animations exist before we attempt to pause
+        or seek them. This broke interrupting and resuming hardware animations (such as an interrupted CSS Transition or an animation
+        running in a non-visible tab) since a pause operation recorded _before_ an animation was added would be paused anyway since
+        the animation was now first added, and then paused. The fix is simply to clear any pending AnimationProcessingAction for a
+        newly-uncommitted animation.
+
+        Test: transitions/interrupted-transition-hardware.html
+
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::GraphicsLayerCA::createAnimationFromKeyframes):
+        (WebCore::GraphicsLayerCA::appendToUncommittedAnimations):
+        (WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):
+        * platform/graphics/ca/GraphicsLayerCA.h:
+        (WebCore::GraphicsLayerCA::LayerPropertyAnimation::LayerPropertyAnimation):
+
 2018-05-15  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

@@ -3016 +3016 @@
         return false;
 
-    m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
+    appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
 
     return true;
@@ -3043 +3043 @@
         return false;
 
-    m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
+    appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
     return true;
 }
@@ -3105 +3105 @@
         ASSERT(valuesOK);
 
-        m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, internalFilterPropertyIndex, timeOffset));
+        appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, internalFilterPropertyIndex, timeOffset));
     }
 
     return true;
+}
+
+void GraphicsLayerCA::appendToUncommittedAnimations(LayerPropertyAnimation&& animation)
+{
+    // Since we're adding a new animation, make sure we clear any pending AnimationProcessingAction for this animation
+    // as these are applied after we've committed new animations.
+    m_animationsToProcess.remove(animation.m_name);
+
+    m_uncomittedAnimations.append(WTFMove(animation));
 }
 

trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h

@@ -460 +460 @@
     }
 
+    // This represents the animation of a single property. There may be multiple transform animations for
+    // a single transition or keyframe animation, so index is used to distinguish these.
+    struct LayerPropertyAnimation {
+        LayerPropertyAnimation(Ref<PlatformCAAnimation>&& caAnimation, const String& animationName, AnimatedPropertyID property, int index, int subIndex, Seconds timeOffset)
+            : m_animation(WTFMove(caAnimation))
+            , m_name(animationName)
+            , m_property(property)
+            , m_index(index)
+            , m_subIndex(subIndex)
+            , m_timeOffset(timeOffset)
+        { }
+
+        RefPtr<PlatformCAAnimation> m_animation;
+        String m_name;
+        AnimatedPropertyID m_property;
+        int m_index;
+        int m_subIndex;
+        Seconds m_timeOffset;
+    };
+
     bool appendToUncommittedAnimations(const KeyframeValueList&, const TransformOperations*, const Animation*, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool isMatrixAnimation);
     bool appendToUncommittedAnimations(const KeyframeValueList&, const FilterOperation*, const Animation*, const String& animationName, int animationIndex, Seconds timeOffset);
+    void appendToUncommittedAnimations(LayerPropertyAnimation&&);
 
     enum LayerChange : uint64_t {
@@ -573 +594 @@
     RetainPtr<CGImageRef> m_pendingContentsImage;
     
-    // This represents the animation of a single property. There may be multiple transform animations for
-    // a single transition or keyframe animation, so index is used to distinguish these.
-    struct LayerPropertyAnimation {
-        LayerPropertyAnimation(Ref<PlatformCAAnimation>&& caAnimation, const String& animationName, AnimatedPropertyID property, int index, int subIndex, Seconds timeOffset)
-            : m_animation(WTFMove(caAnimation))
-            , m_name(animationName)
-            , m_property(property)
-            , m_index(index)
-            , m_subIndex(subIndex)
-            , m_timeOffset(timeOffset)
-        { }
-
-        RefPtr<PlatformCAAnimation> m_animation;
-        String m_name;
-        AnimatedPropertyID m_property;
-        int m_index;
-        int m_subIndex;
-        Seconds m_timeOffset;
-    };
-    
     // Uncommitted transitions and animations.
     Vector<LayerPropertyAnimation> m_uncomittedAnimations;