From: graouts@webkit.org Date: Wed, 16 May 2018 08:27:54 +0000 (+0000) Subject: REGRESSION (r230574): Interrupted hardware transitions don't behave correctly X-Git-Url: http://git.webkit.org/?p=WebKit-https.git;a=commitdiff_plain;h=2d5969af679ffc79ec4255e17d30503980c3f93e REGRESSION (r230574): Interrupted hardware transitions don't behave correctly https://bugs.webkit.org/show_bug.cgi?id=185299 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. git-svn-id: https://svn.webkit.org/repository/webkit/trunk@231840 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 161e28e..a22f5e7 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,17 @@ +2018-05-16 Antoine Quint + + REGRESSION (r230574): Interrupted hardware transitions don't behave correctly + https://bugs.webkit.org/show_bug.cgi?id=185299 + + + 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 Unreviewed, rolling out r231765. diff --git a/LayoutTests/transitions/interrupted-transition-hardware-expected.html b/LayoutTests/transitions/interrupted-transition-hardware-expected.html new file mode 100644 index 0000000..ff73df0 --- /dev/null +++ b/LayoutTests/transitions/interrupted-transition-hardware-expected.html @@ -0,0 +1,22 @@ + + + + + + + +

+ + diff --git a/LayoutTests/transitions/interrupted-transition-hardware.html b/LayoutTests/transitions/interrupted-transition-hardware.html new file mode 100644 index 0000000..b6e77a5 --- /dev/null +++ b/LayoutTests/transitions/interrupted-transition-hardware.html @@ -0,0 +1,61 @@ + + + + + + + + +

+ + + + + diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 9cc5dfd..2af26a5 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,27 @@ +2018-05-16 Antoine Quint + + REGRESSION (r230574): Interrupted hardware transitions don't behave correctly + https://bugs.webkit.org/show_bug.cgi?id=185299 + + + 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 [JSC] Check TypeInfo first before calling getCallData when we would like to check whether given object is a function diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp index 2b35a97..0a02a35 100644 --- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp +++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp @@ -3015,7 +3015,7 @@ bool GraphicsLayerCA::createAnimationFromKeyframes(const KeyframeValueList& valu if (!valuesOK) 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; } @@ -3042,7 +3042,7 @@ bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& val if (!validMatrices) 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; } @@ -3104,12 +3104,21 @@ bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& val 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)); +} + bool GraphicsLayerCA::createFilterAnimationsFromKeyframes(const KeyframeValueList& valueList, const Animation* animation, const String& animationName, Seconds timeOffset) { #if ENABLE(FILTERS_LEVEL_2) diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h index 5011ccd..ee41cd4 100644 --- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h +++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h @@ -459,8 +459,29 @@ private: moveOrCopyAnimations(Copy, fromLayer, toLayer); } + // 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&& 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 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 { NoChange = 0, @@ -572,26 +593,6 @@ private: RetainPtr m_uncorrectedContentsImage; RetainPtr 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&& 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 m_animation; - String m_name; - AnimatedPropertyID m_property; - int m_index; - int m_subIndex; - Seconds m_timeOffset; - }; - // Uncommitted transitions and animations. Vector m_uncomittedAnimations;