implement effect-level easing

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: hiro)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox47 fixed)

Details

Attachments

(15 attachments, 30 obsolete attachments)

3.33 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.41 KB, patch
heycam
: review+
Details | Diff | Splinter Review
8.74 KB, patch
heycam
: review+
Details | Diff | Splinter Review
10.27 KB, patch
hiro
: review+
Details | Diff | Splinter Review
13.30 KB, patch
hiro
: review+
Details | Diff | Splinter Review
8.08 KB, patch
hiro
: review+
Details | Diff | Splinter Review
7.36 KB, patch
hiro
: review+
Details | Diff | Splinter Review
1.05 KB, patch
hiro
: review+
Details | Diff | Splinter Review
2.19 KB, patch
hiro
: review+
Details | Diff | Splinter Review
949 bytes, text/html
Details
2.26 KB, text/html
Details
11.35 KB, patch
hiro
: review+
Tomcat
: checkin+
Details | Diff | Splinter Review
2.20 KB, patch
hiro
: review+
Tomcat
: checkin+
Details | Diff | Splinter Review
2.57 KB, patch
hiro
: review+
Tomcat
: checkin+
Details | Diff | Splinter Review
21.12 KB, patch
hiro
: review+
Tomcat
: checkin+
Details | Diff | Splinter Review
We don't yet support easing applied to an animation as a whole.  Once we accept and store easing from KeyframeEffectReadOnly's constructor's KeyframeEffectOptions argument in bug 1215406, we'll need to actually take it into account in the effect.
(Assignee)

Comment 1

2 years ago
Created attachment 8693400 [details] [diff] [review]
Pass easing in KeyframeEffectOptions to KeyframeEffect constructor.
Assignee: nobody → hiikezoe
Comment on attachment 8693400 [details] [diff] [review]
Pass easing in KeyframeEffectOptions to KeyframeEffect constructor.

I'm not sure this is quite right. The easing we need to introduce is an extra level of easing. Basically, we should add an mEasing (or mTimingFunction) member to *KeyframeEffectReadOnly* (or AnimationEffectReadOnly). Then we should use that when we calculate mProgress in GetComputedTimingAt.

It's this step from the spec:
http://w3c.github.io/web-animations/#calculating-the-transformed-time

Of course, we need to store the value passed-in as well but from a quick glance at this patch it seems like this was using the effect-level easing on a keyframe-level? We need to do both effect-level easing AND keyframe-level easing. We already do the keyframe-level easing but we don't do effect-level easing yet.
(Assignee)

Comment 3

2 years ago
Thanks for pointing it out! I did totally misread the spec!
Depends on: 1214536
(Assignee)

Updated

2 years ago
Blocks: 1228915
(Assignee)

Comment 4

2 years ago
Created attachment 8693823 [details] [diff] [review]
Part 1: Store easing function in KeyframeEffectOptions as a member of AnimationTiming

The reason why we use Maybe<> to store the function is that CSS
animations/transitions do not have the function property.
Attachment #8693400 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Created attachment 8693824 [details] [diff] [review]
Part 2: Change the first argument of ToTimingFunction a Maybe<ComputedTimingFunction>.

As a result of this change we can also use ToTimingFunction for animation
effect's timing function.
(Assignee)

Comment 6

2 years ago
Created attachment 8693825 [details] [diff] [review]
Part 3: Add easing function to laryer::Animation.

null_t is also added because CSS animations/transitions do not have the
easing function property.
(Assignee)

Updated

2 years ago
Attachment #8693824 - Attachment description: Bug 1096773 - Part 2: Change the first argument of ToTimingFunction a Maybe<ComputedTimingFunction>. → Part 2: Change the first argument of ToTimingFunction a Maybe<ComputedTimingFunction>.
(Assignee)

Comment 7

2 years ago
Created attachment 8693826 [details] [diff] [review]
Part 4: Add LayerAnimationUtils.

we need to a new class to share the function which convert TimingFunction
to ComputedTimingFunction for either keyframe's timing function or keyframe
effect's timing function.
(Assignee)

Comment 8

2 years ago
Created attachment 8693827 [details] [diff] [review]
Part 5: Set keyframe effect's timing function on compositor.
(Assignee)

Comment 9

2 years ago
Created attachment 8693828 [details] [diff] [review]
Part 8: Calculate transformed progress using animation effect's timing function
(Assignee)

Comment 10

2 years ago
Created attachment 8693830 [details] [diff] [review]
Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

This test relies on bug 1214536.
Other tests will be handled in bug 1228915.
(Assignee)

Comment 11

2 years ago
Created attachment 8693880 [details] [diff] [review]
Part 2: Change the first argument of ToTimingFunction a Maybe<ComputedTimingFunction>.
Attachment #8693824 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8693823 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693880 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693825 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693826 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693827 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693828 - Flags: review?(bbirtles)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8693830 [details] [diff] [review]
Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

Normal tests will be coming in bug 1228915.
Attachment #8693830 - Flags: review?(cam)
Attachment #8693828 - Flags: review?(bbirtles) → review+
I discussed this bug with hiro yesterday and I think we agreed that we should make the handling of linear timing functions consistent.

In particular, currently we represent linear timing functions as a type of cubic bezier:

  https://dxr.mozilla.org/mozilla-central/rev/a8965ae93c5d098a4f91ad9da72150bb43df07a7/layout/style/nsStyleStruct.cpp#2608

However, in this bug we're special-casing this and representing effect-level timing functions as a Maybe<> type which is Nothing() in the case where the animation is generated by CSS.

Firstly, representing linear timing functions as cubic beziers is a bit inefficient. ComputedTimingFunction objects contain an nsSMILKeySpline which contains a cache of sample values along the curve that we interpolate between. Firstly, it seems really inefficient to store the same cubic bezier points and the exact same cache of sample values for every single keyframe that using 'ease' for example. We really should have a common ComputedTimingFunction object for each of the cubic bezier keywords. That's a separate bug, however.

For this bug we could probably just say that for *both* keyframe-level timing functions and animation-level timing functions, linear timing functions are represented by an empty/null/Nothing() value. For example, we could rework ParseEasing in KeyframeEffect.cpp to return a Maybe<ComputedTimingFunction> value.
Summary: implement animation-level easing → implement effect-level easing
(In reply to Brian Birtles (:birtles) from comment #13)
> We really should have a common
> ComputedTimingFunction object for each of the cubic bezier keywords. That's
> a separate bug, however.

Filed bug 1231471 for that.
(Assignee)

Comment 15

2 years ago
Thanks, Brian, for the suggestion.

I've almost finished to rewrite patches as your suggestion.
The patch set will be re-ordered and re-constructed except part 6 and 7.
So, Cameron, you can review the part 7 patch for now. Thanks for waiting for reviewing.

Rest of patches will come today if the result of a try which I just pushed now looks good.
(Assignee)

Updated

2 years ago
Attachment #8693823 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693825 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693826 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693827 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8693880 - Flags: review?(cam)
One other thing I realised, if we have *both* effect-level easing and keyframe-level easing, *and* we have a cubic-bezier on the effect-level easing that produces *output* outside [0..1], then we might get an *input* to the keyframe-level easing function that is outside [0..1]. I guess assertions might fail if we have that.
(Assignee)

Updated

2 years ago
See Also: → bug 1232529
(Assignee)

Comment 17

2 years ago
(In reply to Brian Birtles (:birtles) from comment #16)
> One other thing I realised, if we have *both* effect-level easing and
> keyframe-level easing, *and* we have a cubic-bezier on the effect-level
> easing that produces *output* outside [0..1], then we might get an *input*
> to the keyframe-level easing function that is outside [0..1]. I guess
> assertions might fail if we have that.

Thanks, Brian! I did not notice the fact at all.
According the spec [1], in case of the values is outside [0, 1] we need computed keyframe offset to obtain a proper AnimationPropertySegment. I filed bug 1232529 for it.

[1] https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect
Attachment #8693830 - Flags: review?(cam) → review+
(Assignee)

Comment 18

2 years ago
Created attachment 8712509 [details] [diff] [review]
Part 1: Add null_t into TimingFunction to skip calculation of linear timing function

This is a patch for compositor side to represent linear function as null_t/Nothing().

Also the first argument of ToTimingFunction changes to a Maybe<ComputedTimingFunction>.
As a result of this change we can also use ToTimingFunction for animation
effect's timing function.
Attachment #8693823 - Attachment is obsolete: true
Attachment #8712509 - Flags: review?(cam)
(Assignee)

Comment 19

2 years ago
Created attachment 8712510 [details] [diff] [review]
Part 2: Add LayerAnimationUtils.

This is also for compositor side.
we need a new class to share the function which converts TimingFunction
to ComputedTimingFunction for either keyframe's timing function or keyframe
effect's timing function.
Attachment #8693880 - Attachment is obsolete: true
Attachment #8712510 - Flags: review?(cam)
(Assignee)

Comment 20

2 years ago
Created attachment 8712511 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Nothing() represents linear function, i.e. skip calculation.
ParseEasing is changed to return a Maybe<ComputedTimingFunction>,
if timing function is linear function, ParseEasing returns Nothing().
Attachment #8693825 - Attachment is obsolete: true
Attachment #8712511 - Flags: review?(cam)
(Assignee)

Comment 21

2 years ago
Created attachment 8712512 [details] [diff] [review]
Part 4: Move ParseEasing into AnimationUtils

ParseEasing will be also used in AnimationEffectTimingReadOnly class.
Attachment #8693826 - Attachment is obsolete: true
Attachment #8712512 - Flags: review?(cam)
(Assignee)

Comment 22

2 years ago
Created attachment 8712513 [details] [diff] [review]
Part 5: Add easing property in TimingParams.
Attachment #8693827 - Attachment is obsolete: true
Attachment #8712513 - Flags: review?(cam)
(Assignee)

Comment 23

2 years ago
Created attachment 8712514 [details] [diff] [review]
Part 6: Store ComputedTimingFunction in TimingParams

The reason why we use Maybe<> to store the function is that CSS
animations/transitions do not have the function property.
Attachment #8712514 - Flags: review?(cam)
(Assignee)

Comment 24

2 years ago
Created attachment 8712515 [details] [diff] [review]
Part 7: Add easing function to laryer::Animation
Attachment #8712515 - Flags: review?(cam)
(Assignee)

Comment 25

2 years ago
Comment on attachment 8693828 [details] [diff] [review]
Part 8: Calculate transformed progress using animation effect's timing function

This patch became part 8 now.
Attachment #8693828 - Attachment description: Part 6: Calculate transformed progress using animation effect's timing function → Part 8: Calculate transformed progress using animation effect's timing function
(Assignee)

Comment 26

2 years ago
Comment on attachment 8693830 [details] [diff] [review]
Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

This patch became part 9 now.
Attachment #8693830 - Attachment description: Part 7: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly → Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly
(Assignee)

Comment 27

2 years ago
Created attachment 8712517 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Now we produce cumputed timing progress outside [0,1] range.
We use the last segment to calculate animation values if the value is greater than 1.
We use the first segment to calculate animation values if the value is lesser than 0.

This patch includes 4 test cases:
1) progress value < 0 for compositor animation
2) progress value > 1 for compositor animation
3) progress value < 0 for main-thread animation
4) progress value > 1 for main-thread animation

