Changeset 268746 in webkit

Timestamp:

Oct 20, 2020, 10:34:38 AM (6 years ago)

Author:

graouts@webkit.org

Message:

REGRESSION(r268615): some accelerated transform tests are failing
​https://bugs.webkit.org/show_bug.cgi?id=217851
<rdar://problem/70394402>

Reviewed by Dean Jackson.

Source/WebCore:

When we added support for accelerated individual transform properties animations, we added
the notion of base transform animations which are used to set the base value of any
transform-related property that is not animated.

Those animations were defined to start as early as possible, assuming that a very small value
after 0s was as early as possible. However, it's possible that other animations start with a
negative time if they have a delay or are seeked, if the value returned by CACurrentMediaTime()
is smaller than that delay. This means that if the machine had been booted for less time than
an accelerated animation's delay, the base transform animation wouldn't overlap.

We now ensure that those base transform animations start as early as the earliest animation
that is being committed in a call to GraphicsLayerCA::updateAnimations().

• platform/graphics/ca/GraphicsLayerCA.cpp:

(WebCore::GraphicsLayerCA::updateAnimations):
(WebCore::GraphicsLayerCA::setAnimationOnLayer):

• platform/graphics/ca/GraphicsLayerCA.h:

(WebCore::GraphicsLayerCA::LayerPropertyAnimation::computedBeginTime const):

LayoutTests:

Remove flaky epectations for affected tests and skip tests on Windows where failures remain.

• TestExpectations:
• platform/mac/TestExpectations:
• platform/win/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/platform/mac/TestExpectations (modified) (1 diff)
• LayoutTests/platform/win/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (modified) (4 diffs)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-10-20  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION(r268615): some accelerated transform tests are failing
+        https://bugs.webkit.org/show_bug.cgi?id=217851
+        <rdar://problem/70394402>
+
+        Reviewed by Dean Jackson.
+
+        Remove flaky epectations for affected tests and skip tests on Windows where failures remain.
+
+        * TestExpectations:
+        * platform/mac/TestExpectations:
+        * platform/win/TestExpectations:
+
 2020-10-20  Rob Buis  <rbuis@igalia.com>
 

trunk/LayoutTests/TestExpectations

@@ -4519 +4519 @@
 fast/layoutformattingcontext/ [ ImageOnlyFailure ]
 webkit.org/b/217054 fast/layoutformattingcontext/horizontal-sizing-with-trailing-letter-spacing.html [ Skip ]
-
-webkit.org/b/217851 transitions/interrupted-transition-hardware.html [ Pass Failure ]
-webkit.org/b/217851 webanimations/accelerated-transform-related-animation-property-order.html [ Pass Failure ]
-webkit.org/b/217851 webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html [ Pass Failure ]
-webkit.org/b/217851 webanimations/accelerated-translate-animation-underlying-transform-changed-in-flight.html [ Pass Failure ]
-webkit.org/b/217851 webanimations/accelerated-translate-animation-with-transform.html [ Pass Failure ]
-webkit.org/b/217851 webanimations/accelerated-translate-animation.html [ Pass Failure ]

trunk/LayoutTests/platform/mac/TestExpectations

@@ -2247 +2247 @@
 webkit.org/b/217620 inspector/audit/basic-async.htm [ Pass Timeout ]
 
-# These are failing on Mojave due to an error in the way CA animations are applied.
-# https://bugs.webkit.org/show_bug.cgi?id=217842
-[ Mojave ] transitions/interrupted-transition-hardware.html [ Pass Failure ]
-[ Mojave ] webanimations/accelerated-transform-related-animation-property-order.html [ Pass Failure ]
-[ Mojave ] webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html [ Pass Failure ]
-[ Mojave ] webanimations/accelerated-translate-animation-underlying-transform-changed-in-flight.html [ Pass Failure ]
-[ Mojave ] webanimations/accelerated-translate-animation-with-transform.html [ Pass Failure ]
-[ Mojave ] webanimations/accelerated-translate-animation.html [ Pass Failure ]
-
 webkit.org/b/217931 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/before-prepare-createHTMLDocument-success-empty-src-module.html [ Pass Failure ]
 

trunk/LayoutTests/platform/win/TestExpectations

@@ -4587 +4587 @@
 webkit.org/b/217812 transforms/2d/translate-change-composited.html [ Skip ]
 webkit.org/b/217812 transforms/2d/rotate-composited.html [ Skip ]
+webkit.org/b/217812 webanimations/accelerated-transform-related-animation-property-order.html [ Skip ]
+webkit.org/b/217812 webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html [ Skip ]
+webkit.org/b/217812 webanimations/accelerated-translate-animation-underlying-transform-changed-in-flight.html [ Skip ]
+webkit.org/b/217812 webanimations/accelerated-translate-animation-with-transform.html [ Skip ]
+webkit.org/b/217812 webanimations/accelerated-translate-animation.html [ Skip ]
 
 webkit.org/b/217922 [ Release ] animations/additive-transform-animations.html [ ImageOnlyFailure ]
 webkit.org/b/217922 animations/needs-layout.html [ ImageOnlyFailure ]
 webkit.org/b/217922 fast/animation/animation-mixed-transform-crash.html [ ImageOnlyFailure ]
