2010-09-24 Simon Fraser <simon.fraser@apple.com>
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Sep 2010 02:52:10 +0000 (02:52 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Sep 2010 02:52:10 +0000 (02:52 +0000)
        Reviewed by Sam Weinig.

        Accelerated transitions do not suspend/resume properly.
        https://bugs.webkit.org/show_bug.cgi?id=43792

        (1) Fix regression from r68233, where if an animation affected two
        properties, only one would animate. The testcase tests this.

        (2) Fix a flash at the end of an animation or transition that has been
        paused and resumed. The flash occurred because we used CAAnimation's
        timeOffset when resuming to push the start of the animation into
        the past. However, timeOffset does not play nicely with fill modes,
        causing a single frame of animation with the element in its unanimated state.

        Fixed this by offsetting the beginTime into the past, rather than setting
        timeOffset. Normally we submit animations with beginTime == 0, and rely
        on CA assigning a beginTime when the animation is committed. This beginTime
        is then passed to AnimationController to sync hardware and software animations.
        However, since the code now assigns beginTimes in the past (on resume),
        we now have to denote whether we've done this, and send an appropriate
        timestamp back to AnimationController.

        (3) Finally, the patch removes PropertyAnimationPair and just uses LayerPropertyAnimation
        instead. This is just cleanup.

        Test: animations/opacity-transform-animation.html

        * platform/graphics/mac/GraphicsLayerCA.h: Remove PropertyAnimationPair,
        and change AnimationsMap to store a vector of LayerPropertyAnimation.
        New method, animationDidStart(), is called from the -animationDidStart: callback.

        * platform/graphics/mac/GraphicsLayerCA.mm:
        (-[WebAnimationDelegate animationDidStart:]): Just call m_graphicsLayer->animationDidStart()
        now.
        (WebCore::animationIdentifier): We need to pass in the property, to fix (1)
        (WebCore::GraphicsLayerCA::moveOrCopyAnimationsForProperty): Copy the WebKitAnimationBeginTimeSet
        value, if present.
        (WebCore::GraphicsLayerCA::animationDidStart): Call notifyAnimationStarted() on the client,
        after testing if we set a non-zero beginTime on this particular animation.
        (WebCore::GraphicsLayerCA::updateLayerAnimations): Change to use LayerPropertyAnimation.
        (WebCore::GraphicsLayerCA::setCAAnimationOnLayer): Set value for WebKitAnimationBeginTimeSetKey
        if we have a non-zero timeOffset. Make animationID an NSString, to avoid several conversions.
        (WebCore::GraphicsLayerCA::removeCAAnimationFromLayer):  Make animationID an NSString, to
        avoid several conversions.
        (WebCore::copyAnimationProperties): Copy WebKitAnimationBeginTimeSetKey if present.
        (WebCore::GraphicsLayerCA::pauseCAAnimationOnLayer): Make animationID an NSString, to avoid
        several conversions.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@68323 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/animations/opacity-transform-animation-expected.checksum [new file with mode: 0644]
LayoutTests/animations/opacity-transform-animation-expected.png [new file with mode: 0644]
LayoutTests/animations/opacity-transform-animation-expected.txt [new file with mode: 0644]
LayoutTests/animations/opacity-transform-animation.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/platform/graphics/mac/GraphicsLayerCA.h
WebCore/platform/graphics/mac/GraphicsLayerCA.mm

index ef8b14b..ce54fb2 100644 (file)
@@ -1,3 +1,17 @@
+2010-09-24  Simon Fraser  <simon.fraser@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        Accelerated transitions do not suspend/resume properly.
+        https://bugs.webkit.org/show_bug.cgi?id=43792
+        
+        Testcase with an animation that has two accelerated properties changing.
+
+        * animations/opacity-transform-animation-expected.checksum: Added.
+        * animations/opacity-transform-animation-expected.png: Added.
+        * animations/opacity-transform-animation-expected.txt: Added.
+        * animations/opacity-transform-animation.html: Added.
+
 2010-09-24  Mihai Parparita  <mihaip@chromium.org>
 
         Unreviewed Chromium test_expectations.txt and drt_expectations.txt update.
diff --git a/LayoutTests/animations/opacity-transform-animation-expected.checksum b/LayoutTests/animations/opacity-transform-animation-expected.checksum
new file mode 100644 (file)
index 0000000..ada2409
--- /dev/null
@@ -0,0 +1 @@
+f22c42e2db129ca7a1658831681fc7c8
\ No newline at end of file
diff --git a/LayoutTests/animations/opacity-transform-animation-expected.png b/LayoutTests/animations/opacity-transform-animation-expected.png
new file mode 100644 (file)
index 0000000..78c31b5
Binary files /dev/null and b/LayoutTests/animations/opacity-transform-animation-expected.png differ
diff --git a/LayoutTests/animations/opacity-transform-animation-expected.txt b/LayoutTests/animations/opacity-transform-animation-expected.txt
new file mode 100644 (file)
index 0000000..8cdf1c6
--- /dev/null
@@ -0,0 +1,9 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x8
+  RenderBlock {HTML} at (0,0) size 800x8
+    RenderBody {BODY} at (8,8) size 784x0
+layer at (200,0) size 100x100
+  RenderBlock (positioned) {DIV} at (200,0) size 100x100 [bgcolor=#FF0000]
+layer at (0,0) size 100x100
+  RenderBlock (positioned) {DIV} at (0,0) size 100x100 [bgcolor=#008000]
diff --git a/LayoutTests/animations/opacity-transform-animation.html b/LayoutTests/animations/opacity-transform-animation.html
new file mode 100644 (file)
index 0000000..432b088
--- /dev/null
@@ -0,0 +1,65 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <style type="text/css" media="screen">
+    #box {
+        position: absolute;
+        left: 0;
+        top: 0;
+        height: 100px;
+        width: 100px;
+        background-color: green;
+        -webkit-animation-name: move;
+        -webkit-animation-duration: 1s;
+        -webkit-animation-timing-function: linear;
+        -webkit-animation-iteration-count: 1;
+    }
+
+    @-webkit-keyframes move {
+        from {
+            -webkit-transform: translateX(0) scale(1);
+            opacity: 0.8;
+        }
+        to {
+            -webkit-transform: translateX(400px) scale(1);
+            opacity: 0.9;
+        }
+    }
+    
+    #indicator {
+        position: absolute;
+        left: 0;
+        top: 0;
+        height: 100px;
+        width: 100px;
+        left: 200px;
+        background-color: red;
+    }
+    
+    #result {
+        display: none; /* only the pixel output is valuable */
+    }
+  </style>
+  <script src="animation-test-helpers.js" type="text/javascript" charset="utf-8"></script>
+  <script type="text/javascript" charset="utf-8">
+
+    const expectedValues = [
+      // [animation-name, time, element-id, property, expected-value, tolerance]
+      ["move", 0.5, "box", "webkitTransform.4", 200, 2],
+    ];
+
+    var disablePauseAnimationAPI = false;
+    var doPixelTest = true;
+    runAnimationTest(expectedValues, null, null, disablePauseAnimationAPI, doPixelTest);
+    
+  </script>
+</head>
+<body>
+
+<!-- In the pixel results, the green square should overlay the red square -->
+<div id="indicator"></div>
+<div id="box"></div>
+<div id="result"></div>
+
+</body>
+</html>
index d216e51..cca5602 100644 (file)
@@ -1,3 +1,53 @@
+2010-09-24  Simon Fraser  <simon.fraser@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        Accelerated transitions do not suspend/resume properly.
+        https://bugs.webkit.org/show_bug.cgi?id=43792
+        
+        (1) Fix regression from r68233, where if an animation affected two
+        properties, only one would animate. The testcase tests this.
+        
+        (2) Fix a flash at the end of an animation or transition that has been
+        paused and resumed. The flash occurred because we used CAAnimation's
+        timeOffset when resuming to push the start of the animation into
+        the past. However, timeOffset does not play nicely with fill modes,
+        causing a single frame of animation with the element in its unanimated state.
+        
+        Fixed this by offsetting the beginTime into the past, rather than setting
+        timeOffset. Normally we submit animations with beginTime == 0, and rely
+        on CA assigning a beginTime when the animation is committed. This beginTime
+        is then passed to AnimationController to sync hardware and software animations.
+        However, since the code now assigns beginTimes in the past (on resume),
+        we now have to denote whether we've done this, and send an appropriate
+        timestamp back to AnimationController.
+        
+        (3) Finally, the patch removes PropertyAnimationPair and just uses LayerPropertyAnimation
+        instead. This is just cleanup.
+
+        Test: animations/opacity-transform-animation.html
+
+        * platform/graphics/mac/GraphicsLayerCA.h: Remove PropertyAnimationPair,
+        and change AnimationsMap to store a vector of LayerPropertyAnimation.
+        New method, animationDidStart(), is called from the -animationDidStart: callback.
+        
+        * platform/graphics/mac/GraphicsLayerCA.mm:
+        (-[WebAnimationDelegate animationDidStart:]): Just call m_graphicsLayer->animationDidStart()
+        now.
+        (WebCore::animationIdentifier): We need to pass in the property, to fix (1)
+        (WebCore::GraphicsLayerCA::moveOrCopyAnimationsForProperty): Copy the WebKitAnimationBeginTimeSet
+        value, if present.
+        (WebCore::GraphicsLayerCA::animationDidStart): Call notifyAnimationStarted() on the client,
+        after testing if we set a non-zero beginTime on this particular animation.
+        (WebCore::GraphicsLayerCA::updateLayerAnimations): Change to use LayerPropertyAnimation.
+        (WebCore::GraphicsLayerCA::setCAAnimationOnLayer): Set value for WebKitAnimationBeginTimeSetKey
+        if we have a non-zero timeOffset. Make animationID an NSString, to avoid several conversions.
+        (WebCore::GraphicsLayerCA::removeCAAnimationFromLayer):  Make animationID an NSString, to
+        avoid several conversions.
+        (WebCore::copyAnimationProperties): Copy WebKitAnimationBeginTimeSetKey if present.
+        (WebCore::GraphicsLayerCA::pauseCAAnimationOnLayer): Make animationID an NSString, to avoid
+        several conversions.
+
 2010-09-24  Andreas Kling  <andreas.kling@nokia.com>
 
         Reviewed by Kenneth Rohde Christiansen.