I will ask Brian to review this once he returns to daily work.
Attachment #8712509 - Flags: review?(cam) → review+
Comment on attachment 8712510 [details] [diff] [review]
Part 2: Add LayerAnimationUtils.

Review of attachment 8712510 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +478,5 @@
>      for (uint32_t j = 0; j < segments.Length(); j++) {
>        TimingFunction tf = segments.ElementAt(j).sampleFn();
> +
> +      Maybe<ComputedTimingFunction> ctf;
> +      ctf = AnimationUtils::TimingFunctionToComputedTimingFunction(tf);

Nit: just join this into the one variable declaration/initialization statement.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +617,5 @@
>        (segment->endPortion() - segment->startPortion());
>  
> +    double portion = animData.mFunctions[segmentIndex].isSome() ?
> +      animData.mFunctions[segmentIndex]->GetValue(positionInSegment) :
> +      positionInSegment;

I wonder if it would make sense to have a static helper method on ComputedTimingFunction that takes a |const Maybe<ComputedTimingFunction>&| so that it can do this "isSome() ? ... : ..." work, since Part 3 also needs to do it?

::: gfx/layers/ipc/LayerAnimationUtils.cpp
@@ +15,5 @@
> +/* static */ Maybe<ComputedTimingFunction>
> +AnimationUtils::TimingFunctionToComputedTimingFunction(
> +  const TimingFunction& aTimingFunction)
> +{
> +  ComputedTimingFunction result;

Move this variable into the two switch cases that need it.
Attachment #8712510 - Flags: review?(cam) → review+
Comment on attachment 8712511 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Review of attachment 8712511 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the OrderedKeyframeValueEntry::mTiming change.

::: dom/animation/KeyframeEffect.cpp
@@ +539,5 @@
>        (computedTiming.mProgress.Value() - segment->mFromKey) /
>        (segment->mToKey - segment->mFromKey);
> +    double valuePosition = segment->mTimingFunction.isSome() ?
> +      segment->mTimingFunction->GetValue(positionInSegment) :
> +      positionInSegment;

Use the helper static method I mentioned in the previous part's review, if you decide to add it.

@@ +1706,5 @@
>    nsTArray<OrderedKeyframeValueEntry> entries;
>  
> +  // We need a linear function here to sort key frames correctly.
> +  // mTimingFunction in AnimationPropertySegment is Nothing() in case of
> +  // the timining function is 'linear'. So if the mTimingFunction is

s/timining/timing/

@@ +1710,5 @@
> +  // the timining function is 'linear'. So if the mTimingFunction is
> +  // Nothing(), we need a dummy ComputedTimingFunction to be passed to
> +  // ComputedTimingFunction::Compare.
> +  ComputedTimingFunction linear;
> +  linear.Init(nsTimingFunction(NS_STYLE_TRANSITION_TIMING_FUNCTION_LINEAR));

Since AnimationPropertySegment::mTimingFunction is now a Maybe<>, we could make OrderedKeyframeValueEntry::mTiming function be a Maybe<>*.  Then we wouldn't need to create this dummy linear ComputedTimingFunction -- pointing to the existing Maybe<>(Nothing()) value would be fine.
Attachment #8712511 - Flags: review?(cam) → review+
Comment on attachment 8712512 [details] [diff] [review]
Part 4: Move ParseEasing into AnimationUtils

Review of attachment 8712512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationUtils.cpp
@@ +62,5 @@
> +        // don't support a list of timing functions
> +        break;
> +      }
> +      switch (list->mValue.GetUnit()) {
> +        case eCSSUnit_Enumerated:

Why do we not return Nothing() when "linear" was passed in?

::: dom/animation/AnimationUtils.h
@@ +52,5 @@
>                                         const nsIContent* aContent = nullptr);
> +
> +  /**
> +   * Parses a CSS <single-transition-timing-function> value from
> +   * aEasing into a ComputedTimingFunction.  If parsing fails, 'Nohting' will

s/Nohthing/Nothing/
Comment on attachment 8712513 [details] [diff] [review]
Part 5: Add easing property in TimingParams.

Review of attachment 8712513 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +48,5 @@
>    TimeDuration mDelay;      // Initializes to zero
>    double mIterations = 1.0; // Can be NaN, negative, +/-Infinity
>    dom::PlaybackDirection mDirection = dom::PlaybackDirection::Normal;
>    dom::FillMode mFill = dom::FillMode::Auto;
> +  nsString mEasing;

Is there a reason we store a string rather than a timing function that we serialize when GetEasing is called?
Comment on attachment 8712515 [details] [diff] [review]
Part 7: Add easing function to laryer::Animation

Review of attachment 8712515 [details] [diff] [review]:
-----------------------------------------------------------------

s/laryer/layers/ in the commit message.
Attachment #8712515 - Flags: review?(cam) → review+
ni? for comment 30 and comment 31 questions.
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 34

2 years ago
Thank you, Cameron.

(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #31)
> Comment on attachment 8712513 [details] [diff] [review]
> Part 5: Add easing property in TimingParams.
> 
> Review of attachment 8712513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/AnimationEffectTimingReadOnly.h
> @@ +48,5 @@
> >    TimeDuration mDelay;      // Initializes to zero
> >    double mIterations = 1.0; // Can be NaN, negative, +/-Infinity
> >    dom::PlaybackDirection mDirection = dom::PlaybackDirection::Normal;
> >    dom::FillMode mFill = dom::FillMode::Auto;
> > +  nsString mEasing;
> 
> Is there a reason we store a string rather than a timing function that we
> serialize when GetEasing is called?

My intention here was to return the easing string as it is.  I did not realized that it does not match to KeyframeEffectReadOnly.getFrames()[].easing.  I will fix it.
 
(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #30)
> Comment on attachment 8712512 [details] [diff] [review]
> Part 4: Move ParseEasing into AnimationUtils
> 
> Review of attachment 8712512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/AnimationUtils.cpp
> @@ +62,5 @@
> > +        // don't support a list of timing functions
> > +        break;
> > +      }
> > +      switch (list->mValue.GetUnit()) {
> > +        case eCSSUnit_Enumerated:
> 
> Why do we not return Nothing() when "linear" was passed in?

I did not realize the fact at all!  Thank you, Cameron!  I will change it to return Nothing() if "linear" is specified there.
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 35

2 years ago
Created attachment 8712592 [details] [diff] [review]
Part 4: Move ParseEasing into AnimationUtils v2

Changes from v1:

* Returns Nothing if "linear" is specified
* Adds "const" to the dom::Element argument
Attachment #8712512 - Attachment is obsolete: true
Attachment #8712512 - Flags: review?(cam)
Attachment #8712592 - Flags: review?(cam)
(Assignee)

Comment 36

2 years ago
Created attachment 8712596 [details] [diff] [review]
Part 5: Store ComputedTimingFunction in TimingParams

Now GetEasing returns a serialized string from ComputedTimingFunction.

I did forget to mention these patch series applied upon patches for bug 1096773.  A patch (attachment 8712403 [details] [diff] [review]) in that bug introduces utility methods to create TimingParams, this patch modifies the methods.

'Part 6' is a missing number for now.
Attachment #8712513 - Attachment is obsolete: true
Attachment #8712514 - Attachment is obsolete: true
Attachment #8712513 - Flags: review?(cam)
Attachment #8712514 - Flags: review?(cam)
Attachment #8712596 - Flags: review?(cam)
Attachment #8712592 - Flags: review?(cam) → review+
Comment on attachment 8712596 [details] [diff] [review]
Part 5: Store ComputedTimingFunction in TimingParams

Review of attachment 8712596 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #8712596 - Flags: review?(cam) → review+
(Assignee)

Comment 38

2 years ago
Created attachment 8713032 [details] [diff] [review]
Part 2: Add LayerAnimationUtils v2.

Added a helper method to get value from Maybe<ComputedTimingFunction>.
Attachment #8712510 - Attachment is obsolete: true
Attachment #8713032 - Flags: review+
(Assignee)

Comment 39

2 years ago
Created attachment 8713034 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Cameron, could you please take a look this patch again?
I did forget to fix timingFunction.GetValue() in nsTransitionManager.cpp.

I've decided to left the part of making OrderedKeyframeValueEntry::mTiming as Maybe<>* in a subsequent patch because it needs a bit of tweaking since 'ease' is prior to 'linear' in nsTimingFunction::Type.
Attachment #8712511 - Attachment is obsolete: true
Attachment #8713034 - Flags: review?(cam)
Comment on attachment 8713034 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

Review of attachment 8713034 [details] [diff] [review]:
-----------------------------------------------------------------

I think a better name for the helper function would be GetPortion.  GetValueOr is similar to the Maybe methods, so to me it sounds like it would return the value inside the Maybe or the second argument, but it actually is passing in that second value to the Maybe's timing function.
Attachment #8713034 - Flags: review?(cam) → review+
(Assignee)

Comment 41

2 years ago
Created attachment 8713041 [details] [diff] [review]
Part 6: Make mTimingFunction in OrderedKeyframeValueEntry cosnt Maybe<>*.

This is the part of the making OrderedKeyframeValueEntry::mTiming Maybe<>*. Yay, now this patch fits in part 6.

Actually I am not a big fan of this patch.  If we could change the nsTimingFunction::Type, things gets easier.  But the order of nsTimingFunction::Type matches to the spec[1].  I don't think we can change the order.  I don't know why 'ease' is the top of them though...

[1] https://drafts.csswg.org/css-transitions-1/#single-transition-timing-function
Attachment #8713041 - Flags: review?(cam)
Comment on attachment 8713041 [details] [diff] [review]
Part 6: Make mTimingFunction in OrderedKeyframeValueEntry cosnt Maybe<>*.

Review of attachment 8713041 [details] [diff] [review]:
-----------------------------------------------------------------

s/cosnt/const/ in the commit message.
Attachment #8713041 - Flags: review?(cam) → review+
(Assignee)

Comment 43

2 years ago
Created attachment 8713847 [details] [diff] [review]
Part 2: Add LayerAnimationUtils v3

GetValueOr -> GetPortion.
Attachment #8713032 - Attachment is obsolete: true
Attachment #8713847 - Flags: review+
(Assignee)

Comment 44

2 years ago
Created attachment 8713848 [details] [diff] [review]
Part 3: Change ComputedTimingFunction* to Maybe<ComputedTimingFunction>

GetValueOr -> GetPortion
Attachment #8713034 - Attachment is obsolete: true
Attachment #8713848 - Flags: review+
(Assignee)

Comment 45

2 years ago
Created attachment 8713849 [details] [diff] [review]
Part 5: Store ComputedTimingFunction in TimingParams

Rebased.
Attachment #8712596 - Attachment is obsolete: true
Attachment #8713849 - Flags: review+
(Assignee)

Comment 46

2 years ago
Created attachment 8713853 [details] [diff] [review]
Part 6: Make mTimingFunction in OrderedKeyframeValueEntry cosnt Maybe<>*.

Rebased.
Attachment #8713041 - Attachment is obsolete: true
Attachment #8713853 - Flags: review+
(Assignee)

Comment 47

2 years ago
Created attachment 8713856 [details] [diff] [review]
Part 8: Calculate transformed progress using animation effect's timing function

Rebased.
Attachment #8693828 - Attachment is obsolete: true
Attachment #8713856 - Flags: review+
(Assignee)

Comment 48

2 years ago
Created attachment 8713858 [details] [diff] [review]
Part 9: Tests that easing property in KeyframeEffectOptions is passed to KeyframeEffectReadOnly

Change the part number in the commit message.
Attachment #8693830 - Attachment is obsolete: true
Attachment #8713858 - Flags: review+
(Assignee)

Comment 49

2 years ago
Sheriff, could you please check part 1 to 9 patches in?  It's harmless without part 10.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3915509961
Keywords: checkin-needed, leave-open
(Assignee)

Comment 52

2 years ago
Comment on attachment 8712517 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

I am going to fix a typo 'cumputed' in commit message once this patch gets reviewed.
Attachment #8712517 - Flags: review?(bbirtles)
Comment on attachment 8712517 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Review of attachment 8712517 [details] [diff] [review]:
-----------------------------------------------------------------

Two issues:

1. I don't think this works when we have a step timing function. I think when we get a progress < 0 or > 1 we'll end up with positionInSegment also < 0 or > 1 and pass that to ComputedTimingFunction::GetValue. Then, if we have a step timing function we'll pass that on to StepEnd which has an assertion for aPortion being between 0 and 1. So that assertion will fail.

2. When we have a cubic bezier curve as a timing function, we'll pass the progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that that will return a sensible value when the progress is < 0 or > 1. In fact, I think the spec was supposed to say that in that case we should treat the timing function as 'linear' outside the [0, 1] range but I can't see where that's defined. I'll talk to the Google guys and see what they did here.

We also need to update the documentation for StyleAnimatinValue::Interpolate to say that aPortion can be outside the range [0.0, 1.0].

I'm reluctant to review this without tests that we actually do something sensible when the progress value is outside the [0.0, 1.0] range. As discussed, I'd like to add mochitests to this bug (i.e. fold in bug 1228915). Do you mind re-requesting review after we have those tests?

The rest looks good.
Attachment #8712517 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> 2. When we have a cubic bezier curve as a timing function, we'll pass the
> progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> that will return a sensible value when the progress is < 0 or > 1. In fact,
> I think the spec was supposed to say that in that case we should treat the
> timing function as 'linear' outside the [0, 1] range but I can't see where
> that's defined. I'll talk to the Google guys and see what they did here.

I think what the spec says is that when the interval progress is < 0 or > 1 you basically end up just using the first or last keyframe's value.

 http://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect

The spec doesn't seem to actually apply the keyframe-level timing function however.
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> The spec doesn't seem to actually apply the keyframe-level timing function
> however.

Filed https://github.com/w3c/web-animations/issues/139 for that.
Blocks: 1245000
(Assignee)

Comment 56

2 years ago
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> > 2. When we have a cubic bezier curve as a timing function, we'll pass the
> > progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> > that will return a sensible value when the progress is < 0 or > 1. In fact,
> > I think the spec was supposed to say that in that case we should treat the
> > timing function as 'linear' outside the [0, 1] range but I can't see where
> > that's defined. I'll talk to the Google guys and see what they did here.
> 
> I think what the spec says is that when the interval progress is < 0 or > 1
> you basically end up just using the first or last keyframe's value.

Thank you, Brian. Let me clarify it.
We do not need any extrapolation whatever easing function is used if positionInSegment is outside [0, 1] range?
That broke b2g builds:
In file included from Unified_cpp_gfx_layers6.cpp:2:0:
/home/fabrice/dev/mozilla-inbound/gfx/layers/ipc/LayerAnimationUtils.cpp:17:40: error: no 'mozilla::Maybe<mozilla::ComputedTimingFunction> mozilla::layers::AnimationUtils::TimingFunctionToComputedTimingFunction(const mozilla::layers::TimingFunction&)' member function declared in class 'mozilla::layers::AnimationUtils'
Depends on: 1245016
(In reply to Hiroyuki Ikezoe (:hiro) from comment #56)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> > > 2. When we have a cubic bezier curve as a timing function, we'll pass the
> > > progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> > > that will return a sensible value when the progress is < 0 or > 1. In fact,
> > > I think the spec was supposed to say that in that case we should treat the
> > > timing function as 'linear' outside the [0, 1] range but I can't see where
> > > that's defined. I'll talk to the Google guys and see what they did here.
> > 
> > I think what the spec says is that when the interval progress is < 0 or > 1
> > you basically end up just using the first or last keyframe's value.
> 
> Thank you, Brian. Let me clarify it.
> We do not need any extrapolation whatever easing function is used if
> positionInSegment is outside [0, 1] range?

Right, I believe that's what it says. Are you able to test what Chrome does? (Or check the chromium source?)
Depends on: 1245058
(Assignee)

Comment 59

2 years ago
Created attachment 8715148 [details]
An example for easing which generates outside [0,1] values

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #58)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #56)
> > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #54)
> > > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #53)
> > > > 2. When we have a cubic bezier curve as a timing function, we'll pass the
> > > > progress onto nsSMILKeySpline::GetSplineValue. It's not clear to me that
> > > > that will return a sensible value when the progress is < 0 or > 1. In fact,
> > > > I think the spec was supposed to say that in that case we should treat the
> > > > timing function as 'linear' outside the [0, 1] range but I can't see where
> > > > that's defined. I'll talk to the Google guys and see what they did here.
> > > 
> > > I think what the spec says is that when the interval progress is < 0 or > 1
> > > you basically end up just using the first or last keyframe's value.
> > 
> > Thank you, Brian. Let me clarify it.
> > We do not need any extrapolation whatever easing function is used if
> > positionInSegment is outside [0, 1] range?
> 
> Right, I believe that's what it says. Are you able to test what Chrome does?
> (Or check the chromium source?)

Chromium 50.0.2639.0 does calculate extrapolation.  I did use attaching HTML file.
Created attachment 8716126 [details]
Additional overflow test

I expanded attachment 8715148 [details] to further debug what Chrome does.
For cubic beziers, Chromium uses the tangent at the extremity:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/animation/TimingFunction.cpp&l=101

For step timing functions, Chromium clamps to [0, 1] so that the steps basically extend out:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/animation/TimingFunction.cpp&l=303

That's not what the spec says so I will take it up with Google on Monday next week.
(Assignee)

Updated

2 years ago
Depends on: 1246893
I spoke to Google about this and it seems like we do want to extrapolate behavior. Otherwise, even if you only have "linear" as the easing on your keyframe segments, if the effect-level easing goes outside the range [0, 1] then those segments won't animate beyond their bounds as expected.

I'll update the spec sometime next week I hope, but basically we want to do what Chrome does here:

* Extrapolate linear interpolation in a linear fashion,
* Extrapolate bezier functions using the tangent at the endpoint
* Extrapolate step timing functions by just projecting the 0,1 value infinitely.
(Assignee)

Comment 63

2 years ago
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #62)
> I spoke to Google about this and it seems like we do want to extrapolate
> behavior. Otherwise, even if you only have "linear" as the easing on your
> keyframe segments, if the effect-level easing goes outside the range [0, 1]
> then those segments won't animate beyond their bounds as expected.
> 
> I'll update the spec sometime next week I hope, but basically we want to do
> what Chrome does here:
> 
> * Extrapolate linear interpolation in a linear fashion,
> * Extrapolate bezier functions using the tangent at the endpoint
> * Extrapolate step timing functions by just projecting the 0,1 value
> infinitely.

Thank you, Brian.
A test case I am concerned about Chrome implementation is the edge value for bezier functions.
It's (3) case in chromium code:
https://src.chromium.org/viewvc/blink/trunk/Source/platform/animation/UnitBezier.h?r1=172821&r2=172820&pathrev=172821

Here are two typical animations:
  var anim = target.animate(
    [ { width: "0px", easing: "cubic-bezier(1,1.5,1,1.5)" },
      { width: "100px" } ],
    { duration: 1000, easing: "cubic-bezier(0,1.5,1,1.5)" });

  var anim = target.animate(
    [ { width: "0px", easing: "cubic-bezier(1,1.5,0.99,1.5)" },
      { width: "100px" } ],
    { duration: 1000, easing: "cubic-bezier(0,1.5,1,1.5)" });

These animations have very similar bezier functions but its behaviors on chrome are totally different because the tangent of the first animation is minus infinity, so they use 0 as the tangent in this case.  On the other hand, the tangent of the second animation is a big minus value....
I have no good idea to fill this gap between these two similar animations though...
(Assignee)

Comment 64

2 years ago
Created attachment 8717860 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Extrapolations are handled in subsequent patches.
Attachment #8712517 - Attachment is obsolete: true
Attachment #8717860 - Flags: review?(bbirtles)
(Assignee)

Comment 65

2 years ago
Created attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

If both of control points of cubic bezier functions are on y = x line,
we can consider the function as a linear function, then skip calculations.

With this patch it will be easier to handle extrapolation of cubic-bezier(1,1,1,1) or cubic-bezier(0,0,0,0).
Attachment #8717866 - Flags: review?(bbirtles)
(Assignee)

Comment 66

2 years ago
Created attachment 8717868 [details] [diff] [review]
Part 11:  Clamp values of step functions outside [0, 1].
Attachment #8717868 - Flags: review?(bbirtles)
(Assignee)

Comment 67

2 years ago
Created attachment 8717870 [details] [diff] [review]
Part 13: Extrapolate bezier function outside [0,1]

We use the tangent at the each boundary points as same as Chrome does.
Attachment #8717870 - Flags: review?(bbirtles)
(Assignee)

Comment 68

2 years ago
Created attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.
Attachment #8717871 - Flags: review?(bbirtles)
(Assignee)

Comment 69

2 years ago
Comment on attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

Review of attachment 8717866 [details] [diff] [review]:
-----------------------------------------------------------------

This should be "Part 11" actually.
Comment on attachment 8717860 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Review of attachment 8717860 [details] [diff] [review]:
-----------------------------------------------------------------

This is brilliant work.

r=birtles. I think we should probably change the main thread tests to seek to the extrapolated range. The compositor tests are probably fine as-is unless you think it makes sense to use a non-integral iteration count to make sure we fill in the extrapolated range (that would also let us make the test duration shorter).

::: dom/animation/test/crashtests/1216842-1.html
@@ +20,5 @@
> +        { opacity: [0, 1] },
> +        {
> +          fill: "forwards",
> +          easing: "cubic-bezier(0,-0.5,1,-0.5)",
> +          duration: 100

I guess we're just hoping that we'll get a tick that lies outside the [0,1] range. I was thinking that if we use 'iterations: 0.75' or something like that so that the animation finishes in the extrapolated range we might have a higher chance of getting a tick there (since we normally get one tick where the compositor animation fills forwards while waiting for the main thread to remove it). What do you think?

Otherwise I think this is fine.

::: dom/animation/test/crashtests/1216842-3.html
@@ +14,5 @@
> +        { color: ["red", "blue"] },
> +        {
> +          fill: "forwards",
> +          easing: "cubic-bezier(0,1.5,1,1.5)",
> +          duration: 100

For the main thread tests, would it be better to pause the animation and seek it to the extrapolated range?

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +609,3 @@
>  
> +    uint32_t segmentIndex = 0;
> +    size_t segmentSize = animation.segments().Length();

s/segmentSize/segmentLength/ ?
Attachment #8717860 - Flags: review?(bbirtles) → review+
Comment on attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

Review of attachment 8717866 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +16,5 @@
>    mType = aFunction.mType;
>    if (nsTimingFunction::IsSplineType(mType)) {
> +    mIsPseudoLinear =
> +      aFunction.mFunc.mX1 == aFunction.mFunc.mY1 &&
> +      aFunction.mFunc.mX2 == aFunction.mFunc.mY2;

I thought nsSMILKeySpline already did this?

https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/dom/smil/nsSMILKeySpline.cpp#37

Why do we need this?
Attachment #8717868 - Flags: review?(bbirtles) → review+
Comment on attachment 8717866 [details] [diff] [review]
Part 11: Part 10: Do not calculate any value if the function can be considered as linear functions.

Review of attachment 8717866 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #71)
> Comment on attachment 8717866 [details] [diff] [review]
> Part 11: Part 10: Do not calculate any value if the function can be
> considered as linear functions.
> 
> Review of attachment 8717866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/ComputedTimingFunction.cpp
> @@ +16,5 @@
> >    mType = aFunction.mType;
> >    if (nsTimingFunction::IsSplineType(mType)) {
> > +    mIsPseudoLinear =
> > +      aFunction.mFunc.mX1 == aFunction.mFunc.mY1 &&
> > +      aFunction.mFunc.mX2 == aFunction.mFunc.mY2;
> 
> I thought nsSMILKeySpline already did this?
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> ac39fba33c6daf95b2cda71e588ca18e2eb752ab/dom/smil/nsSMILKeySpline.cpp#37
> 
> Why do we need this?

Oh, I just read the comment now. This is for the extrapolation case. We should at least add a comment saying that.

Alternatively, I wonder if we need to store mIsPseudoLinear as a member variable. Can we just check it every time in GetValue?

e.g.

if (<portion it out of range>) {
  if (HasSpline() && mTimingFunction.X1() == mTimingFunction.Y1() &&
                     mTimingFunction.X2() == mTimingFunction.Y2()) {
     return portion;
  }
  // Other extrapolation behavior from part 13 etc.
}

I think this calculation is cheap enough and rare enough that we could do it each time?

What do you think?
Comment on attachment 8717870 [details] [diff] [review]
Part 13: Extrapolate bezier function outside [0,1]

Review of attachment 8717870 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.h
@@ +60,5 @@
>    uint32_t mSteps;
>    nsTimingFunction::StepSyntax mStepSyntax;
>    bool mIsPseudoLinear = false;
> +  double mTangentForLessThanZero;
> +  double mTangentForGreaterThanOne;

As with part 11, I wonder if we need to store this as member variables or whether we could just perform the calculation as-necessary in GetValue. What do you think?
Comment on attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.

This testing is brilliant but I'm going to come back to it later so I can check it properly.
(Assignee)

Comment 75

2 years ago
Comment on attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.

Review of attachment 8717871 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/web-animations/keyframe-effect/easing.html
@@ +52,5 @@
> +      // return the tangent at 0.
> +    }
> +    if (x > 1) {
> +      // return the tangent at 1.
> +    }

I just noticed now these lines should be removed.
(Assignee)

Comment 76

2 years ago
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #72)
> Comment on attachment 8717866 [details] [diff] [review]
> Part 11: Part 10: Do not calculate any value if the function can be
> considered as linear functions.
> 
> Review of attachment 8717866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #71)
> > Comment on attachment 8717866 [details] [diff] [review]
> > Part 11: Part 10: Do not calculate any value if the function can be
> > considered as linear functions.
> > 
> > Review of attachment 8717866 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/animation/ComputedTimingFunction.cpp
> > @@ +16,5 @@
> > >    mType = aFunction.mType;
> > >    if (nsTimingFunction::IsSplineType(mType)) {
> > > +    mIsPseudoLinear =
> > > +      aFunction.mFunc.mX1 == aFunction.mFunc.mY1 &&
> > > +      aFunction.mFunc.mX2 == aFunction.mFunc.mY2;
> > 
> > I thought nsSMILKeySpline already did this?
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > ac39fba33c6daf95b2cda71e588ca18e2eb752ab/dom/smil/nsSMILKeySpline.cpp#37
> > 
> > Why do we need this?
> 
> Oh, I just read the comment now. This is for the extrapolation case. We
> should at least add a comment saying that.
> 
> Alternatively, I wonder if we need to store mIsPseudoLinear as a member
> variable. Can we just check it every time in GetValue?
> 
> e.g.
> 
> if (<portion it out of range>) {
>   if (HasSpline() && mTimingFunction.X1() == mTimingFunction.Y1() &&
>                      mTimingFunction.X2() == mTimingFunction.Y2()) {
>      return portion;
>   }
>   // Other extrapolation behavior from part 13 etc.
> }
> 
> I think this calculation is cheap enough and rare enough that we could do it
> each time?
> 
> What do you think?

In the pseudo linear case I wanted to avoid calling GetValue at all, but your suggestion looks much better! Thanks! I will do it.
(Assignee)

Comment 77

2 years ago
(In reply to Brian Birtles (:birtles, away 11-12 Feb) from comment #73)
> Comment on attachment 8717870 [details] [diff] [review]
> Part 13: Extrapolate bezier function outside [0,1]
> 
> Review of attachment 8717870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/ComputedTimingFunction.h
> @@ +60,5 @@
> >    uint32_t mSteps;
> >    nsTimingFunction::StepSyntax mStepSyntax;
> >    bool mIsPseudoLinear = false;
> > +  double mTangentForLessThanZero;
> > +  double mTangentForGreaterThanOne;
> 
> As with part 11, I wonder if we need to store this as member variables or
> whether we could just perform the calculation as-necessary in GetValue. What
> do you think?

Actually I did it first, then adjusted to Chrome style. :-)  I will go back the first style.
(Assignee)

Comment 78

2 years ago
Created attachment 8718134 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1]

Thank you, Brian.  Now this patch gets easier to understand I think.

The part of part 11 is absorbed into this patch.  Previous part 12 becomes part 11, and this patch becomes part 12 now.
Attachment #8717866 - Attachment is obsolete: true
Attachment #8717870 - Attachment is obsolete: true
Attachment #8717866 - Flags: review?(bbirtles)
Attachment #8717870 - Flags: review?(bbirtles)
Attachment #8718134 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1228915
Comment on attachment 8718134 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1]

Review of attachment 8718134 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +33,5 @@
>  
>  double
>  ComputedTimingFunction::GetValue(double aPortion) const
>  {
> +  if (aPortion < 0.0 || aPortion > 1.0) {

// Handle the case where we need to extrapolate the function

@@ +36,5 @@
>  {
> +  if (aPortion < 0.0 || aPortion > 1.0) {
> +    if (HasSpline()) {
> +      // If all the control points are on y = x line,
> +      // it's a linear.

// Check for a linear curve.

@@ +47,5 @@
> +      // (p2 - p0).
> +      if (aPortion < 0.0) {
> +        if (mTimingFunction.X1() > 0.0) {
> +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> +        } else if (mTimingFunction.X2() > 0.0) {

What is the logic here? This doesn't seem to match what Chromium does. This seems like it should correspond to case-2 in Chromium's code[1] which is about when the first control point is coincident (not just horizontally coincident) with the end point. i.e. it seems like we need a check that mTimingFunction.Y1() == 0 here?

[1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463

@@ +61,5 @@
> +      if (aPortion > 1.0) {
> +        if (mTimingFunction.X2() < 1.0) {
> +          return 1.0 + (aPortion - 1.0) *
> +            (mTimingFunction.Y2() - 1) / (mTimingFunction.X2() - 1);
> +        } else if (mTimingFunction.X1() < 1.0) {

Similarly here.

@@ +78,1 @@
>    if (HasSpline()) {

It seems a little bit unusual to me to have a big block at the start for the (unusual) case of extrapolation. What do you think about something like the following:

if (HasSpline()) {
  // Check for a linear curve.
  // (GetSplineValue(), below, also checks this but doesn't work when aPortion
  // is outside the range [0.0, 1.0])
  if (mTimingFunction.X1() == mTimingFunction.Y1() &&
      mTimingFunction.X2() == mTimingFunction.Y2()) {
    return aPortion;
  }

  // For negative values, try to extrapolate with tangent (p1 - p0) or,
  // if p1 is coincident with p0, with (p2 - p0).
  if (aPortion < 0.0) {
    ...
  }

  // For values greater than 1, try to extrapolate with tangent
  // (p2 - p3) or, if p2 is coincident with p3, with (p1 - p3).
  if (aPortion > 1.0) {
    ...
  }

  return mTimingFunction.GetSplineValue(aPortion);
}

aPortion = clamped(aPortion, 0.0, 1.0);
...
Attachment #8718134 - Flags: review?(bbirtles)
(Assignee)

Comment 81

2 years ago
(In reply to Brian Birtles (:birtles) from comment #80)
> @@ +47,5 @@
> > +      // (p2 - p0).
> > +      if (aPortion < 0.0) {
> > +        if (mTimingFunction.X1() > 0.0) {
> > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > +        } else if (mTimingFunction.X2() > 0.0) {
> 
> What is the logic here? This doesn't seem to match what Chromium does. This
> seems like it should correspond to case-2 in Chromium's code[1] which is
> about when the first control point is coincident (not just horizontally
> coincident) with the end point. i.e. it seems like we need a check that
> mTimingFunction.Y1() == 0 here?
> 
> [1]
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463

As I sent a e-mail privately to google guys and Brian, I think the check is a bug,
it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 == 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And (X1==0) is not useful there.


> @@ +78,1 @@
> >    if (HasSpline()) {
> 
> It seems a little bit unusual to me to have a big block at the start for the
> (unusual) case of extrapolation. What do you think about something like the
> following:
> 
> if (HasSpline()) {
>   // Check for a linear curve.
>   // (GetSplineValue(), below, also checks this but doesn't work when
> aPortion
>   // is outside the range [0.0, 1.0])
>   if (mTimingFunction.X1() == mTimingFunction.Y1() &&
>       mTimingFunction.X2() == mTimingFunction.Y2()) {
>     return aPortion;
>   }
> 
>   // For negative values, try to extrapolate with tangent (p1 - p0) or,
>   // if p1 is coincident with p0, with (p2 - p0).
>   if (aPortion < 0.0) {
>     ...
>   }
> 
>   // For values greater than 1, try to extrapolate with tangent
>   // (p2 - p3) or, if p2 is coincident with p3, with (p1 - p3).
>   if (aPortion > 1.0) {
>     ...
>   }
> 
>   return mTimingFunction.GetSplineValue(aPortion);
> }
> 
> aPortion = clamped(aPortion, 0.0, 1.0);
> ...

Thanks. I will do it.
(Assignee)

Comment 82

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #81)
> (In reply to Brian Birtles (:birtles) from comment #80)
> > @@ +47,5 @@
> > > +      // (p2 - p0).
> > > +      if (aPortion < 0.0) {
> > > +        if (mTimingFunction.X1() > 0.0) {
> > > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > > +        } else if (mTimingFunction.X2() > 0.0) {
> > 
> > What is the logic here? This doesn't seem to match what Chromium does. This
> > seems like it should correspond to case-2 in Chromium's code[1] which is
> > about when the first control point is coincident (not just horizontally
> > coincident) with the end point. i.e. it seems like we need a check that
> > mTimingFunction.Y1() == 0 here?
> > 
> > [1]
> > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> > WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463
> 
> As I sent a e-mail privately to google guys and Brian, I think the check is
> a bug,
> it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 ==
> 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And
> (X1==0) is not useful there.

Oh my. This comment is totally wrong.  I will post an example of Y1 != 0 later.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> later.

Ok, thanks. The Chromium code seems to match the comment next to the code at least. If p1x is always >= 0, then the first branch is for when p1x > 0 and the second branch is for when p1x == 0 && p1y == 0 && p2x > 0.
(Assignee)

Comment 84

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #81)
> > (In reply to Brian Birtles (:birtles) from comment #80)
> > > @@ +47,5 @@
> > > > +      // (p2 - p0).
> > > > +      if (aPortion < 0.0) {
> > > > +        if (mTimingFunction.X1() > 0.0) {
> > > > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > > > +        } else if (mTimingFunction.X2() > 0.0) {
> > > 
> > > What is the logic here? This doesn't seem to match what Chromium does. This
> > > seems like it should correspond to case-2 in Chromium's code[1] which is
> > > about when the first control point is coincident (not just horizontally
> > > coincident) with the end point. i.e. it seems like we need a check that
> > > mTimingFunction.Y1() == 0 here?
> > > 
> > > [1]
> > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> > > WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463
> > 
> > As I sent a e-mail privately to google guys and Brian, I think the check is
> > a bug,
> > it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 ==
> > 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And
> > (X1==0) is not useful there.
> 
> Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> later.

Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>

(In reply to Brian Birtles (:birtles) from comment #83)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> > Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> > later.
> 
> Ok, thanks. The Chromium code seems to match the comment next to the code at
> least. If p1x is always >= 0, then the first branch is for when p1x > 0 and
> the second branch is for when p1x == 0 && p1y == 0 && p2x > 0.

Yes, the 'p1y == 0'  is a problematic code there.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #84)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #82)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #81)
> > > (In reply to Brian Birtles (:birtles) from comment #80)
> > > > @@ +47,5 @@
> > > > > +      // (p2 - p0).
> > > > > +      if (aPortion < 0.0) {
> > > > > +        if (mTimingFunction.X1() > 0.0) {
> > > > > +          return aPortion * mTimingFunction.Y1() / mTimingFunction.X1();
> > > > > +        } else if (mTimingFunction.X2() > 0.0) {
> > > > 
> > > > What is the logic here? This doesn't seem to match what Chromium does. This
> > > > seems like it should correspond to case-2 in Chromium's code[1] which is
> > > > about when the first control point is coincident (not just horizontally
> > > > coincident) with the end point. i.e. it seems like we need a check that
> > > > mTimingFunction.Y1() == 0 here?
> > > > 
> > > > [1]
> > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> > > > WebKit/Source/platform/animation/UnitBezier.h&l=50&rcl=1455510463
> > > 
> > > As I sent a e-mail privately to google guys and Brian, I think the check is
> > > a bug,
> > > it should actually X1 == 0 there.  Otherwise, we return 0 in case of Y1 ==
> > > 0, e.g. cubic-bezier(0,0,0.4,0.7) <http://cubic-bezier.com/#0,0,.4,.7>, And
> > > (X1==0) is not useful there.
> > 
> > Oh my. This comment is totally wrong.  I will post an example of Y1 != 0
> > later.
> 
> Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>

What is the tangent in this case?
(Assignee)

Comment 86

2 years ago
(In reply to Brian Birtles (:birtles) from comment #85)

> > Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> > cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>
> 
> What is the tangent in this case?

0.39 / 0.77 for negative portion.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #86)
> (In reply to Brian Birtles (:birtles) from comment #85)
> 
> > > Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> > > cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>
> > 
> > What is the tangent in this case?
> 
> 0.39 / 0.77 for negative portion.

Why is that the tangent? If you change the value of Y1, the shape of the curve changes so I would expect the tangent to change.
(Assignee)

Comment 88

2 years ago
(In reply to Brian Birtles (:birtles) from comment #87)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #86)
> > (In reply to Brian Birtles (:birtles) from comment #85)
> > 
> > > > Here is an example: X1 == 0 and Y1 != 0, but we can calculate the tangent.
> > > > cubic-bezier(0,.64,.39,.77) <http://cubic-bezier.com/#0,.64,.39,.77>
> > > 
> > > What is the tangent in this case?
> > 
> > 0.39 / 0.77 for negative portion.
> 
> Why is that the tangent? If you change the value of Y1, the shape of the
> curve changes so I would expect the tangent to change.

I meant the tangent is just an angle from P2 to P0 here.  I am not sure |tangent| is a good word.
(Assignee)

Comment 89

2 years ago
This is a figure explaining the tangents (inclinations) in most cases.
http://hiikezoe.github.io/Extrapolation_normal.png

Blue arrow is inclination to be used for extrapolation for negative portions.
Red arrow is inclination to be used for values greater than 1.

This is an edge case we don't calculate the inclinations on both boundaries because it's infinity (or minus infinity).  Inclination against another control points is used.  Should we also use 0 in this case instead?
http://hiikezoe.github.io/Extrapolation_substitute.png
(In reply to Hiroyuki Ikezoe (:hiro) from comment #89)
> This is a figure explaining the tangents (inclinations) in most cases.
> http://hiikezoe.github.io/Extrapolation_normal.png
> 
> Blue arrow is inclination to be used for extrapolation for negative portions.
> Red arrow is inclination to be used for values greater than 1.
> 
> This is an edge case we don't calculate the inclinations on both boundaries
> because it's infinity (or minus infinity).  Inclination against another
> control points is used.  Should we also use 0 in this case instead?
> http://hiikezoe.github.io/Extrapolation_substitute.png

I don't really know the mathematically correct thing to do here so I'm inclined to just match Chrome. i.e. I think it makes sense to use 0 and 1 for those cases where we can't use the inclination to the nearest control point (except for the special case where the near control point is coincident with the endpoint since then the near control point has no effect).

What do you think?
(Assignee)

Comment 91

2 years ago
(In reply to Brian Birtles (:birtles) from comment #90)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #89)
> > This is a figure explaining the tangents (inclinations) in most cases.
> > http://hiikezoe.github.io/Extrapolation_normal.png
> > 
> > Blue arrow is inclination to be used for extrapolation for negative portions.
> > Red arrow is inclination to be used for values greater than 1.
> > 
> > This is an edge case we don't calculate the inclinations on both boundaries
> > because it's infinity (or minus infinity).  Inclination against another
> > control points is used.  Should we also use 0 in this case instead?
> > http://hiikezoe.github.io/Extrapolation_substitute.png
> 
> I don't really know the mathematically correct thing to do here so I'm
> inclined to just match Chrome. i.e. I think it makes sense to use 0 and 1
> for those cases where we can't use the inclination to the nearest control
> point (except for the special case where the near control point is
> coincident with the endpoint since then the near control point has no
> effect).
> 
> What do you think?

OK, it makes sense.  I will post a revised patch and test.
Comment on attachment 8717871 [details] [diff] [review]
Part 14: Tests for effect-level easing.

Review of attachment 8717871 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I'm not sure about the behavior of the step-start timing functions when the time is zero.

::: testing/web-platform/tests/web-animations/keyframe-effect/easing.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Effect level easing tests</title>

Nit: Effect-level

@@ +12,5 @@
> +<div id="target"></div>
> +<script>
> +"use strict";
> +
> +function px_to_num(str) {

Can we move px_to_num, cubicBezier, step_end etc. to ../testcommon.js?

Also, do we normally use _ to separate words in Javascript?

(assert_style_left_at, at least, is fine since it matches testharness.js's asssertions)

@@ +77,5 @@
> +  var portion = time / animation.effect.timing.duration;
> +  assert_approx_equals(px_to_num(getComputedStyle(animation.effect.target).left),
> +                       easingFunction(portion) * 100,
> +                       0.01,
> +                       'The width of the animation should be approximately ' +

width of the animation? I thought we were testing the left offset of the animation target?

@@ +83,5 @@
> +}
> +
> +var gEffectEasingTests = [
> +  {
> +    desc: 'steps-start function',

Call this 'steps(start)'? 'steps-start' looks like a misspelling of 'step-start'? (Likewise elsewhere)

@@ +129,5 @@
> +    easingFunction: cubicBezier(0, -0.5, 1, -0.5)
> +  },
> +];
> +
> +gEffectEasingTests.forEach(function(option) {

s/option/options/

@@ +136,5 @@
> +    target.style.position = 'absolute';
> +    var anim = target.animate([ { left: '0px' }, { left: '100px' } ],
> +                              { duration: 1000,
> +                                fill: 'forwards',
> +                                easing: option.easingOption });

'easingOption' is a bit strange. 'easingString', 'easingSpec', or just 'easing' would be better I think.

@@ +154,5 @@
> +  {
> +    desc: 'effect easing produces values greater than 1 with keyframe ' +
> +          'easing cubic-bezier(0, 0, 0, 0)',
> +    easingOption: 'cubic-bezier(0, 1.5, 1, 1.5)',
> +    keyframeEasingOption: 'cubic-bezier(0, 0, 0, 0)', // linear

Likewise, just 'keyframeEasing' would be fine I think.

@@ +215,5 @@
> +  anim.pause();
> +
> +  /* The bezier function produces values greater than 1 in (0.23368794, 1) */
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(target).left, '0px');

