[cc] Fix incorrect KeyframeModel::InEffect() calculation on the cc side

By spec [1], an animation effect is "in effect" if its active time is
not unresolved. However, if the effect is in the "after phase" and its
fill mode is neither "forwards" nor "both", its active time should be
unresolved which makes the effect not "in effect". Our cc logic doesn't
handle this case correctly.

This bug was not exposed because for regular animations we cancel the
animation on compositor in such scenario. But for worklet animations
that's not the case.

This patch adds the logic of calculating phases and the corresponding
active time based on the spec [2].

[1] https://drafts.csswg.org/web-animations/#in-effect
[2] https://drafts.csswg.org/web-animations/#active-time

Bug: 906675
Change-Id: I8872719432ede1eeeb1e7bdcf3b5f0312787c0d7
Reviewed-on: https://chromium-review.googlesource.com/c/1342598
Commit-Queue: Yi Gu <yigu@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614366}

cc/animation/keyframe_model.cc

@@ -168,12 +168,70 @@ bool KeyframeModel::IsFinishedAt(base::TimeTicks monotonic_time) const {
              (ConvertMonotonicTimeToLocalTime(monotonic_time) + time_offset_);
 }
 
+KeyframeModel::Phase KeyframeModel::CalculatePhase(
+    base::TimeDelta local_time) const {
+  base::TimeDelta before_active_boundary_time =
+      std::max(-time_offset_, base::TimeDelta());
+  if (local_time < before_active_boundary_time ||
+      (local_time == before_active_boundary_time && playback_rate_ < 0)) {
+    return KeyframeModel::Phase::BEFORE;
+  }
+  // Scaling the duration is against spec but needed to comply with the cc
+  // implementation. By spec (in blink) the playback rate is an Animation level
+  // concept but in cc it's per KeyframeModel. We grab the active time
+  // calculated here and later scale it with the playback rate in order to get a
+  // proper progress. Therefore we need to un-scale it here. This can be fixed
+  // once we scale the local time by playback rate. See
+  // https://crbug.com/912407.
+  base::TimeDelta active_duration =
+      curve_->Duration() * iterations_ / std::abs(playback_rate_);
+  // TODO(crbug.com/909794): By spec end time = max(start delay + duration +
+  // end delay, 0). The logic should be updated once "end delay" is supported.
+  base::TimeDelta active_after_boundary_time =
+      // Negative iterations_ represents "infinite iterations".
+      iterations_ >= 0
+          ? std::max(-time_offset_ + active_duration, base::TimeDelta())
+          : base::TimeDelta::Max();
+  if (local_time > active_after_boundary_time ||
+      (local_time == active_after_boundary_time && playback_rate_ > 0)) {
+    return KeyframeModel::Phase::AFTER;
+  }
+  return KeyframeModel::Phase::ACTIVE;
+}
+
+base::Optional<base::TimeDelta> KeyframeModel::CalculateActiveTime(
+    base::TimeTicks monotonic_time) const {
+  base::TimeDelta local_time = ConvertMonotonicTimeToLocalTime(monotonic_time);
+  KeyframeModel::Phase phase = CalculatePhase(local_time);
+  DCHECK(playback_rate_);
+  switch (phase) {
+    case KeyframeModel::Phase::BEFORE:
+      if (fill_mode_ == FillMode::BACKWARDS || fill_mode_ == FillMode::BOTH)
+        return std::max(local_time + time_offset_, base::TimeDelta());
+      return base::nullopt;
+    case KeyframeModel::Phase::ACTIVE:
+      return local_time + time_offset_;
+    case KeyframeModel::Phase::AFTER:
+      if (fill_mode_ == FillMode::FORWARDS || fill_mode_ == FillMode::BOTH) {
+        DCHECK_GE(iterations_, 0);
+        base::TimeDelta active_duration =
+            curve_->Duration() * iterations_ / std::abs(playback_rate_);
+        return std::max(std::min(local_time + time_offset_, active_duration),
+                        base::TimeDelta());
+      }
+      return base::nullopt;
+    default:
+      NOTREACHED();
+      return base::nullopt;
+  }
+}
+
 bool KeyframeModel::InEffect(base::TimeTicks monotonic_time) const {
-  return ConvertMonotonicTimeToLocalTime(monotonic_time) + time_offset_ >=
-             base::TimeDelta() ||
-         (fill_mode_ == FillMode::BOTH || fill_mode_ == FillMode::BACKWARDS);
+  return CalculateActiveTime(monotonic_time).has_value();
 }
 