index 91d5de0..17a67ac 100644 (file)
@@ -118,6 +118,9 @@ public:
     virtual void syncCompositingState();
     virtual void syncCompositingStateForThisLayerOnly();
 
+    // Should only be called by animationDidStart: callback
+    void animationDidStart(CAAnimation*);
+    
 protected:
     virtual void setOpacityInternal(float);
 
@@ -354,9 +357,6 @@ private:
     
     // Uncommitted transitions and animations.
     Vector<LayerPropertyAnimation> m_uncomittedAnimations;
-
-    typedef int AnimatedProperty; // std containers choke on the AnimatedPropertyID enum
-    typedef pair<AnimatedProperty, int> PropertyAnimationPair; // pair of property, index
     
     enum Action { Remove, Pause };
     struct AnimationProcessingAction {
@@ -372,7 +372,7 @@ private:
     AnimationsToProcessMap m_animationsToProcess;
 
     // Map of animation names to their associated lists of property animations, so we can remove/pause them.
-    typedef HashMap<String, Vector<PropertyAnimationPair> > AnimationsMap;
+    typedef HashMap<String, Vector<LayerPropertyAnimation> > AnimationsMap;
     AnimationsMap m_runningAnimations;
 
     Vector<FloatRect> m_dirtyRects;
index 33f14e6..d4cd851 100644 (file)
@@ -55,6 +55,8 @@ using namespace std;
 
 namespace WebCore {
 
+static NSString * const WebKitAnimationBeginTimeSetKey = @"WebKitAnimationBeginTimeSet";
+
 // The threshold width or height above which a tiled layer will be used. This should be
 // large enough to avoid tiled layers for most GraphicsLayers, but less than the OpenGL
 // texture size limit on all supported hardware.
@@ -101,11 +103,8 @@ static double mediaTimeToCurrentTime(CFTimeInterval t)
 
 - (void)animationDidStart:(CAAnimation *)animation
 {
-    if (!m_graphicsLayer)
-        return;
-
-    double startTime = WebCore::mediaTimeToCurrentTime([animation beginTime]);
-    m_graphicsLayer->client()->notifyAnimationStarted(m_graphicsLayer, startTime);
+    if (m_graphicsLayer)
+        m_graphicsLayer->animationDidStart(animation);
 }
 
 - (WebCore::GraphicsLayerCA*)graphicsLayer
@@ -247,14 +246,9 @@ static String propertyIdToString(AnimatedPropertyID property)
     return "";
 }
 
-static String animationIdentifier(const String& animationName, int index)
+static String animationIdentifier(const String& animationName, AnimatedPropertyID property, int index)
 {
-    StringBuilder builder;
-
-    builder.append(animationName);
-    builder.append("_");
-    builder.append(String::number(index));
-    return builder.toString();
+    return animationName + String::format("_%d_%d", property, index);
 }
 
 static CAMediaTimingFunction* getCAMediaTimingFunction(const TimingFunction* timingFunction)
@@ -570,12 +564,12 @@ void GraphicsLayerCA::moveOrCopyAnimationsForProperty(MoveOrCopy operation, Anim
     // Look for running animations affecting this property.
     AnimationsMap::const_iterator end = m_runningAnimations.end();
     for (AnimationsMap::const_iterator it = m_runningAnimations.begin(); it != end; ++it) {
-        const Vector<PropertyAnimationPair>& propertyAnimations = it->second;
+        const Vector<LayerPropertyAnimation>& propertyAnimations = it->second;
         size_t numAnimations = propertyAnimations.size();
         for (size_t i = 0; i < numAnimations; ++i) {
-            const PropertyAnimationPair& currPair = propertyAnimations[i];
-            if (currPair.first == property)
-                moveOrCopyLayerAnimation(operation, animationIdentifier(it->first, currPair.second), fromLayer, toLayer);
+            const LayerPropertyAnimation& currAnimation = propertyAnimations[i];
+            if (currAnimation.m_property == property)
+                moveOrCopyLayerAnimation(operation, animationIdentifier(currAnimation.m_name, currAnimation.m_property, currAnimation.m_index), fromLayer, toLayer);
         }
     }
 }
@@ -756,6 +750,22 @@ void GraphicsLayerCA::removeAnimation(const String& animationName)
     noteLayerPropertyChanged(AnimationChanged);
 }
 