This should be 100px, right? Maybe our implementation of step timing functions is wrong?

@@ +259,5 @@
> +  /* The bezier function produces negative values in (0, 0.766312060) */
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(target).left, '0px');
> +  anim.currentTime = 750;
> +  assert_equals(getComputedStyle(target).left, '0px');

For this I would expect:

currentTime = 0; left => 100px
currentTime = 250; left => 0px
currentTime = 750; left => 0px
currentTime = 800; left => 100px

?

@@ +299,5 @@
> +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +  var easing = function(x) {
> +    if (x > 1.0) {
> +      /* p3x + (p1y - p3y) / (p1x - p3x) * (x - p3x) */
> +      return 1.0 + (1.5 - 1) / (0 - 1) * (x - 1.0);

I guess this depends on what we agree upon for the extrapolation case. I was expecting the result would just be 1.0 in this case.

@@ +364,5 @@
> +  var target = createDiv(t);
> +  target.style.position = 'absolute';
> +  var anim = target.animate(
> +    // http://cubic-bezier.com/#1,1.5,1,1.5
> +    [ { left: '0px', easing: 'cubic-bezier(1, 1.5, 1, 1.5)' },

Shouldn't we test some cases where we have both keyframe-level and effect-level easing and where the control points are *not* horizontally coincident with the end points?
Attachment #8717871 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8717868 - Attachment description: Part 12: Clamp values of step functions outside [0, 1]. → Part 11: Clamp values of step functions outside [0, 1].
(Assignee)

Comment 93

2 years ago
Created attachment 8719675 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1] v2
Attachment #8718134 - Attachment is obsolete: true
Attachment #8719675 - Flags: review?(bbirtles)
(Assignee)

Comment 94

2 years ago
(In reply to Brian Birtles (:birtles) from comment #92)

> @@ +215,5 @@
> > +  anim.pause();
> > +
> > +  /* The bezier function produces values greater than 1 in (0.23368794, 1) */
> > +  anim.currentTime = 0;
> > +  assert_equals(getComputedStyle(target).left, '0px');
> 
> This should be 100px, right? Maybe our implementation of step timing
> functions is wrong?

Unfortunately we seem to encounter a similar pitfall as bug 1246893.
Comment on attachment 8719675 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1] v2

Review of attachment 8719675 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +51,5 @@
> +      } else if (mTimingFunction.Y1() == 0 && mTimingFunction.X2() > 0.0) {
> +        return aPortion * mTimingFunction.Y2() / mTimingFunction.X2();
> +      }
> +      // We don't extrapolate at all in case of we can not calculate
> +      // the tangent.

// If we can't calculate a sensible tangent, don't extrapolate at all.

@@ +66,5 @@
> +        return 1.0 + (aPortion - 1.0) *
> +          (mTimingFunction.Y1() - 1) / (mTimingFunction.X1() - 1);
> +      }
> +      // We don't extrapolate at all in case of we can not calculate
> +      // the tangent.

// If we can't calculate a sensible tangent, don't extrapolate at all.
Attachment #8719675 - Flags: review?(bbirtles) → review+
(Assignee)

Updated

2 years ago
Depends on: 1248532
(Assignee)

Comment 96

2 years ago
Brian, I am going to put the tests related to steps(start) into dom/animation/mozilla until bug 1248532 is fixed.  Is it OK with you?
(Assignee)

Comment 97

2 years ago
Removing bug 1248532 dependency.  I have not read web animations spec carefully.  steps(start) is exactly well-defined in the spec.
No longer depends on: 1245058
(Assignee)

Updated

2 years ago
Depends on: 1245058
No longer depends on: 1248532
(Assignee)

Comment 98

2 years ago
Created attachment 8720044 [details] [diff] [review]
Part 13: Tests for effect-level easing

The tests for steps(start) in this patch rely on the patch I attached in bug 1248532.
I am holding a review request until that bug is closed.

(In reply to Brian Birtles (:birtles) from comment #92)

> @@ +259,5 @@
> > +  /* The bezier function produces negative values in (0, 0.766312060) */
> > +  anim.currentTime = 0;
> > +  assert_equals(getComputedStyle(target).left, '0px');
> > +  anim.currentTime = 750;
> > +  assert_equals(getComputedStyle(target).left, '0px');
> 
> For this I would expect:
> 
> currentTime = 0; left => 100px
> currentTime = 250; left => 0px
> currentTime = 750; left => 0px
> currentTime = 800; left => 100px
> 
> ?

In my understandings, step-start with negative values still always produces 100px here because the value of step-start at 0.0 is 1.0. Right?

> @@ +364,5 @@
> > +  var target = createDiv(t);
> > +  target.style.position = 'absolute';
> > +  var anim = target.animate(
> > +    // http://cubic-bezier.com/#1,1.5,1,1.5
> > +    [ { left: '0px', easing: 'cubic-bezier(1, 1.5, 1, 1.5)' },
> 
> Shouldn't we test some cases where we have both keyframe-level and
> effect-level easing and where the control points are *not* horizontally
> coincident with the end points?

The cases we have both of easings was split into two cases, one is for the case you mentioned, the other is for coincident case.
Attachment #8717871 - Attachment is obsolete: true
(Assignee)

Comment 99

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #98)
> Created attachment 8720044 [details] [diff] [review]
> Part 13: Tests for effect-level easing
> 
> The tests for steps(start) in this patch rely on the patch I attached in bug
> 1248532.
> I am holding a review request until that bug is closed.
> 
> (In reply to Brian Birtles (:birtles) from comment #92)
> 
> > @@ +259,5 @@
> > > +  /* The bezier function produces negative values in (0, 0.766312060) */
> > > +  anim.currentTime = 0;
> > > +  assert_equals(getComputedStyle(target).left, '0px');
> > > +  anim.currentTime = 750;
> > > +  assert_equals(getComputedStyle(target).left, '0px');
> > 
> > For this I would expect:
> > 
> > currentTime = 0; left => 100px
> > currentTime = 250; left => 0px
> > currentTime = 750; left => 0px
> > currentTime = 800; left => 100px
> > 
> > ?
> 
> In my understandings, step-start with negative values still always produces
> 100px here because the value of step-start at 0.0 is 1.0. Right?

Brian told me I was wrong on IRC.  We need to fix part 11 patch (attachment 8717868 [details] [diff] [review]) too.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #99)
> Brian told me I was wrong on IRC.  We need to fix part 11 patch (attachment
> 8717868 [details] [diff] [review]) too.

I think my original comment is possibly wrong but I agree that we probably need to change the patch to match what the "before flag" handling in the spec.[1]

[1] http://w3c.github.io/web-animations/#before-flag
(Assignee)

Comment 101

2 years ago
Created attachment 8720525 [details] [diff] [review]
Part 11: Clamp values of step functions outside [0, 1]

Based on comment #100, this patch needs to be reviewed again.
Attachment #8717868 - Attachment is obsolete: true
Attachment #8720525 - Flags: review?(bbirtles)
(Assignee)

Comment 102

2 years ago
Created attachment 8720535 [details] [diff] [review]
Part 13: Tests for effect-level easing v2

Adapted to the steps(start) function behavior to the spec.
Three tests in this patch still fail due to bug 1248532.
Attachment #8720044 - Attachment is obsolete: true
Attachment #8720535 - Flags: review?(bbirtles)
Comment on attachment 8720525 [details] [diff] [review]
Part 11: Clamp values of step functions outside [0, 1]

Review of attachment 8720525 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/ComputedTimingFunction.cpp
@@ +40,5 @@
> +
> +  // For steps(start) function, we force to return 0.0 in nagative range
> +  // to achieve before-flag situations because clamping below makes
> +  // steps(start) function's value range (0,1].
> +  // See http://w3c.github.io/web-animations/#before-flag

Although this seems correct to me, I don't think this actually relates to the before flag, does it? This is just how step functions should work?

So perhaps the comment should just be:
// Since we use endpoint-exclusive timing, the output of a steps(start) timing
// function when aPortion = 0.0 is the top of the first step. When aPortion is
// negative, however, we should use the bottom of the first step. We handle negative
// values of aPortion specially here since once we clamp aPortion to [0,1] below we
// will no longer be able to distinguish to the two cases.
Attachment #8720525 - Flags: review?(bbirtles) → review+
Comment on attachment 8720535 [details] [diff] [review]
Part 13: Tests for effect-level easing v2

Review of attachment 8720535 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with those two confusing tests made easier to follow

::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html
@@ +241,5 @@
> +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +  var easing = function(x) {
> +    if (x > 1.0) {
> +      /* p3x + (p2y - p3y) / (p2x - p3x) * (x - p3x) */
> +      return 1.0 + (0 - 1) / (0.5 - 1) * (x - 1.0);

I don't understand how this test works.

We have a function called 'easing' that we call like so:

  easing(easing(x))

However, in the case where x <= 1.0, easing is simply:

  cubicBezier(0, 1.5, 1, 1.5)(x);

i.e. we're evaluating

  cubicBezier(0, 1.5, 1, 1.5)(cubicBezier(0, 1.5, 1, 1.5)(x));

But I thought that this test was supposed to be testing:

  cubicBezier(0.5, 1, 0.5, 0)(cubicBezier(0, 1.5, 1, 1.5)(x));

I can see that for x > 1 we do the extrapolation behavior of cubic-bezier(0.5, 1, 0.5, 0), but for any case where x < 1 we won't.

I think we're getting lucky because of the sample points we chose where 0 is 0 no matter where what cubic bezier we use, and 250,500,750 all happen to produce values > 1 for cubicBezier(0, 1.5, 1, 1.5). Can we make this test actually use the two functions correctly so that if we choose other sample points it still works?

@@ +278,5 @@
> +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> +  var easing = function(x) {
> +    if (x > 1.0) {
> +      // In case of the tangent on the upper boundary is infinity,
> +      // don't extrapolate outer the upper boundary

// For cubic-bezier(0, 1.5, 1, 1.5), the tangent at the endpoint (x = 1.0) is
// infinity so we should just return 1.0.

@@ +319,5 @@
> +    if (x < 0.0) {
> +      /* p0x + (p1y - p0y) / (p1x - p0x) * (x - p0x) */
> +      return (1 / 0.5) * x;
> +    }
> +    return cubicBezier(0, -0.5, 1, -0.5)(x);

As before, I'd prefer if we use the actual functions such that we get the right results even when x > 0.0. But perhaps I'm misunderstanding something?

@@ +354,5 @@
> +        easing: 'cubic-bezier(0, -0.5, 1, -0.5)' });
> +  var easing = function(x) {
> +    if (x < 0.0) {
> +      // In case of the tangent on the lower boundary is infinity,
> +      // don't extrapolate outer the lower boundary

// For cubic-bezier(0, -0.5, 1, -0.5), the tangent at the endpoint (x = 0.0) is
// infinity so we should just return 0.0.

::: testing/web-platform/tests/web-animations/testcommon.js
@@ +66,5 @@
> +function cubicBezier(x1, y1, x2, y2) {
> +    function xForT(t) {
> +      var omt = 1-t;
> +      return 3 * omt * omt * t * x1 + 3 * omt * t * t * x2 + t * t * t;
> +    }

This is a bit weird: We mix 4-space and 2-space indents here.

I think the 4-space indents just came about because this file was originally written by some contractors for Google. We should just go ahead and make it all use 2-spaces (and I'm pretty sure we don't require reviews for whitespace-only changes).
Attachment #8720535 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 105

2 years ago
(In reply to Brian Birtles (:birtles) from comment #104)
> Comment on attachment 8720535 [details] [diff] [review]
> Part 13: Tests for effect-level easing v2
> 
> Review of attachment 8720535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=birtles with those two confusing tests made easier to follow
> 
> :::
> testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html
> @@ +241,5 @@
> > +        easing: 'cubic-bezier(0, 1.5, 1, 1.5)' });
> > +  var easing = function(x) {
> > +    if (x > 1.0) {
> > +      /* p3x + (p2y - p3y) / (p2x - p3x) * (x - p3x) */
> > +      return 1.0 + (0 - 1) / (0.5 - 1) * (x - 1.0);
> 
> I don't understand how this test works.
> 
> We have a function called 'easing' that we call like so:
> 
>   easing(easing(x))
> 
> However, in the case where x <= 1.0, easing is simply:
> 
>   cubicBezier(0, 1.5, 1, 1.5)(x);
> 
> i.e. we're evaluating
> 
>   cubicBezier(0, 1.5, 1, 1.5)(cubicBezier(0, 1.5, 1, 1.5)(x));
> 
> But I thought that this test was supposed to be testing:
> 
>   cubicBezier(0.5, 1, 0.5, 0)(cubicBezier(0, 1.5, 1, 1.5)(x));
> 
> I can see that for x > 1 we do the extrapolation behavior of
> cubic-bezier(0.5, 1, 0.5, 0), but for any case where x < 1 we won't.
> 
> I think we're getting lucky because of the sample points we chose where 0 is
> 0 no matter where what cubic bezier we use, and 250,500,750 all happen to
> produce values > 1 for cubicBezier(0, 1.5, 1, 1.5). Can we make this test
> actually use the two functions correctly so that if we choose other sample
> points it still works?

Thanks!  I did forget to change the inside of easing function.  Your observations are quite right.  The test accidentally passes.  I will revise it and request a review once more.  Sorry for taking time.
(Assignee)

Comment 106

2 years ago
Created attachment 8720595 [details] [diff] [review]
Part 13: Tests for effect-level easing v3

* Extrapolation function are separated as each other function variable and have some assertions in itself.
* Indentation changes in testcommon.js
* Making comment styles consistent (there were both '//' and '/**/'.)

Thank you for careful reviewing, and thank you always for corrections in English sentence!
Attachment #8720535 - Attachment is obsolete: true
Attachment #8720595 - Flags: review?(bbirtles)
Comment on attachment 8720595 [details] [diff] [review]
Part 13: Tests for effect-level easing v3

Review of attachment 8720595 [details] [diff] [review]:
-----------------------------------------------------------------

You need to fix the commit message: it looks like you used hg qfold.

::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html
@@ +247,5 @@
> +    return cubicBezier(0.5, 1, 0.5, 0)(x);
> +  }
> +  var keyframeEasingExtrapolated = function(x) {
> +    assert_greater_than(x, 1.0,
> +      'This function should be called in (1.0, inifinity) range');

s/inifinity/infinity/

@@ +299,5 @@
> +    return cubicBezier(0, 1.5, 1, 1.5)(x);
> +  }
> +  var easingExtrapolated = function(x) {
> +    assert_greater_than(x, 1.0,
> +      'This function should be called in nagative range');

s/nagative/negative/

@@ +350,5 @@
> +    return cubicBezier(0.5, 1, 0.5, 0)(x);
> +  }
> +  var keyframeEasingExtrapolated = function(x) {
> +    assert_less_than(x, 0.0,
> +      'This function should be called in nagative range');

negative

@@ +402,5 @@
> +    return cubicBezier(0, -0.5, 1, -0.5)(x);
> +  }
> +  var easingExtrapolated = function(x) {
> +    assert_less_than(x, 0.0,
> +      'This function should be called in nagative range');

negative
Attachment #8720595 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 108

2 years ago
Created attachment 8720997 [details] [diff] [review]
Part 10: Remove the limit of the computed timing progress.

Tests have been changed what Brian suggested in comment #70.  I've never thought of using iterations for reftest. Amazing!
Attachment #8717860 - Attachment is obsolete: true
Attachment #8720997 - Flags: review+
(Assignee)

Comment 109

2 years ago
Created attachment 8720998 [details] [diff] [review]
Part 11: Clamp values of step functions outside [0, 1] v2

Fix comments.
Attachment #8720525 - Attachment is obsolete: true
Attachment #8720998 - Flags: review+
(Assignee)

Comment 110

2 years ago
Created attachment 8720999 [details] [diff] [review]
Part 12: Extrapolate bezier function outside [0,1] v3

Fix comments.
Attachment #8720999 - Flags: review+
(Assignee)

Comment 111

2 years ago
Created attachment 8721000 [details] [diff] [review]
Part 13: Tests for effect-level easing v4

Fix commit message and descriptions for assertions.
Attachment #8721000 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8719675 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8720595 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8720997 - Flags: checkin?
(Assignee)

Updated

2 years ago
Attachment #8720998 - Flags: checkin?
(Assignee)

Updated

2 years ago
Attachment #8720999 - Flags: checkin?
(Assignee)

Updated

2 years ago
Attachment #8721000 - Flags: checkin?
(Assignee)

Comment 112

2 years ago
Sorry for lots of spamy messages.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ced244b5b7b

Hello sheriffs, please check-in part 10 to part 13 patches. Thank you!
Keywords: leave-open → checkin-needed

Updated

2 years ago
Attachment #8720997 - Flags: checkin? → checkin+

Updated

2 years ago
Attachment #8720998 - Flags: checkin? → checkin+

Updated

2 years ago
Attachment #8720999 - Flags: checkin? → checkin+

Updated

2 years ago
Attachment #8721000 - Flags: checkin? → checkin+

Comment 114

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99a2bb2048e1
https://hg.mozilla.org/mozilla-central/rev/2ae4553b5b21
https://hg.mozilla.org/mozilla-central/rev/94f20e10fc28
https://hg.mozilla.org/mozilla-central/rev/98d27c4336aa
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.