+// TODO(yigu): Local time should be scaled by playback rate by spec.
+// https://crbug.com/912407.
 base::TimeDelta KeyframeModel::ConvertMonotonicTimeToLocalTime(
     base::TimeTicks monotonic_time) const {
   // When waiting on receiving a start time, then our global clock is 'stuck' at
@@ -190,17 +248,11 @@ base::TimeDelta KeyframeModel::ConvertMonotonicTimeToLocalTime(
 
 base::TimeDelta KeyframeModel::TrimTimeToCurrentIteration(
     base::TimeTicks monotonic_time) const {
-  base::TimeDelta local_time = ConvertMonotonicTimeToLocalTime(monotonic_time);
-  return TrimLocalTimeToCurrentIteration(local_time);
-}
-
-base::TimeDelta KeyframeModel::TrimLocalTimeToCurrentIteration(
-    base::TimeDelta local_time) const {
-  // Check for valid parameters
   DCHECK(playback_rate_);
   DCHECK_GE(iteration_start_, 0);
 
-  base::TimeDelta active_time = local_time + time_offset_;
+  DCHECK(InEffect(monotonic_time));
+  base::TimeDelta active_time = CalculateActiveTime(monotonic_time).value();
   base::TimeDelta start_offset = curve_->Duration() * iteration_start_;
 
   // Return start offset if we are before the start of the keyframe model
@@ -218,10 +270,6 @@ base::TimeDelta KeyframeModel::TrimLocalTimeToCurrentIteration(
   base::TimeDelta active_duration =
       repeated_duration / std::abs(playback_rate_);
 
-  // Check if we are past active duration
-  if (iterations_ > 0 && active_time >= active_duration)
-    active_time = active_duration;
-
   // Calculate the scaled active time
   base::TimeDelta scaled_active_time;
   if (playback_rate_ < 0) {

cc/animation/keyframe_model.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "cc/animation/animation_export.h"
 #include "cc/trees/element_id.h"
@@ -55,6 +56,8 @@ class CC_ANIMATION_EXPORT KeyframeModel {
 
   enum class FillMode { NONE, FORWARDS, BACKWARDS, BOTH, AUTO };
 
+  enum class Phase { BEFORE, ACTIVE, AFTER };
+
   static std::unique_ptr<KeyframeModel> Create(
       std::unique_ptr<AnimationCurve> curve,
       int keyframe_model_id,
@@ -202,8 +205,9 @@ class CC_ANIMATION_EXPORT KeyframeModel {
   base::TimeDelta ConvertMonotonicTimeToLocalTime(
       base::TimeTicks monotonic_time) const;
 
-  base::TimeDelta TrimLocalTimeToCurrentIteration(
-      base::TimeDelta local_time) const;
+  KeyframeModel::Phase CalculatePhase(base::TimeDelta local_time) const;
+  base::Optional<base::TimeDelta> CalculateActiveTime(
+      base::TimeTicks monotonic_time) const;
 
   std::unique_ptr<AnimationCurve> curve_;
 

cc/animation/keyframe_model_unittest.cc

@@ -350,7 +350,7 @@ TEST(KeyframeModelTest, TrimTimeStartTimeReverse) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1));
   keyframe_model->set_start_time(TicksFromSecondsF(4));
   keyframe_model->set_direction(KeyframeModel::Direction::REVERSE);
-  EXPECT_EQ(0,
+  EXPECT_EQ(1.0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.0))
                 .InSecondsF());
   EXPECT_EQ(1.0,
@@ -399,9 +399,6 @@ TEST(KeyframeModelTest, TrimTimeTimeOffsetReverse) {
   EXPECT_EQ(0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(1.0))
                 .InSecondsF());
-  EXPECT_EQ(0,
-            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(1.0))
-                .InSecondsF());
 }
 
 TEST(KeyframeModelTest, TrimTimeNegativeTimeOffset) {
@@ -427,7 +424,7 @@ TEST(KeyframeModelTest, TrimTimeNegativeTimeOffsetReverse) {
   keyframe_model->set_time_offset(TimeDelta::FromMilliseconds(-4000));
   keyframe_model->set_direction(KeyframeModel::Direction::REVERSE);
 
-  EXPECT_EQ(0,
+  EXPECT_EQ(1.0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.0))
                 .InSecondsF());
   EXPECT_EQ(1.0,
@@ -739,11 +736,12 @@ TEST(KeyframeModelTest, TrimTimePlaybackFast) {
 
 TEST(KeyframeModelTest, TrimTimePlaybackNormalReverse) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1, 2, -1));
-  EXPECT_EQ(0,
+  EXPECT_EQ(2.0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(-1.0))
                 .InSecondsF());
-  EXPECT_EQ(2, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0))
-                   .InSecondsF());
+  EXPECT_EQ(2.0,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0))
+                .InSecondsF());
   EXPECT_EQ(1.5,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.5))
                 .InSecondsF());
