CC: Allow cubic-bezier timing inputs outside the range [0, 1]

CC: Allow cubic-bezier timing inputs outside the range [0, 1]

A chromium part. Blink part: https://codereview.chromium.org/787353003

Support this blink change
https://codereview.chromium.org/238573002
but on the compositor side.

BUG=445507
R=dstockwell@chromium.org
R=ajuma@chromium.org
TEST=ManualTests/animation/compositor-animation-steps.html

Committed: https://crrev.com/ef8c01ae40ae877c7e7ba27b822ac896e91839b4
Cr-Commit-Position: refs/heads/master@{#310671}

[loyso (OOO), 2015-01-05 06:51:51]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, dstockwell@chromium.org

[loyso (OOO), 2015-01-05 06:53:10]

loyso@chromium.org changed reviewers:
+ danakj@chromium.org

[loyso (OOO), 2015-01-05 07:00:35]

I've checked all the direct/indirect calls to gfx::CubicBezier::Solve. All of
those callers guarantee the range to be inside [0,1] interval.

[Ian Vollick, 2015-01-05 15:36:03]

vollick@chromium.org changed reviewers:
+ vollick@chromium.org

[Ian Vollick, 2015-01-05 15:36:04]

This looks totally reasonable, but I've got a nit and some dumb questions.

https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:728: // Tests that a linear
timing function works as expected for inputs outside of
A couple of dumb questions.

I'm a bit confused by the term inputs here. Are you referring to the outputs of
the timing function which will be the inputs to the keyframe
interpolation/extrapolation?

Also, this test looks like it's using a cubic timing function; is linear in
reference to the keyframe timing function?

https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
File ui/gfx/geometry/cubic_bezier_unittest.cc (right):

https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
ui/gfx/geometry/cubic_bezier_unittest.cc:174: EXPECT_EQ(2.0,
coincidentEndpoints.Solve(2.0));
It'd be nice to have a test where one of the endpoints was coincident, but the
other wasn't. Also, having the first three knots coincident would be interesting
(0.0, 0.0, 0.0, 0.0).

[loyso (OOO), 2015-01-06 02:24:54]

https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:728: // Tests that a linear
timing function works as expected for inputs outside of
On 2015/01/05 15:36:04, vollick wrote:
> A couple of dumb questions.
> 
> I'm a bit confused by the term inputs here. Are you referring to the outputs
of
> the timing function which will be the inputs to the keyframe
> interpolation/extrapolation?
Yes. The outputs of per-curve timing function will be an input for per-keyframe
timing functions.
See the changelist here:
https://codereview.chromium.org/598853003/patch/80001/90001
TransformedAnimationTime function transforms time.

> Also, this test looks like it's using a cubic timing function; is linear in
> reference to the keyframe timing function?
Yes, it tests per-keyframe linear timing function. Cubic per-curve timing
function is the only way to get input outside of range [0,1].

Overall, what's important. It's mentioned in original bug and I had to copy it
here:

We need to support this blink change:
https://codereview.chromium.org/238573002
for compositor-side animations.

So the main idea is to be consistent with the blink side. And we have a plan to
remove cc/blink redundant code in the future.

https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
File ui/gfx/geometry/cubic_bezier_unittest.cc (right):

https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
ui/gfx/geometry/cubic_bezier_unittest.cc:174: EXPECT_EQ(2.0,
coincidentEndpoints.Solve(2.0));
On 2015/01/05 15:36:04, vollick wrote:
> It'd be nice to have a test where one of the endpoints was coincident, but the
> other wasn't. Also, having the first three knots coincident would be
interesting
> (0.0, 0.0, 0.0, 0.0).

As I mentioned above, this is a port of tests from here:
https://codereview.chromium.org/238573002

I agree, we should add this test. But to be consistent, we should add it on both
sides (cc/blink). I could setup a bug and introduce it as a separate CL.
What's the priority for that?

[loyso (OOO), 2015-01-06 02:26:56]

loyso@chromium.org changed reviewers:
+ shanestephens@google.com

[Ian Vollick, 2015-01-06 03:00:47]

On 2015/01/06 02:24:54, loyso wrote:
>
https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
> File cc/animation/keyframed_animation_curve_unittest.cc (right):
> 
>
https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
> cc/animation/keyframed_animation_curve_unittest.cc:728: // Tests that a linear
> timing function works as expected for inputs outside of
> On 2015/01/05 15:36:04, vollick wrote:
> > A couple of dumb questions.
> > 
> > I'm a bit confused by the term inputs here. Are you referring to the outputs
> of
> > the timing function which will be the inputs to the keyframe
> > interpolation/extrapolation?
> Yes. The outputs of per-curve timing function will be an input for
per-keyframe
> timing functions.
> See the changelist here:
> https://codereview.chromium.org/598853003/patch/80001/90001
> TransformedAnimationTime function transforms time.
> 
> > Also, this test looks like it's using a cubic timing function; is linear in
> > reference to the keyframe timing function?
> Yes, it tests per-keyframe linear timing function. Cubic per-curve timing
> function is the only way to get input outside of range [0,1].
> 
> Overall, what's important. It's mentioned in original bug and I had to copy it
> here:
> 
> We need to support this blink change:
> https://codereview.chromium.org/238573002
> for compositor-side animations.
> 
> So the main idea is to be consistent with the blink side. And we have a plan
to
> remove cc/blink redundant code in the future.
> 
>
https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
> File ui/gfx/geometry/cubic_bezier_unittest.cc (right):
> 
>
https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
> ui/gfx/geometry/cubic_bezier_unittest.cc:174: EXPECT_EQ(2.0,
> coincidentEndpoints.Solve(2.0));
> On 2015/01/05 15:36:04, vollick wrote:
> > It'd be nice to have a test where one of the endpoints was coincident, but
the
> > other wasn't. Also, having the first three knots coincident would be
> interesting
> > (0.0, 0.0, 0.0, 0.0).
> 
> As I mentioned above, this is a port of tests from here:
> https://codereview.chromium.org/238573002
> 
> I agree, we should add this test. But to be consistent, we should add it on
both
> sides (cc/blink). I could setup a bug and introduce it as a separate CL.
> What's the priority for that?

Thanks for the responses. This change lgtm, but unless there are difficulties
I'm missing I'd prefer it if you added the few new cases to the unit test in
this patch before landing since I expect they'll be quite small. Adding the
extra unit tests to the blink side could be done in a follow up at your
convenience.

[loyso (OOO), 2015-01-06 03:10:01]

On 2015/01/06 03:00:47, vollick wrote:
> On 2015/01/06 02:24:54, loyso wrote:
> >
>
https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
> > File cc/animation/keyframed_animation_curve_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/833093002/diff/1/cc/animation/keyframed_anima...
> > cc/animation/keyframed_animation_curve_unittest.cc:728: // Tests that a
linear
> > timing function works as expected for inputs outside of
> > On 2015/01/05 15:36:04, vollick wrote:
> > > A couple of dumb questions.
> > > 
> > > I'm a bit confused by the term inputs here. Are you referring to the
outputs
> > of
> > > the timing function which will be the inputs to the keyframe
> > > interpolation/extrapolation?
> > Yes. The outputs of per-curve timing function will be an input for
> per-keyframe
> > timing functions.
> > See the changelist here:
> > https://codereview.chromium.org/598853003/patch/80001/90001
> > TransformedAnimationTime function transforms time.
> > 
> > > Also, this test looks like it's using a cubic timing function; is linear
in
> > > reference to the keyframe timing function?
> > Yes, it tests per-keyframe linear timing function. Cubic per-curve timing
> > function is the only way to get input outside of range [0,1].
> > 
> > Overall, what's important. It's mentioned in original bug and I had to copy
it
> > here:
> > 
> > We need to support this blink change:
> > https://codereview.chromium.org/238573002
> > for compositor-side animations.
> > 
> > So the main idea is to be consistent with the blink side. And we have a plan
> to
> > remove cc/blink redundant code in the future.
> > 
> >
>
https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
> > File ui/gfx/geometry/cubic_bezier_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/833093002/diff/1/ui/gfx/geometry/cubic_bezier...
> > ui/gfx/geometry/cubic_bezier_unittest.cc:174: EXPECT_EQ(2.0,
> > coincidentEndpoints.Solve(2.0));
> > On 2015/01/05 15:36:04, vollick wrote:
> > > It'd be nice to have a test where one of the endpoints was coincident, but
> the
> > > other wasn't. Also, having the first three knots coincident would be
> > interesting
> > > (0.0, 0.0, 0.0, 0.0).
> > 
> > As I mentioned above, this is a port of tests from here:
> > https://codereview.chromium.org/238573002
> > 
> > I agree, we should add this test. But to be consistent, we should add it on
> both
> > sides (cc/blink). I could setup a bug and introduce it as a separate CL.
> > What's the priority for that?
> 
> Thanks for the responses. This change lgtm, but unless there are difficulties
> I'm missing I'd prefer it if you added the few new cases to the unit test in
> this patch before landing since I expect they'll be quite small. Adding the
> extra unit tests to the blink side could be done in a follow up at your
> convenience.

Yes, I'll add a few new cases. Thanks for pointing that out!

[loyso (OOO), 2015-01-07 02:18:18]

loyso@chromium.org changed reviewers:
- shanestephens@google.com

[loyso (OOO), 2015-01-07 04:38:12]

loyso@chromium.org changed reviewers:
- ajuma@chromium.org

[dstockwell, 2015-01-07 06:05:27]

lgtm, please update the description to link to the change where the same
behavior was landed in blink
(https://src.chromium.org/viewvc/blink?view=rev&revision=172821)

[loyso (OOO), 2015-01-08 04:00:42]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2015-01-08 04:01:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833093002/20001

[commit-bot: I haz the power, 2015-01-08 06:02:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-08 06:02:55]

Exceeded time limit waiting for builds to trigger.

[loyso (OOO), 2015-01-08 06:28:18]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2015-01-08 06:29:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833093002/20001

[loyso (OOO), 2015-01-08 22:52:02]

The CQ bit was unchecked by loyso@chromium.org

[loyso (OOO), 2015-01-08 22:56:54]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2015-01-08 22:58:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833093002/40001

[commit-bot: I haz the power, 2015-01-09 02:19:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-09 02:20:42]

Patchset 3 (id:??) landed as
https://crrev.com/ef8c01ae40ae877c7e7ba27b822ac896e91839b4
Cr-Commit-Position: refs/heads/master@{#310671}