-

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-10-20  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION(r268615): some accelerated transform tests are failing
+        https://bugs.webkit.org/show_bug.cgi?id=217851
+        <rdar://problem/70394402>
+
+        Reviewed by Dean Jackson.
+
+        When we added support for accelerated individual transform properties animations, we added
+        the notion of base transform animations which are used to set the base value of any
+        transform-related property that is not animated.
+
+        Those animations were defined to start as early as possible, assuming that a very small value
+        after 0s was as early as possible. However, it's possible that other animations start with a
+        negative time if they have a delay or are seeked, if the value returned by CACurrentMediaTime()
+        is smaller than that delay. This means that if the machine had been booted for less time than
+        an accelerated animation's delay, the base transform animation wouldn't overlap.
+
+        We now ensure that those base transform animations start as early as the earliest animation
+        that is being committed in a call to GraphicsLayerCA::updateAnimations().
+
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::GraphicsLayerCA::updateAnimations):
+        (WebCore::GraphicsLayerCA::setAnimationOnLayer):
+        * platform/graphics/ca/GraphicsLayerCA.h:
+        (WebCore::GraphicsLayerCA::LayerPropertyAnimation::computedBeginTime const):
+
 2020-10-20  Rob Buis  <rbuis@igalia.com>
 

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

@@ -2862 +2862 @@
 void GraphicsLayerCA::updateAnimations()
 {
+    auto baseTransformAnimationBeginTime = 1_s;
+    auto currentTime = Seconds(CACurrentMediaTime());
+    auto updateBeginTimes = [&](LayerPropertyAnimation& animation)
+    {
+        if (animation.m_pendingRemoval)
+            return;
+
+        // In case we have an offset, and we haven't set an explicit begin time previously,
+        // we need to record the beginTime now.
+        if (animation.m_timeOffset && !animation.m_beginTime)
+            animation.m_beginTime = currentTime;
+
+        // Now check if we have a resolved begin time and ensure the begin time we'll use for
+        // base transform animations matches the smallest known begin time to guarantee that
+        // such animations can combine with other explicit transform animations correctly.
+        if (auto computedBeginTime = animation.computedBeginTime())
+            baseTransformAnimationBeginTime = std::min(baseTransformAnimationBeginTime, *computedBeginTime);
+    };
+
     enum class Additive { Yes, No };
     auto addAnimation = [&](LayerPropertyAnimation& animation, Additive additive = Additive::Yes) {
@@ -2891 +2910 @@
         // To ensure the base value transform is applied along with all the interpolating animations, we set it to have started
         // as early as possible, which combined with the infinite duration ensures it's current for any given CA media time.
-        animation.m_beginTime = Seconds::fromNanoseconds(1);
+        animation.m_beginTime = baseTransformAnimationBeginTime;
 
         // Additivity will depend on the source of the matrix, if it was explicitly provided as an identity matrix, it
@@ -2901 +2920 @@
     };
 
-    // Remove all running CA animations.
+    // Iterate through all animations to update each animation's begin time, if necessary,
+    // compute the base transform animation begin times and remove all running CA animations.
     for (auto& animation : m_animations) {
+        updateBeginTimes(animation);
         if (animation.m_playState == PlayState::Playing || animation.m_playState == PlayState::Paused)
             removeCAAnimationFromLayer(animation);
@@ -3022 +3043 @@
     auto& caAnim = *animation.m_animation;
 
-    if (animation.m_timeOffset) {
-        // In case we have an offset, we need to record the beginTime now since we have to pass in an explicit
-        // value in the first place.
-        if (!animation.m_beginTime)
-            animation.m_beginTime = Seconds(CACurrentMediaTime());
-        caAnim.setBeginTime((animation.m_beginTime - animation.m_timeOffset).seconds());
-    } else if (animation.m_beginTime) {
-        // If we already have a begin time, then we already started in the past and must ensure we use that same
-        // begin time. Any other case will get use the CA transaction's time as its begin time and will be recorded
-        // in platformCALayerAnimationStarted().
-        caAnim.setBeginTime(animation.m_beginTime.seconds());
-    }
+    if (auto beginTime = animation.computedBeginTime())
+        caAnim.setBeginTime(beginTime->seconds());
 
     String animationID = animation.animationIdentifier();

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

@@ -31 +31 @@
 #include "PlatformCALayerClient.h"
 #include <wtf/HashMap.h>
+#include <wtf/Optional.h>
 #include <wtf/RetainPtr.h>
 #include <wtf/text/StringHash.h>
@@ -463 +464 @@
 
         String animationIdentifier() const { return makeString(m_name, '_', static_cast<unsigned>(m_property), '_', m_index, '_', m_subIndex); }
+        Optional<Seconds> computedBeginTime() const
+        {
+            if (m_beginTime)
+                return m_beginTime - m_timeOffset;
+            return WTF::nullopt;
+        }
 
         RefPtr<PlatformCAAnimation> m_animation;