@@ -764,11 +762,12 @@ TEST(KeyframeModelTest, TrimTimePlaybackNormalReverse) {
 TEST(KeyframeModelTest, TrimTimePlaybackSlowReverse) {
   std::unique_ptr<KeyframeModel> keyframe_model(
       CreateKeyframeModel(1, 2, -0.5));
-  EXPECT_EQ(0,
+  EXPECT_EQ(2.0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(-1.0))
                 .InSecondsF());
-  EXPECT_EQ(2, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0))
-                   .InSecondsF());
+  EXPECT_EQ(2.0,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0))
+                .InSecondsF());
   EXPECT_EQ(1.75,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.5))
                 .InSecondsF());
@@ -799,11 +798,12 @@ TEST(KeyframeModelTest, TrimTimePlaybackSlowReverse) {
 
 TEST(KeyframeModelTest, TrimTimePlaybackFastReverse) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1, 2, -2));
-  EXPECT_EQ(0,
+  EXPECT_EQ(2.0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(-1.0))
                 .InSecondsF());
-  EXPECT_EQ(2, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0))
-                   .InSecondsF());
+  EXPECT_EQ(2.0,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0))
+                .InSecondsF());
   EXPECT_EQ(1.5,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.25))
                 .InSecondsF());
@@ -927,7 +927,7 @@ TEST(KeyframeModelTest, TrimTimeAlternateTwoIterationsPlaybackFast) {
 TEST(KeyframeModelTest, TrimTimeAlternateTwoIterationsPlaybackFastReverse) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(2, 2, 2));
   keyframe_model->set_direction(KeyframeModel::Direction::ALTERNATE_REVERSE);
-  EXPECT_EQ(0.0,
+  EXPECT_EQ(2.0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(-1.0))
                 .InSecondsF());
   EXPECT_EQ(2.0,
@@ -1193,9 +1193,12 @@ TEST(KeyframeModelTest,
 TEST(KeyframeModelTest, InEffectFillMode) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1));
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+  // Effect before start is not InEffect
   EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  // Effect at start is InEffect
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
-  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+  // Effect at end is not InEffect
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
 
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::FORWARDS);
   EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
@@ -1205,7 +1208,7 @@ TEST(KeyframeModelTest, InEffectFillMode) {
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::BACKWARDS);
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
-  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
 
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::BOTH);
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
@@ -1213,27 +1216,86 @@ TEST(KeyframeModelTest, InEffectFillMode) {
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
 }
 
-TEST(KeyframeModelTest, InEffectFillModePlayback) {
+TEST(KeyframeModelTest, InEffectFillModeNoneWithNegativePlaybackRate) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1, 1, -1));
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
   EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
-  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
 
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::FORWARDS);
   EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::BACKWARDS);
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::BOTH);
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+}
+
+TEST(KeyframeModelTest, InEffectFillModeWithIterations) {
+  std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(2, 1));
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
+
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::FORWARDS);
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
 
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::BACKWARDS);
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
+
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::BOTH);
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
+}
+
+TEST(KeyframeModelTest, InEffectFillModeWithInfiniteIterations) {
+  std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(-1, 1));
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
+
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::FORWARDS);
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
+
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::BACKWARDS);
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
 
   keyframe_model->set_fill_mode(KeyframeModel::FillMode::BOTH);
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
+}
+
+TEST(KeyframeModelTest, InEffectReverseWithIterations) {
+  std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(2, 1));
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+  // KeyframeModel::direction_ doesn't affect InEffect.
+  keyframe_model->set_direction(KeyframeModel::Direction::REVERSE);
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(-1.0)));
+  EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(0.0)));
   EXPECT_TRUE(keyframe_model->InEffect(TicksFromSecondsF(1.0)));
+  EXPECT_FALSE(keyframe_model->InEffect(TicksFromSecondsF(2.0)));
 }
 
 TEST(KeyframeModelTest, ToString) {

third_party/blink/web_tests/TestExpectations

@@ -5567,7 +5567,6 @@ crbug.com/886566 http/tests/csspaint/invalidation-content-image.html [ Pass Time
 
 # Enable AnimationWorklet tests for mainthread
 crbug.com/785940 [ Release ] animations/animationworklet/worklet-animation-currentTime.html [ Pass Failure ]
-crbug.com/887659 virtual/threaded/animations/animationworklet/worklet-animation-local-time-after-duration.html [ Failure ]
 
 # Sheriff 2018-09-25
 crbug.com/888609 [ Mac ] http/tests/devtools/coverage/gutter-css.js [ Pass Timeout ]

third_party/blink/web_tests/animations/animationworklet/worklet-animation-local-time-after-duration.html

@@ -14,7 +14,7 @@ registerAnimator("test_animator", class {
   animate(currentTime, effect) {
     // If an effect doesn't have fill-mode specified, setting its local time
     // beyond its duration makes the animation inactive.
-    effect.localTime = 5000;
+    effect.localTime = 2000;
   }
 });
 </script>
@@ -36,6 +36,7 @@ runInAnimationWorklet(
      { transform: 'translateY(200px)' }
     ], {
       duration: 1000,
+      delay: 1000
     }
   );