[cc] Pausing Keyframes now correctly handles delay and multiple pauses

Existing pause time application in cc::KeyframeModel is incorrect.

To convert the client pause time to monotonic time, we used to add the
|time_offset_| (1) but not add |total_pause_duration_| (2). The new logic now
excludes (1) and adds (2). This ensures pause time and monotonic time
have the same base clock.

The original conversion was incorrect because:
a. Pause time should have the same base clock as monotonic time since we compute
   total_paused_duration_ as difference between the two.
b. Blink provides its animation's current time as pause offset which maps to
   the concept of local time in cc. So to convert it to monotonic time we don't
   need to include time_offset_.


Without this correction animations with start-delay or multiple pause/unpause
cycles would lead to incorrect pause time calculation. I suspect these issues
flew under the radar mainly because pausing is only ever used for simple test
cases not involving either of these conditions.

Bug: 840364
TEST: cc/animation/keyframe_model_unittest.cc

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Id52ca7a3f3d63899864b7153470c7ef64c582d29
Reviewed-on: https://chromium-review.googlesource.com/c/1045126
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619437}

cc/animation/keyframe_model.cc

@@ -143,14 +143,10 @@ void KeyframeModel::SetRunState(RunState run_state,
 }
 
 void KeyframeModel::Pause(base::TimeDelta pause_offset) {
-  // Convert pause offset to monotonic time.
-
-  // TODO(crbug.com/840364): This conversion is incorrect. pause_offset is
-  // actually a local time so to convert it to monotonic time we should include
-  // total_paused_duration_ but exclude time_offset. The current calculation is
-  // is incorrect for animations that have start-delay or are paused and
-  // unpaused multiple times.
-  base::TimeTicks monotonic_time = pause_offset + start_time_ + time_offset_;
+  // Convert pause offset which is in local time to monotonic time.
+  // TODO(yigu): This should be scaled by playbackrate. http://crbug.com/912407
+  base::TimeTicks monotonic_time =
+      pause_offset + start_time_ + total_paused_duration_;
   SetRunState(PAUSED, monotonic_time);
 }
 
@@ -242,8 +238,7 @@ base::TimeDelta KeyframeModel::ConvertMonotonicTimeToLocalTime(
     return base::TimeDelta();
 
   // If we're paused, time is 'stuck' at the pause time.
-  base::TimeTicks time =
-      (run_state_ == PAUSED) ? pause_time_ - time_offset_ : monotonic_time;
+  base::TimeTicks time = (run_state_ == PAUSED) ? pause_time_ : monotonic_time;
   return time - start_time_ - total_paused_duration_;
 }
 

cc/animation/keyframe_model_unittest.cc

@@ -438,16 +438,67 @@ TEST(KeyframeModelTest, TrimTimeNegativeTimeOffsetReverse) {
                 .InSecondsF());
 }
 
+TEST(KeyframeModelTest, TrimTimePauseBasic) {
+  std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1));
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.5));
+  // When paused, the time returned is always the pause time
+  EXPECT_EQ(0.5,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(-1))
+                .InSecondsF());
+  EXPECT_EQ(0.5,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.2))
+                .InSecondsF());
+  EXPECT_EQ(0.5,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(2))
+                .InSecondsF());
+}
+
+TEST(KeyframeModelTest, TrimTimePauseAffectedByDelay) {
+  std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1));
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+  // Pause time is in local time so delay should apply on top of it.
+  keyframe_model->set_time_offset(TimeDelta::FromSecondsD(-0.2));
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.5));
+  EXPECT_EQ(0.3,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.1))
+                .InSecondsF());
+
+  keyframe_model->set_time_offset(TimeDelta::FromSecondsD(0.2));
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.5));
+  EXPECT_EQ(0.7,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.1))
+                .InSecondsF());
+}
+
+TEST(KeyframeModelTest, TrimTimePauseNotAffectedByStartTime) {
+  std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1));
+  keyframe_model->set_fill_mode(KeyframeModel::FillMode::NONE);
+  // Pause time is in local time so start time should not affect it.
+  keyframe_model->set_start_time(TicksFromSecondsF(0.2));
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.5));
+  EXPECT_EQ(0.5,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.1))
+                .InSecondsF());
+
+  keyframe_model->set_start_time(TicksFromSecondsF(0.4));
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.5));
+  EXPECT_EQ(0.5,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.1))
+                .InSecondsF());
+}
+
 TEST(KeyframeModelTest, TrimTimePauseResume) {
   std::unique_ptr<KeyframeModel> keyframe_model(CreateKeyframeModel(1));
   keyframe_model->SetRunState(KeyframeModel::RUNNING, TicksFromSecondsF(0.0));
   EXPECT_EQ(0,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.0))
                 .InSecondsF());
-  EXPECT_EQ(0.5,
-            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.5))
+  EXPECT_EQ(0.4,
+            keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.4))
                 .InSecondsF());
-  keyframe_model->SetRunState(KeyframeModel::PAUSED, TicksFromSecondsF(0.5));
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.5));
   EXPECT_EQ(
       0.5, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(1024.0))
                .InSecondsF());
@@ -459,6 +510,15 @@ TEST(KeyframeModelTest, TrimTimePauseResume) {
   EXPECT_EQ(
       1, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(1024.5))
              .InSecondsF());
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.6));
+  EXPECT_EQ(
+      0.6, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(2000.0))
+               .InSecondsF());
+  keyframe_model->SetRunState(KeyframeModel::RUNNING,
+                              TicksFromSecondsF(2000.0));
+  EXPECT_EQ(
+      0.7, keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(2000.1))
+               .InSecondsF());
 }
 
 TEST(KeyframeModelTest, TrimTimePauseResumeReverse) {
@@ -471,7 +531,7 @@ TEST(KeyframeModelTest, TrimTimePauseResumeReverse) {
   EXPECT_EQ(0.5,
             keyframe_model->TrimTimeToCurrentIteration(TicksFromSecondsF(0.5))
                 .InSecondsF());
-  keyframe_model->SetRunState(KeyframeModel::PAUSED, TicksFromSecondsF(0.25));
+  keyframe_model->Pause(base::TimeDelta::FromSecondsD(0.25));
   EXPECT_EQ(0.75, keyframe_model
                       ->TrimTimeToCurrentIteration(TicksFromSecondsF(1024.0))
                       .InSecondsF());