New issue
Advanced search Search tips

Issue 737867 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocked on:
issue 876029



Sign in to add a comment

Rewrite Blink animation engine to use TimeDelta instead of double

Project Member Reported by alancutter@chromium.org, Jun 29 2017

Issue description

The animation code base uses double to store its time values. We should instead be using TimeDeltas to avoid floating point precision and to make time unit conversions more clear.
 
Owner: ----
Status: Available (was: Assigned)
This is a bit harder than just replacing double with TimeDelta it seems. We make extensive use of double's infinity and NaN values.

TimeDelta has Min() and Max() values that could serve as replacements but arithmetic will not function in the same way as for doubles potentially leading to bugs that are even harder to detect than unit mishandling.

One option could be to implement an AnimationTime class that wraps TimeDelta along with infinity and null states.
Project Member

Comment 2 by bugdroid1@chromium.org, May 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9b2b9391d69d42996ddb09564f7787218119c47

commit b9b2b9391d69d42996ddb09564f7787218119c47
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue May 01 13:55:44 2018

Convert blink::AnimationClock to use base::TimeTicks internally

Using a double timestamp is prone to errors, since nothing stops a
numerical quantity with the wrong units from being passed in. This CL is
part of a series to make more time interactions in Blink saner.

Bug: 737867, 763980
Change-Id: Id977edb1ad9511ecefadd38610e06f88fda8224e
Reviewed-on: https://chromium-review.googlesource.com/1033431
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555032}
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/animation_clock.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/animation_clock.h
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/animation_clock_test.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/animation_test.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/compositor_animations_test.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/document_timeline_test.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/effect_stack_test.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/animation/keyframe_effect_test.cc
[modify] https://crrev.com/b9b2b9391d69d42996ddb09564f7787218119c47/third_party/blink/renderer/core/page/page_animator.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e37e738ae45824cdb3ea608527500c1cf9316e62

commit e37e738ae45824cdb3ea608527500c1cf9316e62
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Mon Aug 20 20:12:33 2018

Introduce AnimationTimeDelta class

This class is intended to be used to facilitate the transition of Blink
animations from double-based times to TimeDelta-based times. This CL
introduces a dual-compiled class, AnimationTimeDelta, which is compiled
by default as double-based but can also be compiled to use
base::TimeDelta as the underlying value.

Bug: 737867
Change-Id: Ibba92d9eb860b20659b0897c505000a91d042988
Reviewed-on: https://chromium-review.googlesource.com/1175859
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584547}
[modify] https://crrev.com/e37e738ae45824cdb3ea608527500c1cf9316e62/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/e37e738ae45824cdb3ea608527500c1cf9316e62/third_party/blink/renderer/core/animation/BUILD.gn
[add] https://crrev.com/e37e738ae45824cdb3ea608527500c1cf9316e62/third_party/blink/renderer/core/animation/animation_time_delta.cc
[add] https://crrev.com/e37e738ae45824cdb3ea608527500c1cf9316e62/third_party/blink/renderer/core/animation/animation_time_delta.h
[add] https://crrev.com/e37e738ae45824cdb3ea608527500c1cf9316e62/third_party/blink/renderer/core/animation/animation_time_delta_test.cc
[modify] https://crrev.com/e37e738ae45824cdb3ea608527500c1cf9316e62/third_party/blink/renderer/core/core.gni

Blockedon: 876029
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3

commit 504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Sep 07 16:45:04 2018

Make blink::Timing::iteration_duration use base:Optional<AnimationTimeDelta>

This CL converts most of the code referring to iteration_duration
to use the transitional AnimationTimeDelta class. As it can be null (meaning
'auto'), it is wrapped in a base::Optional where necessary.

Additional important changes resulting from this:

  * animation_browsertest.js is changed because a sufficiently large
    duration would now be treated as 'infinite', and the test doesn't want
    that (as it finish()es the animation).
  * timing_input.cc can now use UpdateValueIfChanged for iteration_duration
    because base::Optional has equality semantics that work for it.

Bug: 737867
Change-Id: Ifa322d8a3672b2fe3ba93a0fd704636f383f2ef5
Reviewed-on: https://chromium-review.googlesource.com/1175860
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589547}
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/chrome/test/data/webui/settings/animation_browsertest.js
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/animation_effect.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/animation_effect.h
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/animation_effect_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/animation_sim_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/animation_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/compositor_animations.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/compositor_animations.h
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/compositor_animations_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/css/css_animations.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/css/css_timing_data.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/document_timeline_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/effect_model.h
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/effect_stack_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/keyframe_effect_model.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/keyframe_effect_model.h
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/keyframe_effect_model_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/keyframe_effect_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/timing.h
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/timing_calculations.h
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/timing_input.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/core/animation/timing_input_test.cc
[modify] https://crrev.com/504c77dd2b8d7fe6a7c7627dc526739f7ba4eeb3/third_party/blink/renderer/modules/animationworklet/worklet_animation_test.cc

Sign in to add a comment