+void GraphicsLayerCA::animationDidStart(CAAnimation* caAnimation)
+{
+    bool hadNonZeroBeginTime = [[caAnimation valueForKey:WebKitAnimationBeginTimeSetKey] boolValue];
+
+    double startTime;
+    if (hadNonZeroBeginTime) {
+        // We don't know what time CA used to commit the animation, so just use the current time
+        // (even though this will be slightly off).
+        startTime = WebCore::mediaTimeToCurrentTime(CACurrentMediaTime());
+    } else
+        startTime = WebCore::mediaTimeToCurrentTime([caAnimation beginTime]);
+
+    if (m_client)
+        m_client->notifyAnimationStarted(this, startTime);
+}
+
 void GraphicsLayerCA::setContentsToImage(Image* image)
 {
     if (image) {
@@ -1493,15 +1503,15 @@ void GraphicsLayerCA::updateLayerAnimations()
                 continue;
 
             const AnimationProcessingAction& processingInfo = it->second;
-            const Vector<PropertyAnimationPair>& animations = animationIt->second;
+            const Vector<LayerPropertyAnimation>& animations = animationIt->second;
             for (size_t i = 0; i < animations.size(); ++i) {
-                const PropertyAnimationPair& currPair = animations[i];
+                const LayerPropertyAnimation& currAnimation = animations[i];
                 switch (processingInfo.action) {
                     case Remove:
-                        removeCAAnimationFromLayer(static_cast<AnimatedPropertyID>(currPair.first), currAnimationName, currPair.second);
+                        removeCAAnimationFromLayer(currAnimation.m_property, currAnimationName, currAnimation.m_index);
                         break;
                     case Pause:
-                        pauseCAAnimationOnLayer(static_cast<AnimatedPropertyID>(currPair.first), currAnimationName, currPair.second, processingInfo.timeOffset);
+                        pauseCAAnimationOnLayer(currAnimation.m_property, currAnimationName, currAnimation.m_index, processingInfo.timeOffset);
                         break;
                 }
             }
@@ -1521,12 +1531,12 @@ void GraphicsLayerCA::updateLayerAnimations()
             
             AnimationsMap::iterator it = m_runningAnimations.find(pendingAnimation.m_name);
             if (it == m_runningAnimations.end()) {
-                Vector<PropertyAnimationPair> firstPair;
-                firstPair.append(PropertyAnimationPair(pendingAnimation.m_property, pendingAnimation.m_index));
-                m_runningAnimations.add(pendingAnimation.m_name, firstPair);
+                Vector<LayerPropertyAnimation> animations;
+                animations.append(pendingAnimation);
+                m_runningAnimations.add(pendingAnimation.m_name, animations);
             } else {
-                Vector<PropertyAnimationPair>& animPairs = it->second;
-                animPairs.append(PropertyAnimationPair(pendingAnimation.m_property, pendingAnimation.m_index));
+                Vector<LayerPropertyAnimation>& animations = it->second;
+                animations.append(pendingAnimation);
             }
         }
         
@@ -1538,10 +1548,13 @@ void GraphicsLayerCA::setCAAnimationOnLayer(CAPropertyAnimation* caAnim, Animate
 {
     PlatformLayer* layer = animatedLayer(property);
 
-    [caAnim setTimeOffset:timeOffset];
-    
-    String animationID = animationIdentifier(animationName, index);
-    
+    if (timeOffset) {
+        [caAnim setBeginTime:CACurrentMediaTime() - timeOffset];
+        [caAnim setValue:[NSNumber numberWithBool:YES] forKey:WebKitAnimationBeginTimeSetKey];
+    }
+
+    NSString *animationID = animationIdentifier(animationName, property, index);
+
     [layer removeAnimationForKey:animationID];
     [layer addAnimation:caAnim forKey:animationID];
 
@@ -1577,7 +1590,7 @@ bool GraphicsLayerCA::removeCAAnimationFromLayer(AnimatedPropertyID property, co
 {
     PlatformLayer* layer = animatedLayer(property);
 
-    String animationID = animationIdentifier(animationName, index);
+    NSString *animationID = animationIdentifier(animationName, property, index);
 
     if (![layer animationForKey:animationID])
         return false;
@@ -1613,13 +1626,16 @@ static void copyAnimationProperties(CAPropertyAnimation* from, CAPropertyAnimati
 #if HAVE_MODERN_QUARTZCORE
     [to setValueFunction:[from valueFunction]];
 #endif
+
+    if (id object = [from valueForKey:WebKitAnimationBeginTimeSetKey])
+        [to setValue:object forKey:WebKitAnimationBeginTimeSetKey];
 }
 
 void GraphicsLayerCA::pauseCAAnimationOnLayer(AnimatedPropertyID property, const String& animationName, int index, double timeOffset)
 {
     PlatformLayer* layer = animatedLayer(property);
 
-    String animationID = animationIdentifier(animationName, index);
+    NSString *animationID = animationIdentifier(animationName, property, index);
 
     CAAnimation *caAnim = [layer animationForKey:animationID];
     if (!caAnim)