Attachment 359162 Details for Bug 193450 – Patch

[patch] Patch

bug-193450-20190115084349.patch (text/plain), 18.33 KB, created by Simon Fraser (smfr) on 2019-01-15 08:43:50 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-15 08:43:50 PST

Size: 18.33 KB

patch

obsolete

>Subversion Revision: 239985
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c17146950419defd9d23895052113ba2be1f0327..bec886dbf72ca63d51e7a5e81fe6884ed35fbf81 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,42 @@
>+2019-01-15  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Animations should only trigger layer recomposite when necessary
>+        https://bugs.webkit.org/show_bug.cgi?id=193450
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Animations only need to trigger compositing updates when their states change in a way
>+        that affects compositing. RenderLayerCompositor::requiresCompositingForAnimation() checks for
>+        running animations of properties that can be accelerated, so this patch fixes the legacy
>+        animation logic to only set 'shouldRecompositeLayer' in TreeResolver::createAnimatedElementUpdate()
>+        when the running state of such an animation changes.
>+
>+        ImplicitAnimation::animate() and KeyframeAnimation::animate() now return OptionSet<AnimateChange>.
>+        This contains information about whether the running state changed, so CompositeAnimation::animate()
>+        asks about whether the running state of an accelerated property changed, and returns this in
>+        the AnimationUpdate result.
>+
>+        * page/animation/AnimationBase.h:
>+        (WebCore::AnimationBase::isPausedState):
>+        (WebCore::AnimationBase::isRunningState):
>+        (WebCore::AnimationBase::inPausedState const):
>+        (WebCore::AnimationBase::inRunningState const):
>+        (WebCore::AnimationBase::isAnimatingProperty const):
>+        * page/animation/CSSAnimationController.h:
>+        * page/animation/CompositeAnimation.cpp:
>+        (WebCore::CompositeAnimation::animate):
>+        * page/animation/ImplicitAnimation.cpp:
>+        (WebCore::ImplicitAnimation::animate):
>+        (WebCore::ImplicitAnimation::affectsAcceleratedProperty const):
>+        * page/animation/ImplicitAnimation.h:
>+        * page/animation/KeyframeAnimation.cpp:
>+        (WebCore::KeyframeAnimation::KeyframeAnimation):
>+        (WebCore::KeyframeAnimation::animate):
>+        (WebCore::KeyframeAnimation::computeStackingContextImpact): Deleted.
>+        * page/animation/KeyframeAnimation.h:
>+        * style/StyleTreeResolver.cpp:
>+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
>+
> 2019-01-15  Simon Fraser  <simon.fraser@apple.com>
> 
>         Simplify isRunningAnimationOnRenderer()
>diff --git a/Source/WebCore/page/animation/AnimationBase.h b/Source/WebCore/page/animation/AnimationBase.h
>index 6ff7bff9518cd968cc4b27faf2e7b28c0289dfdc..df5eeb14ea4a5a961d4688cd04b096534f1fec79 100644
>--- a/Source/WebCore/page/animation/AnimationBase.h
>+++ b/Source/WebCore/page/animation/AnimationBase.h
>@@ -44,6 +44,12 @@ class RenderElement;
> class RenderStyle;
> class TimingFunction;
> 
>+enum class AnimateChange {
>+    StyleBlended            = 1 << 0, // Style was changed.
>+    StateChange             = 1 << 1, // Animation state() changed.
>+    RunningStateChange      = 1 << 2, // Animation "running or paused" changed.
>+};
>+
> class AnimationBase : public RefCounted<AnimationBase>
>     , public CSSPropertyBlendingClient {
>     friend class CompositeAnimation;
>@@ -122,7 +128,13 @@ public:
>     bool active() const { return !postActive() && !preActive(); }
>     bool running() const { return !isNew() && !postActive(); }
>     bool paused() const { return m_pauseTime || m_animationState == AnimationState::PausedNew; }
>-    bool inPausedState() const { return m_animationState >= AnimationState::PausedNew && m_animationState <= AnimationState::PausedRun; }
>+
>+    static bool isPausedState(AnimationState state) { return state >= AnimationState::PausedNew && state <= AnimationState::PausedRun; }
>+    static bool isRunningState(AnimationState state) { return state >= AnimationState::StartWaitStyleAvailable && state < AnimationState::Done; }
>+
>+    bool inPausedState() const { return isPausedState(m_animationState); }
>+    bool inRunningState() const { return isRunningState(m_animationState); }
>+
>     bool isNew() const { return m_animationState == AnimationState::New || m_animationState == AnimationState::PausedNew; }
>     bool waitingForStartTime() const { return m_animationState == AnimationState::StartWaitResponse; }
>     bool waitingForStyleAvailable() const { return m_animationState == AnimationState::StartWaitStyleAvailable; }
>@@ -163,13 +175,7 @@ public:
>         if (!affectsProperty(property))
>             return false;
> 
>-        if (inPausedState())
>-            return true;
>-
>-        if (m_animationState >= AnimationState::StartWaitStyleAvailable && m_animationState < AnimationState::Done)
>-            return true;
>-
>-        return false;
>+        return inRunningState() || inPausedState();
>     }
> 
>     bool transformFunctionListsMatch() const override { return m_transformFunctionListsMatch; }
>diff --git a/Source/WebCore/page/animation/CSSAnimationController.h b/Source/WebCore/page/animation/CSSAnimationController.h
>index 06b566e210ef5b3df467625edfb1590be5f92eee..cbe88500afae65ce2ea2a70d9d71ef5995e27c03 100644
>--- a/Source/WebCore/page/animation/CSSAnimationController.h
>+++ b/Source/WebCore/page/animation/CSSAnimationController.h
>@@ -44,7 +44,7 @@ class RenderElement;
> 
> struct AnimationUpdate {
>     std::unique_ptr<RenderStyle> style;
>-    bool stateChanged { false };
>+    bool animationChangeRequiresRecomposite { false };
> };
> 
> class CSSAnimationController {
>diff --git a/Source/WebCore/page/animation/CompositeAnimation.cpp b/Source/WebCore/page/animation/CompositeAnimation.cpp
>index b2d1567db708a622c4b98ead98050a79b9234e5f..6f97aa5488201c69cbbae631083cf259277dbbb9 100644
>--- a/Source/WebCore/page/animation/CompositeAnimation.cpp
>+++ b/Source/WebCore/page/animation/CompositeAnimation.cpp
>@@ -286,7 +286,7 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
>     updateKeyframeAnimations(element, currentStyle, targetStyle);
>     m_keyframeAnimations.checkConsistency();
> 
>-    bool animationStateChanged = false;
>+    bool animationChangeRequiresRecomposite = false;
>     bool forceStackingContext = false;
> 
>     std::unique_ptr<RenderStyle> animatedStyle;
>@@ -296,12 +296,11 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
>         // to fill in a RenderStyle*& only if needed.
>         bool checkForStackingContext = false;
>         for (auto& transition : m_transitions.values()) {
>-            bool didBlendStyle = false;
>-            if (transition->animate(*this, targetStyle, animatedStyle, didBlendStyle))
>-                animationStateChanged = true;
>-
>-            if (didBlendStyle)
>+            auto changes = transition->animate(*this, targetStyle, animatedStyle);
>+            if (changes.contains(AnimateChange::StyleBlended))
>                 checkForStackingContext |= WillChangeData::propertyCreatesStackingContext(transition->animatingProperty());
>+
>+            animationChangeRequiresRecomposite = changes.contains(AnimateChange::RunningStateChange) && transition->affectsAcceleratedProperty();
>         }
> 
>         if (animatedStyle && checkForStackingContext) {
>@@ -326,11 +325,9 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
>     for (auto& name : m_keyframeAnimationOrderMap) {
>         RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(name);
>         if (keyframeAnim) {
>-            bool didBlendStyle = false;
>-            if (keyframeAnim->animate(*this, targetStyle, animatedStyle, didBlendStyle))
>-                animationStateChanged = true;
>-
>-            forceStackingContext |= didBlendStyle && keyframeAnim->triggersStackingContext();
>+            auto changes = keyframeAnim->animate(*this, targetStyle, animatedStyle);
>+            animationChangeRequiresRecomposite = changes.contains(AnimateChange::RunningStateChange) && keyframeAnim->affectsAcceleratedProperty();
>+            forceStackingContext |= changes.contains(AnimateChange::StyleBlended) && keyframeAnim->triggersStackingContext();
>             m_hasAnimationThatDependsOnLayout |= keyframeAnim->dependsOnLayout();
>         }
>     }
>@@ -344,7 +341,7 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
>             animatedStyle->setZIndex(0);
>     }
> 
>-    return { WTFMove(animatedStyle), animationStateChanged };
>+    return { WTFMove(animatedStyle), animationChangeRequiresRecomposite };
> }
> 
> std::unique_ptr<RenderStyle> CompositeAnimation::getAnimatedStyle() const
>diff --git a/Source/WebCore/page/animation/ImplicitAnimation.cpp b/Source/WebCore/page/animation/ImplicitAnimation.cpp
>index a4992f8bad0549e9f540356cf11beb9ecc37badf..1d5157cbc3f7142adb44474b5f29ac93670a0b0a 100644
>--- a/Source/WebCore/page/animation/ImplicitAnimation.cpp
>+++ b/Source/WebCore/page/animation/ImplicitAnimation.cpp
>@@ -61,12 +61,12 @@ bool ImplicitAnimation::shouldSendEventForListener(Document::ListenerType inList
>     return element()->document().hasListenerType(inListenerType);
> }
> 
>-bool ImplicitAnimation::animate(CompositeAnimation& compositeAnimation, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle)
>+OptionSet<AnimateChange> ImplicitAnimation::animate(CompositeAnimation& compositeAnimation, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle)
> {
>     // If we get this far and the animation is done, it means we are cleaning up a just finished animation.
>     // So just return. Everything is already all cleaned up.
>     if (postActive())
>-        return false;
>+        return { };
> 
>     AnimationState oldState = state();
> 
>@@ -85,9 +85,15 @@ bool ImplicitAnimation::animate(CompositeAnimation& compositeAnimation, const Re
> 
>     // Fire the start timeout if needed
>     fireAnimationEventsIfNeeded();
>-    
>-    didBlendStyle = true;
>-    return state() != oldState;
>+
>+    OptionSet<AnimateChange> change(AnimateChange::StyleBlended);
>+    if (state() != oldState)
>+        change.add(AnimateChange::StateChange);
>+
>+    if ((isPausedState(oldState) || isRunningState(oldState)) != (isPausedState(state()) || isRunningState(state())))
>+        change.add(AnimateChange::RunningStateChange);
>+
>+    return change;
> }
> 
> void ImplicitAnimation::getAnimatedStyle(std::unique_ptr<RenderStyle>& animatedStyle)
>@@ -131,6 +137,11 @@ bool ImplicitAnimation::computeExtentOfTransformAnimation(LayoutRect& bounds) co
>     return true;
> }
> 
>+bool ImplicitAnimation::affectsAcceleratedProperty() const
>+{
>+    return CSSPropertyAnimation::animationOfPropertyIsAccelerated(m_animatingProperty);
>+}
>+
> bool ImplicitAnimation::startAnimation(double timeOffset)
> {
>     if (auto* renderer = compositedRenderer())
>diff --git a/Source/WebCore/page/animation/ImplicitAnimation.h b/Source/WebCore/page/animation/ImplicitAnimation.h
>index 6cb6c45216a5dca93daafa43175e42622cb75176..2f279a5980b332c3d7611f1d447625138dd31414 100644
>--- a/Source/WebCore/page/animation/ImplicitAnimation.h
>+++ b/Source/WebCore/page/animation/ImplicitAnimation.h
>@@ -53,12 +53,14 @@ public:
>     void pauseAnimation(double timeOffset) override;
>     void endAnimation(bool fillingForwards = false) override;
> 
>-    bool animate(CompositeAnimation&, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle);
>+    OptionSet<AnimateChange> animate(CompositeAnimation&, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle);
>     void getAnimatedStyle(std::unique_ptr<RenderStyle>& animatedStyle) override;
>     void reset(const RenderStyle& to, CompositeAnimation&);
> 
>     bool computeExtentOfTransformAnimation(LayoutRect&) const override;
> 
>+    bool affectsAcceleratedProperty() const;
>+
>     void setOverridden(bool);
>     bool overridden() const override { return m_overridden; }
> 
>diff --git a/Source/WebCore/page/animation/KeyframeAnimation.cpp b/Source/WebCore/page/animation/KeyframeAnimation.cpp
>index 0ce80426db93fd61d46272bea86951b8afe70ddb..defa3fdc228d943e38f63f056ddb649ef01ac2c2 100644
>--- a/Source/WebCore/page/animation/KeyframeAnimation.cpp
>+++ b/Source/WebCore/page/animation/KeyframeAnimation.cpp
>@@ -60,7 +60,17 @@ KeyframeAnimation::KeyframeAnimation(const Animation& animation, Element& elemen
> #endif
>     checkForMatchingColorFilterFunctionLists();
> 
>-    computeStackingContextImpact();
>+    for (auto propertyID : m_keyframes.properties()) {
>+        if (WillChangeData::propertyCreatesStackingContext(propertyID))
>+            m_triggersStackingContext = true;
>+        
>+        if (CSSPropertyAnimation::animationOfPropertyIsAccelerated(propertyID))
>+            m_hasAcceleratedProperty = true;
>+        
>+        if (m_triggersStackingContext && m_hasAcceleratedProperty)
>+            break;
>+    }
>+
>     computeLayoutDependency();
> }
> 
>@@ -71,16 +81,6 @@ KeyframeAnimation::~KeyframeAnimation()
>         endAnimation();
> }
> 
>-void KeyframeAnimation::computeStackingContextImpact()
>-{
>-    for (auto propertyID : m_keyframes.properties()) {
>-        if (WillChangeData::propertyCreatesStackingContext(propertyID)) {
>-            m_triggersStackingContext = true;
>-            break;
>-        }
>-    }
>-}
>-
> void KeyframeAnimation::computeLayoutDependency()
> {
>     if (!m_keyframes.containsProperty(CSSPropertyTransform))
>@@ -156,7 +156,7 @@ void KeyframeAnimation::fetchIntervalEndpointsForProperty(CSSPropertyID property
>     prog = progress(scale, offset, prevKeyframe.timingFunction());
> }
> 
>-bool KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle)
>+OptionSet<AnimateChange> KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle)
> {
>     AnimationState oldState = state();
> 
>@@ -171,12 +171,12 @@ bool KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const Re
>             updateStateMachine(AnimationStateInput::PlayStatePaused, -1);
>     }
> 
>-    // If we get this far and the animation is done, it means we are cleaning up a just finished animation.
>+    // If we get this far and the animation is done, it means we are cleaning up a just-finished animation.
>     // If so, we need to send back the targetStyle.
>     if (postActive()) {
>         if (!animatedStyle)
>             animatedStyle = RenderStyle::clonePtr(targetStyle);
>-        return false;
>+        return { };
>     }
> 
>     // If we are waiting for the start timer, we don't want to change the style yet.
>@@ -185,12 +185,12 @@ bool KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const Re
>     // Special case 2 - if there is a backwards fill mode, then we want to continue
>     // through to the style blend so that we get the fromStyle.
>     if (waitingToStart() && m_animation->delay() > 0 && !m_animation->fillsBackwards())
>-        return false;
>+        return { };
>     
>     // If we have no keyframes, don't animate.
>     if (!m_keyframes.size()) {
>         updateStateMachine(AnimationStateInput::EndAnimation, -1);
>-        return false;
>+        return { };
>     }
> 
>     // Run a cycle of animation.
>@@ -209,9 +209,15 @@ bool KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const Re
> 
>         CSSPropertyAnimation::blendProperties(this, propertyID, animatedStyle.get(), fromStyle, toStyle, progress);
>     }
>-    
>-    didBlendStyle = true;
>-    return state() != oldState;
>+
>+    OptionSet<AnimateChange> change(AnimateChange::StyleBlended);
>+    if (state() != oldState)
>+        change.add(AnimateChange::StateChange);
>+
>+    if ((isPausedState(oldState) || isRunningState(oldState)) != (isPausedState(state()) || isRunningState(state())))
>+        change.add(AnimateChange::RunningStateChange);
>+
>+    return change;
> }
> 
> void KeyframeAnimation::getAnimatedStyle(std::unique_ptr<RenderStyle>& animatedStyle)
>diff --git a/Source/WebCore/page/animation/KeyframeAnimation.h b/Source/WebCore/page/animation/KeyframeAnimation.h
>index 2c807318625ef17263166999e9689d3a9a26917e..3889fee25fc9a9334592a7c60eafdaa75bb2e2cb 100644
>--- a/Source/WebCore/page/animation/KeyframeAnimation.h
>+++ b/Source/WebCore/page/animation/KeyframeAnimation.h
>@@ -44,8 +44,8 @@ public:
>     {
>         return adoptRef(*new KeyframeAnimation(animation, element, compositeAnimation, unanimatedStyle));
>     }
>-
>-    bool animate(CompositeAnimation&, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle);
>+    
>+    OptionSet<AnimateChange> animate(CompositeAnimation&, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle);
>     void getAnimatedStyle(std::unique_ptr<RenderStyle>&) override;
> 
>     bool computeExtentOfTransformAnimation(LayoutRect&) const override;
>@@ -58,6 +58,7 @@ public:
> 
>     bool triggersStackingContext() const { return m_triggersStackingContext; }
>     bool dependsOnLayout() const { return m_dependsOnLayout; }
>+    bool affectsAcceleratedProperty() const { return m_hasAcceleratedProperty; }
> 
>     void setUnanimatedStyle(std::unique_ptr<RenderStyle> style) { m_unanimatedStyle = WTFMove(style); }
>     const RenderStyle& unanimatedStyle() const override { return *m_unanimatedStyle; }
>@@ -84,7 +85,6 @@ protected:
> 
>     bool computeExtentOfAnimationForMatchingTransformLists(const FloatRect& rendererBox, LayoutRect&) const;
> 
>-    void computeStackingContextImpact();
>     void computeLayoutDependency();
>     void resolveKeyframeStyles();
>     void validateTransformFunctionList();
>@@ -107,6 +107,7 @@ private:
> 
>     bool m_startEventDispatched { false };
>     bool m_triggersStackingContext { false };
>+    bool m_hasAcceleratedProperty { false };
>     bool m_dependsOnLayout { false };
> };
> 
>diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
>index 7a9759369fb6601b8783829e37f24aa68daebd39..34f8f3c3c86e3fdf95f18606b6aca6132e5d8ee3 100644
>--- a/Source/WebCore/style/StyleTreeResolver.cpp
>+++ b/Source/WebCore/style/StyleTreeResolver.cpp
>@@ -306,7 +306,7 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
>         auto& animationController = m_document.frame()->animation();
> 
>         auto animationUpdate = animationController.updateAnimations(element, *newStyle, oldStyle);
>-        shouldRecompositeLayer = animationUpdate.stateChanged; // FIXME: constrain this to just property animations triggering acceleration.
>+        shouldRecompositeLayer = animationUpdate.animationChangeRequiresRecomposite;
> 
>         if (animationUpdate.style)
>             newStyle = WTFMove(animationUpdate.style);

Flags:

graouts: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193450: 359162