[cc] Rewrite keyframe model timing computation based on local time

Formalize the concept of local time and use it internally where we would
have done a one-off calculations before.

In this process I noticed and documented issues how pause time is being
applied. In particular pause time incorrectly include offset_time_ and
excludes total_paused_duration_. I suspect these issues flew under the
radar mainly because pausing is only ever used for simple test cases not
involving animations with start delays or multiple pause/unpause cycles.
I plan to fix this issues in a follow up patch.


Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I02030dbd5ef0656b2738353a6e468b81c9b259a8
Reviewed-on: https://chromium-review.googlesource.com/1037635
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574949}

cc/animation/keyframe_effect.cc

@@ -221,11 +221,8 @@ void KeyframeEffect::UpdateTickingState(UpdateTickingType type) {
 }
 
 void KeyframeEffect::Pause(base::TimeDelta pause_offset) {
-  for (auto& keyframe_model : keyframe_models_) {
-    base::TimeTicks pause_time = keyframe_model->time_offset() +
-                                 keyframe_model->start_time() + pause_offset;
-    keyframe_model->SetRunState(KeyframeModel::PAUSED, pause_time);
-  }
+  for (auto& keyframe_model : keyframe_models_)
+    keyframe_model->Pause(pause_offset);
 
   if (has_bound_element_animations()) {
     animation_->SetNeedsCommit();
@@ -260,12 +257,11 @@ void KeyframeEffect::AddKeyframeModel(
 
 void KeyframeEffect::PauseKeyframeModel(int keyframe_model_id,
                                         double time_offset) {
-  const base::TimeDelta time_delta = base::TimeDelta::FromSecondsD(time_offset);
+  const base::TimeDelta pause_offset =
+      base::TimeDelta::FromSecondsD(time_offset);
   for (auto& keyframe_model : keyframe_models_) {
     if (keyframe_model->id() == keyframe_model_id) {
-      keyframe_model->SetRunState(KeyframeModel::PAUSED,
-                                  time_delta + keyframe_model->start_time() +
-                                      keyframe_model->time_offset());
+      keyframe_model->Pause(pause_offset);
     }
   }
 

cc/animation/keyframe_model.cc

@@ -66,7 +66,7 @@ std::unique_ptr<KeyframeModel> KeyframeModel::CreateImplInstance(
   to_return->iteration_start_ = iteration_start_;
   to_return->start_time_ = start_time_;
   to_return->pause_time_ = pause_time_;
-  to_return->total_paused_time_ = total_paused_time_;
+  to_return->total_paused_duration_ = total_paused_duration_;
   to_return->time_offset_ = time_offset_;
   to_return->direction_ = direction_;
   to_return->playback_rate_ = playback_rate_;
@@ -121,7 +121,7 @@ void KeyframeModel::SetRunState(RunState run_state,
   const char* old_run_state_name = s_runStateNames[run_state_];
 
   if (run_state == RUNNING && run_state_ == PAUSED)
-    total_paused_time_ += (monotonic_time - pause_time_);
+    total_paused_duration_ += (monotonic_time - pause_time_);
   else if (run_state == PAUSED)
     pause_time_ = monotonic_time;
   run_state_ = run_state;
@@ -140,6 +140,18 @@ void KeyframeModel::SetRunState(RunState run_state,
       TRACE_STR_COPY(name_buffer), "State", TRACE_STR_COPY(state_buffer));
 }
 
+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_;
+  SetRunState(PAUSED, monotonic_time);
+}
+
 bool KeyframeModel::IsFinishedAt(base::TimeTicks monotonic_time) const {
   if (is_finished())
     return true;
@@ -152,39 +164,42 @@ bool KeyframeModel::IsFinishedAt(base::TimeTicks monotonic_time) const {
 
   return run_state_ == RUNNING && iterations_ >= 0 &&
          (curve_->Duration() * (iterations_ / std::abs(playback_rate_))) <=
-             (monotonic_time + time_offset_ - start_time_ - total_paused_time_);
+             (ConvertMonotonicTimeToLocalTime(monotonic_time) + time_offset_);
 }
 
 bool KeyframeModel::InEffect(base::TimeTicks monotonic_time) const {
-  return ConvertToActiveTime(monotonic_time) >= base::TimeDelta() ||
+  return ConvertMonotonicTimeToLocalTime(monotonic_time) + time_offset_ >=
+             base::TimeDelta() ||
          (fill_mode_ == FillMode::BOTH || fill_mode_ == FillMode::BACKWARDS);
 }
 
-base::TimeDelta KeyframeModel::ConvertToActiveTime(
+base::TimeDelta KeyframeModel::ConvertMonotonicTimeToLocalTime(
     base::TimeTicks monotonic_time) const {
-  // If we're just starting or we're waiting on receiving a start time,
-  // time is 'stuck' at the initial state.
+  // When waiting on receiving a start time, then our global clock is 'stuck' at
+  // the initial state.
   if ((run_state_ == STARTING && !has_set_start_time()) ||
-      needs_synchronized_start_time()) {
-    return time_offset_;
-  }
-
-  // Compute active time. If we're paused, time is 'stuck' at the pause time.
-  base::TimeTicks active_time =
-      (run_state_ == PAUSED) ? pause_time_ : (monotonic_time + time_offset_);
+      needs_synchronized_start_time())
+    return base::TimeDelta();
 
-  // Returned time should always be relative to the start time and should
-  // subtract all time spent paused.
-  return active_time - start_time_ - total_paused_time_;
+  // If we're paused, time is 'stuck' at the pause time.
+  base::TimeTicks time =
+      (run_state_ == PAUSED) ? pause_time_ - time_offset_ : monotonic_time;
+  return time - start_time_ - total_paused_duration_;
 }
 
 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 = ConvertToActiveTime(monotonic_time);
+  base::TimeDelta active_time = local_time + time_offset_;
   base::TimeDelta start_offset = curve_->Duration() * iteration_start_;
 
   // Return start offset if we are before the start of the keyframe model
@@ -254,7 +269,7 @@ void KeyframeModel::PushPropertiesTo(KeyframeModel* other) const {
       other->run_state_ == KeyframeModel::PAUSED) {
     other->run_state_ = run_state_;
     other->pause_time_ = pause_time_;
-    other->total_paused_time_ = total_paused_time_;
+    other->total_paused_duration_ = total_paused_duration_;
   }
 }
 

cc/animation/keyframe_model.h

@@ -95,6 +95,9 @@ class CC_ANIMATION_EXPORT KeyframeModel {
     time_offset_ = monotonic_time;
   }
 
+  // Pause the keyframe effect at local time |pause_offset|.
+  void Pause(base::TimeDelta pause_offset);
+
   Direction direction() { return direction_; }
   void set_direction(Direction direction) { direction_ = direction; }
 
@@ -168,7 +171,35 @@ class CC_ANIMATION_EXPORT KeyframeModel {
                 int group_id,
                 int target_property_id);
 
-  base::TimeDelta ConvertToActiveTime(base::TimeTicks monotonic_time) const;
+  // Return local time for this keyframe model given the absolute monotonic
+  // time.
+  //
+  // Local time represents the time value that is used to tick this keyframe
+  // model and is relative to its start time. It is closely related to the local
+  // time concept in web animations [1]. It is:
+  //  - for playing animation : wall time - start time - paused duration
+  //  - for paused animation  : paused time
+  //  - otherwise             : zero
+  //
+  // Here is small diagram that shows how active, local, and monotonic times
+  // relate to each other and to the run state.
+  //
+  //      run state   Starting  (R)unning  Paused (R) Paused (R)  Finished
+  //                    ^                                          ^
+  //                    |                                          |
+  // monotonic time  ------------------------------------------------->
+  //                    |                                          |
+  //     local time     +-----------------+      +---+      +--------->
+  //                    |                                          |
+  //    active time     +          +------+      +---+      +------+
+  //                      (-offset)
+  //
+  // [1] https://drafts.csswg.org/web-animations/#local-time-section
+  base::TimeDelta ConvertMonotonicTimeToLocalTime(
+      base::TimeTicks monotonic_time) const;
+
+  base::TimeDelta TrimLocalTimeToCurrentIteration(
+      base::TimeDelta local_time) const;
 
   std::unique_ptr<AnimationCurve> curve_;
 
@@ -199,12 +230,12 @@ class CC_ANIMATION_EXPORT KeyframeModel {
   bool needs_synchronized_start_time_;
   bool received_finished_event_;
 
-  // These are used in TrimTimeToCurrentIteration to account for time
+  // These are used when converting monotonic to local time to account for time
   // spent while paused. This is not included in AnimationState since it
   // there is absolutely no need for clients of this controller to know
   // about these values.
   base::TimeTicks pause_time_;
-  base::TimeDelta total_paused_time_;
+  base::TimeDelta total_paused_duration_;
 
   // KeyframeModels lead dual lives. An active keyframe model will be
   // conceptually owned by two controllers, one on the impl thread and one on