Web Animations CSS: More cleaning of new CompositorAnimation code.

Source/core/animation/CompositorAnimations.cpp

Index: Source/core/animation/CompositorAnimations.cpp
diff --git a/Source/core/animation/CompositorAnimations.cpp b/Source/core/animation/CompositorAnimations.cpp
index 306e1e709e05e428f5019a6962fdc8055cde9a86..0f34659737234d2d8c96db8f34c3517fe358b990 100644
--- a/Source/core/animation/CompositorAnimations.cpp
+++ b/Source/core/animation/CompositorAnimations.cpp
@@ -43,17 +43,12 @@
 #include "public/platform/Platform.h"
 #include "public/platform/WebAnimation.h"
 #include "public/platform/WebCompositorSupport.h"
-#include "public/platform/WebFilterAnimationCurve.h"
 #include "public/platform/WebFloatAnimationCurve.h"
 #include "public/platform/WebFloatKeyframe.h"
-#include "public/platform/WebTransformAnimationCurve.h"
-#include "public/platform/WebTransformKeyframe.h"
-#include "public/platform/WebTransformOperations.h"
 
 #include <algorithm>
 #include <cmath>
 
-
 namespace WebCore {
 
 // -----------------------------------------------------------------------
@@ -174,14 +169,14 @@ PassOwnPtr<Vector<CSSPropertyID> > CompositorAnimationsKeyframeEffectHelper::get
 }
 
 PassOwnPtr<CompositorAnimationsKeyframeEffectHelper::KeyframeValues> CompositorAnimationsKeyframeEffectHelper::getKeyframeValuesForProperty(
-    const KeyframeAnimationEffect* effect, CSSPropertyID id, double zero, double scale, bool reverse)
+    const KeyframeAnimationEffect* effect, CSSPropertyID id, double scale, bool reverse)
 {
     const_cast<KeyframeAnimationEffect*>(effect)->ensureKeyframeGroups();
-    return getKeyframeValuesForProperty(effect->m_keyframeGroups->get(id), zero, scale, reverse);
+    return getKeyframeValuesForProperty(effect->m_keyframeGroups->get(id), scale, reverse);
 }
 
 PassOwnPtr<CompositorAnimationsKeyframeEffectHelper::KeyframeValues> CompositorAnimationsKeyframeEffectHelper::getKeyframeValuesForProperty(
-    const KeyframeAnimationEffect::PropertySpecificKeyframeGroup* group, double zero, double scale, bool reverse)
+    const KeyframeAnimationEffect::PropertySpecificKeyframeGroup* group, double scale, bool reverse)
 {
     OwnPtr<CompositorAnimationsKeyframeEffectHelper::KeyframeValues> values = adoptPtr(
         new CompositorAnimationsKeyframeEffectHelper::KeyframeValues());
@@ -189,10 +184,7 @@ PassOwnPtr<CompositorAnimationsKeyframeEffectHelper::KeyframeValues> CompositorA
     for (size_t i = 0; i < group->m_keyframes.size(); i++) {
         KeyframeAnimationEffect::PropertySpecificKeyframe* frame = group->m_keyframes[i].get();
 
-        double offset = zero + frame->offset() * scale;
-        if (reverse)
-            offset = zero + (1 - frame->offset()) * scale;
-
+        double offset = (reverse ? (1 - frame->offset()) : frame->offset()) * scale;
         values->append(std::make_pair(offset, frame->value()));
     }
     if (reverse)
@@ -218,7 +210,7 @@ bool CompositorAnimationsImpl::isCandidateForCompositor(const Keyframe& keyframe
         case CSSPropertyOpacity:
         case CSSPropertyWebkitFilter: // FIXME: What about CSSPropertyFilter?
         case CSSPropertyWebkitTransform:
-            return true;
+            continue;
         default:
             return false;
         }
@@ -238,29 +230,15 @@ bool CompositorAnimationsImpl::isCandidateForCompositor(const KeyframeAnimationE
 
 bool CompositorAnimationsImpl::isCandidateForCompositor(const Timing& timing, const KeyframeAnimationEffect::KeyframeVector& frames)
 {
-    // FIXME: Support more then FillModeNone.
-    if (timing.fillMode != Timing::FillModeNone)
-        return false;
-
-    if (!timing.hasIterationDuration)
-        return false;
-
-    // FIXME: Support non-zero iteration start.
-    if (timing.iterationStart != 0.0)
-        return false;
-
-    // FIXME: Compositor only supports integer iteration counts.
-    if (!std::isfinite(timing.iterationCount) || (timing.iterationCount < 0) || (std::floor(timing.iterationCount) != timing.iterationCount))
-        return false;
 
-    // FIXME: Support positive startDelay and iterations.
-    if (timing.iterationCount != 1 && timing.startDelay > 0.0)
+    CompositorTiming out;
+    if (!convertTimingForCompositor(timing, out))
         return false;
 
     return isCandidateForCompositor(*timing.timingFunction.get(), frames);
 }
 
-bool CompositorAnimationsImpl::isCandidateForCompositor(const TimingFunction& timingFunction, const KeyframeAnimationEffect::KeyframeVector& frames, double frameOffset)
+bool CompositorAnimationsImpl::isCandidateForCompositor(const TimingFunction& timingFunction, const KeyframeAnimationEffect::KeyframeVector& frames, bool isNestedCall)
 {
     switch (timingFunction.type()) {
     case TimingFunction::LinearFunction:
@@ -268,11 +246,10 @@ bool CompositorAnimationsImpl::isCandidateForCompositor(const TimingFunction& ti
 
     case TimingFunction::CubicBezierFunction:
         // Can have a cubic if we don't have to split it (IE only have two frames).
-        if (frames.size() != 2)
+        if (!isNestedCall && (frames.size() != 2))
             return false;
 
-        if (frames[0]->offset() - frameOffset)
-            return false;
+        ASSERT(!frames.size() || (frames[0]->offset() == 0.0 && frames[1]->offset() == 1.0));
 
         return true;
 
@@ -284,7 +261,7 @@ bool CompositorAnimationsImpl::isCandidateForCompositor(const TimingFunction& ti
         // generates. These chained segments are only one level deep and have
         // one timing function per frame.
         const ChainedTimingFunction* chained = static_cast<const ChainedTimingFunction*>(&timingFunction);
-        if (frameOffset)
+        if (isNestedCall)
             return false;
 
         if (!chained->m_segments.size())
@@ -300,10 +277,7 @@ bool CompositorAnimationsImpl::isCandidateForCompositor(const TimingFunction& ti
                 return false;
 
             KeyframeAnimationEffect::KeyframeVector timingFrames;

[Steve Block, 2013/11/13 23:17:01]

Yes, I think we should convert this function to take 'const
KeyframeAnimationEffect::KeyframeVector*' and pass null here

[mithro-old, 2013/11/14 02:11:46]

Done.

-            timingFrames.append(frames[timeIndex]);
-            timingFrames.append(frames[timeIndex + 1]);
-
-            if (!isCandidateForCompositor(*(segment.m_timingFunction.get()), timingFrames, segment.m_min))
+            if (!isCandidateForCompositor(*segment.m_timingFunction.get(), timingFrames, true))
                 return false;
         }
         return true;
@@ -314,6 +288,59 @@ bool CompositorAnimationsImpl::isCandidateForCompositor(const TimingFunction& ti
     return false;
 }
 
+bool CompositorAnimationsImpl::convertTimingForCompositor(const Timing& timing, CompositorTiming& out)
+{
+    timing.assertValid();
+    // FIXME: Shouldn't this be asserted in timing.assertValid?

[Steve Block, 2013/11/13 23:17:01]

Good question. It should always be true right now, as CSS always produces a
timing function. As for the general model case, I'm pretty sure it will remain
true then too. So please fix this as part of this patch. We can always revisit
later if required.

[mithro-old, 2013/11/14 02:11:46]

Done.

+    ASSERT(timing.timingFunction);
+
+    // FIXME: Support positive startDelay
+    if (timing.startDelay > 0.0)
+        return false;
+
+    // All fill modes are supported (the calling code handles them).
+
+    // FIXME: Support non-zero iteration start.
+    if (timing.iterationStart != 0.0)

[Steve Block, 2013/11/13 23:17:01]

if (timing.iterationStart)

Ugly I know, but that's the style guide

[mithro-old, 2013/11/14 02:11:46]

Done.

Normally the style checker picks this up, wonder why it didn't catch this one.

+        return false;
+
+    // FIXME: Compositor only supports finite, integer iteration counts.
+    if (!std::isfinite(timing.iterationCount) || (std::floor(timing.iterationCount) != timing.iterationCount))
+        return false;
+
+    if (!timing.iterationDuration)
+        return false;
+
+    if (timing.playbackRate <= 0)

[Steve Block, 2013/11/13 23:17:01]

Why doesn't this one get a FIXME?

[mithro-old, 2013/11/14 02:11:46]

Does now! Changed to == 1.0 as Doug requested.

+        return false;
+
+    // All directions are supported.
+    out.reverse = (timing.direction == Timing::PlaybackDirectionReverse
+        || timing.direction == Timing::PlaybackDirectionAlternateReverse);
+    out.alternate = (timing.direction == Timing::PlaybackDirectionAlternate
+        || timing.direction == Timing::PlaybackDirectionAlternateReverse);

[Steve Block, 2013/11/13 23:17:01]

Move these down to line 334 where they're used, to avoid doing this if possible.

[mithro-old, 2013/11/14 02:11:46]

Done.

+
+    out.scaledDuration = timing.iterationDuration / timing.playbackRate;

[dstockwell, 2013/11/13 11:54:04]

I think we should just bail out if playbackRate is not 1. We can consider how
best to implement this when we need it.

[mithro-old, 2013/11/13 12:01:43]

Trivial to implement, but can remove it you want.

[dstockwell, 2013/11/13 23:29:31]

Remove it.

[mithro-old, 2013/11/14 02:11:46]

Done.

+    ASSERT(out.scaledDuration > 0);
+
+    double scaledStartDelay = timing.startDelay / timing.playbackRate;
+    ASSERT(scaledStartDelay <= 0);
+
+    int pastIterations = std::floor(std::abs(scaledStartDelay) / out.scaledDuration);

[dstockwell, 2013/11/13 11:54:04]

It's not really clear to me what's going on here. I'd assumed after our earlier
discussion that it was possible to just map a negative start delay to the time
offset.

[mithro-old, 2013/11/13 12:01:43]

When start delay greater than iteration duration means you need to trim off the
iterations which have already happened in the past.

Then if you are alternating, you need to fix the reverse value.

[dstockwell, 2013/11/13 23:29:31]

OK. Then I think something like skippedIterations is better than pastIterations.

[mithro-old, 2013/11/14 02:11:46]

Done.

+    ASSERT(pastIterations >= 0);
+    if (pastIterations >= timing.iterationCount)
+        return false;
+
+    if (out.alternate && (pastIterations % 2))
+        out.reverse = !out.reverse;
+
+    out.adjustedIterationCount = std::floor(timing.iterationCount) - pastIterations;
+    ASSERT(out.adjustedIterationCount > 0);

[Steve Block, 2013/11/13 23:17:01]

What if timing.iterationCount is zero?

For example ...
timing.iterationCount = 0
timing.iterationDuration = 1
timing.startDelay = 0
timing.playbackRate = 1

-> out.scaledDuration = 1
-> scaledStartDelay = 0
-> pastIterations = 0
-> out.adjustedIterationCount = 0

We'll hit this assert. I imagine we shouldn't composite if the iteration count
is zero?

[dstockwell, 2013/11/13 23:29:31]

Right, bail out if iteration count is 0.

[mithro-old, 2013/11/14 02:11:46]

Actually we'd bail at line 331, in the zero case skippedIterations ==
timing.iterationCount.
if (skippedIterations >= timing.iterationCount)

Also added a check for iterationCount == 0 at the finite/integer area because
catching it on line 331 is a bit obscure.

[Steve Block, 2013/11/14 02:28:08]

Sounds good. This will be required once we allow positive start delay, as I
could use a positive start delay to avoid the check on line 331 catching this.

+
+    out.scaledTimeOffset = scaledStartDelay + pastIterations * out.scaledDuration;
+    ASSERT(out.scaledTimeOffset <= 0);
+    return true;
+}
 
 namespace {
 
@@ -407,7 +434,7 @@ void CompositorAnimationsImpl::addKeyframesToCurve(PlatformAnimationCurveType& c
                 const ChainedTimingFunction* chained = static_cast<const ChainedTimingFunction*>(&timingFunction);
                 // ChainedTimingFunction criteria was checked in isCandidate,
                 // assert it is valid.
-                ASSERT(values.size() == chained->m_segments.size()+1);
+                ASSERT(values.size() == chained->m_segments.size() + 1);
                 ASSERT(values[i].first == chained->m_segments[i].m_min);
 
                 keyframeTimingFunction = chained->m_segments[i].m_timingFunction.get();
@@ -429,24 +456,19 @@ void CompositorAnimationsImpl::addKeyframesToCurve(PlatformAnimationCurveType& c
 void CompositorAnimationsImpl::getCompositorAnimations(
     const Timing& timing, const KeyframeAnimationEffect& effect, Vector<OwnPtr<blink::WebAnimation> >& animations)
 {
-
-    bool reverse = (timing.direction == Timing::PlaybackDirectionReverse
-        || timing.direction == Timing::PlaybackDirectionAlternateReverse);
+    CompositorTiming compositorTiming;
+    bool timingValid = convertTimingForCompositor(timing, compositorTiming);
+    ASSERT(timingValid);

[Steve Block, 2013/11/13 23:17:01]

I think this will need to be an ASSERT_UNUSED for release builds.

[mithro-old, 2013/11/14 02:11:46]

Done.

 
     RefPtr<TimingFunction> timingFunction = timing.timingFunction;
-    if (reverse) {
+    if (compositorTiming.reverse)
         timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get());

[dstockwell, 2013/11/13 23:29:31]

Hmm, why do we need to reverse the timing function? Couldn't we just flip the
value of reverse?

[dstockwell, 2013/11/13 23:41:05]

Probably because reverse isn't passed through. Something seems odd given that
the interface supports alternate.

[Steve Block, 2013/11/13 23:56:23]

That's my understanding too. The compositor interface needs help, but for now we
should just wire it up to get things working. (This is why I was loath to add
reversing logic to TimingFunction, as it's just a temporary band-aid)

[mithro-old, 2013/11/14 02:11:46]

Correct. If we could just pass reverse, life would be much more peachy :).

[mithro-old, 2013/11/14 02:11:46]

Correct.

[mithro-old, 2013/11/14 02:11:46]

Because it's not passed to the compositor.

To simulate reverse, we reverse the timing functions and flip the locations of
the keyframes (see the tests if you want to look into it further).

-    }
 
     OwnPtr<Vector<CSSPropertyID> > properties = CompositorAnimationsKeyframeEffectHelper::getProperties(&effect);
     for (size_t i = 0; i < properties->size(); i++) {
-        double iterationDuration = timing.iterationDuration / timing.playbackRate;
-
-        double zero = timing.startDelay / timing.playbackRate;
-        double keyframeZero = std::max(0.0, zero);
 
         OwnPtr<CompositorAnimationsKeyframeEffectHelper::KeyframeValues> values = CompositorAnimationsKeyframeEffectHelper::getKeyframeValuesForProperty(
-            &effect, properties->at(i), keyframeZero, iterationDuration, reverse);
+            &effect, properties->at(i), compositorTiming.scaledDuration, compositorTiming.reverse);
 
         blink::WebAnimation::TargetProperty targetProperty;
         OwnPtr<blink::WebAnimationCurve> curve;
@@ -479,20 +501,9 @@ void CompositorAnimationsImpl::getCompositorAnimations(
         OwnPtr<blink::WebAnimation> animation = adoptPtr(
             blink::Platform::current()->compositorSupport()->createAnimation(*curve, targetProperty));
 
-        ASSERT(std::floor(timing.iterationCount) == timing.iterationCount);
-        ASSERT(std::isfinite(timing.iterationCount));
-        double iterationCount = std::floor(timing.iterationCount);
-        if (zero < 0) {
-            int pastIterations = std::floor(std::abs(zero) / iterationDuration);
-
-            iterationCount = iterationCount - pastIterations;
-            animation->setTimeOffset(zero + pastIterations * iterationDuration);
-        }
-        animation->setIterations(iterationCount);
-
-        // PlaybackDirection direction
-        animation->setAlternatesDirection(
-            timing.direction == Timing::PlaybackDirectionAlternate || timing.direction == Timing::PlaybackDirectionAlternateReverse);
+        animation->setIterations(compositorTiming.adjustedIterationCount);
+        animation->setTimeOffset(compositorTiming.scaledTimeOffset);
+        animation->setAlternatesDirection(compositorTiming.alternate);
 
         animations.append(animation.release());
     }