Web Animations CSS: More cleaning of new CompositorAnimation code.

Source/core/animation/CompositorAnimations.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 25 matching lines @@
 #include "core/animation/AnimatableValue.h"
 #include "core/animation/CompositorAnimationsImpl.h"
 #include "core/platform/animation/AnimationTranslationUtil.h"
 #include "core/rendering/CompositedLayerMapping.h"
 #include "core/rendering/RenderBoxModelObject.h"
 #include "core/rendering/RenderLayer.h"
 #include "core/rendering/RenderObject.h"
 #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 {
 
 // -----------------------------------------------------------------------
 // TimingFunctionReverser methods
 // -----------------------------------------------------------------------
 
 PassRefPtr<TimingFunction> CompositorAnimationsTimingFunctionReverser::reverse(const LinearTimingFunction* timefunc)
 {
     return const_cast<LinearTimingFunction*>(timefunc);
 }
@@ skipping 100 matching lines @@
 PassOwnPtr<Vector<CSSPropertyID> > CompositorAnimationsKeyframeEffectHelper::getProperties(
     const KeyframeAnimationEffect* effect)
 {
     const_cast<KeyframeAnimationEffect*>(effect)->ensureKeyframeGroups();
     OwnPtr<Vector<CSSPropertyID> > propertyIds = adoptPtr(new Vector<CSSPropertyID>(effect->m_keyframeGroups->size()));
     copyKeysToVector(*(effect->m_keyframeGroups.get()), *propertyIds.get());
     return propertyIds.release();
 }
 
 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());
 
     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)
         values->reverse();
 
     return values.release();
 }
 
 // -----------------------------------------------------------------------
 // CompositorAnimationsImpl
 // -----------------------------------------------------------------------
 
 bool CompositorAnimationsImpl::isCandidateForCompositor(const Keyframe& keyframe)
 {
     // Only replace mode can be accelerated
     if (keyframe.composite() != AnimationEffect::CompositeReplace)
         return false;
 
     // Check all the properties can be accelerated
     const PropertySet properties = keyframe.properties(); // FIXME: properties creates a whole new PropertySet!
     for (PropertySet::const_iterator it = properties.begin(); it != properties.end(); ++it) {
         switch (*it) {
         case CSSPropertyOpacity:
         case CSSPropertyWebkitFilter: // FIXME: What about CSSPropertyFilter?
         case CSSPropertyWebkitTransform:
-            return true;
+            continue;
         default:
             return false;
         }
     }
     return true;
 }
 
 bool CompositorAnimationsImpl::isCandidateForCompositor(const KeyframeAnimationEffect& effect)
 {
     const KeyframeAnimationEffect::KeyframeVector frames = effect.getFrames();
     for (size_t i = 0; i < frames.size(); ++i) {
         if (!isCandidateForCompositor(*frames[i].get()))
             return false;
     }
     return true;
 }
 
 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:
         return true;
 
     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;
 
     case TimingFunction::StepsFunction:
         return false;
 
     case TimingFunction::ChainedFunction: {
         // Currently we only support chained segments in the form the CSS code
         // 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())
             return false;
 
         if (frames.size() != chained->m_segments.size() + 1)
             return false;
 
         for (size_t timeIndex = 0; timeIndex < chained->m_segments.size(); timeIndex++) {
             const ChainedTimingFunction::Segment& segment = chained->m_segments[timeIndex];
 
             if (frames[timeIndex]->offset() != segment.m_min || frames[timeIndex + 1]->offset() != segment.m_max)
                 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;
     }
     default:
         ASSERT_NOT_REACHED();
     };
     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 {
 
 template<typename PlatformAnimationKeyframeType>
 static PassOwnPtr<PlatformAnimationKeyframeType> createPlatformKeyframe(double offset, const AnimatableValue&)
 {
     // Only the specialized versions of this templated function (found in
     // the cpp file) should ever be called.
     // FIXME: COMPILE_ASSERT(false, CompositorAnimationsNonSpecializedCreateKeyframeShouldNeverBeUsed);
     ASSERT_NOT_REACHED();
@@ skipping 73 matching lines @@
             switch (timingFunction.type()) {
             case TimingFunction::LinearFunction:
             case TimingFunction::CubicBezierFunction:
                 keyframeTimingFunction = &timingFunction;
                 break;
 
             case TimingFunction::ChainedFunction: {
                 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();
                 break;
             }
             case TimingFunction::StepsFunction:
             default:
                 ASSERT_NOT_REACHED();
             }
         }
 
         ASSERT(!values[i].second->dependsOnUnderlyingValue());
         RefPtr<AnimatableValue> value = values[i].second->compositeOnto(0);
         OwnPtr<PlatformAnimationKeyframeType> keyframe = createPlatformKeyframe<PlatformAnimationKeyframeType>(values[i].first, *value.get());
         addKeyframeWithTimingFunction(curve, *keyframe.get(), keyframeTimingFunction);
     }
 }
 
 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;
         switch (properties->at(i)) {
         case CSSPropertyOpacity: {
             targetProperty = blink::WebAnimation::TargetPropertyOpacity;
 
             OwnPtr<blink::WebFloatAnimationCurve> floatCurve = adoptPtr(blink::Platform::current()->compositorSupport()->createFloatAnimationCurve());
             addKeyframesToCurve<blink::WebFloatAnimationCurve, blink::WebFloatKeyframe>(*floatCurve.get(), *values.get(), *timingFunction.get());
             curve = floatCurve.release();
@@ skipping 12 matching lines @@
         }
         default:
             ASSERT_NOT_REACHED();
             continue;
         }
         ASSERT(curve.get());
 
         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());
     }
 }
 
 } // namespace WebCore