Make cc::animation::TrimTimeToCurrentIteration,cc::AnimationCurve::Duration use TimeTicks/TimeDelta

Make cc::Animation::TrimTimeToCurrentIteration and
cc::AnimationCurve::Duration use TimeTicks/TimeDelta to represent time.

This patch adds a new file time_util.h which adds new class TimeUtil.
This TimeUtil class holds helper functions to operate on TimeDelta
in double because base::TimeDelta class doesnot allow
multiply/divide operations in double.

Committed: https://crrev.com/71ff07f3e8b5d1861877cefc046bcea53125c7f7
Cr-Commit-Position: refs/heads/master@{#303790}

[patro, 2014-10-31 15:57:30]

behara.ms@samsung.com changed reviewers:
+ siva.gunturi@samsung.com

[patro, 2014-11-03 11:45:04]

behara.ms@samsung.com changed reviewers:
+ ajuma@chromium.org

[patro, 2014-11-03 11:45:06]

Kindly Review

Thanks,
Shyam Patro

[ajuma, 2014-11-03 19:13:02]

Thanks for working on this!

I'm worried about all the additional conversions between doubles and TimeDelta
that are added by this patch. In particular, calls to InSecondsF() involve
floating-point division, which (as I understand it) happens in software on ARM.

It looks like there are two reasons for these conversions:
1) The inability to directly multiply/divide TimeDelta by a double.
2) Animation curves still use doubles.

For (1), can we add support to base::TimeDelta?

Can we do (2) here too? Or do you have other ideas for how we can avoid the
additional conversions?

[ajuma, 2014-11-03 19:15:06]

On 2014/11/03 19:13:02, ajuma wrote:
> Thanks for working on this!
> 
> I'm worried about all the additional conversions between doubles and TimeDelta
> that are added by this patch. In particular, calls to InSecondsF() involve
> floating-point division, which (as I understand it) happens in software on
ARM.
> 
> It looks like there are two reasons for these conversions:
> 1) The inability to directly multiply/divide TimeDelta by a double.
> 2) Animation curves still use doubles.
> 
> For (1), can we add support to base::TimeDelta?
> 
> Can we do (2) here too? Or do you have other ideas for how we can avoid the
> additional conversions?

(And by "do (2) here too",  I mean also convert animation curves to use
TimeDelta/TimeTicks instead of using doubles.)

[patro, 2014-11-05 10:16:39]

On 2014/11/03 19:15:06, ajuma wrote:
> On 2014/11/03 19:13:02, ajuma wrote:
> > Thanks for working on this!
> > 
> > I'm worried about all the additional conversions between doubles and
TimeDelta
> > that are added by this patch. In particular, calls to InSecondsF() involve
> > floating-point division, which (as I understand it) happens in software on
> ARM.
> > 
> > It looks like there are two reasons for these conversions:
> > 1) The inability to directly multiply/divide TimeDelta by a double.
> > 2) Animation curves still use doubles.
> > 
> > For (1), can we add support to base::TimeDelta?
> > 
> > Can we do (2) here too? Or do you have other ideas for how we can avoid the
> > additional conversions?
> 
> (And by "do (2) here too",  I mean also convert animation curves to use
> TimeDelta/TimeTicks instead of using doubles.)

1) The inability to directly multiply/divide TimeDelta by a double.
For this can we add some helper functions inside cc/base?
to support double multiply/divide on TimeDelta
or
Create a new TimeDelta TimeTicks inside cc inherited form base::TimeDelta and
base::TimeTicks.
And add all the functionality required for cc.

2) Animation curves still use doubles.
I will change curve->duration() also to TimeDelta/TimeTicks in this patch.

[ajuma, 2014-11-05 17:13:30]

ajuma@chromium.org changed reviewers:
+ danakj@chromium.org, mithro@mithis.com

[ajuma, 2014-11-05 17:13:53]

On 2014/11/05 10:16:39, patro wrote:
> 1) The inability to directly multiply/divide TimeDelta by a double.
> For this can we add some helper functions inside cc/base?
> to support double multiply/divide on TimeDelta

This sounds good. It should be sufficient to add a single function that takes a
TimeDelta and a double and scales the TimeDelta by that double.

> Create a new TimeDelta TimeTicks inside cc inherited form base::TimeDelta and
> base::TimeTicks.
> And add all the functionality required for cc.

I don't think we should do this, since we can implement the function we need
using TimeDelta's existing public API (specifically,
TimeDelta::FromInternalValue and TimeDelta::ToInternalValue), and since we're
only scaling the internal value, we're not making any assumptions about the
internal value's units (that is, whether the internal value represents
microseconds or some other unit of time).

Adding danakj@ and mithro@ since they might have better ideas for what to do
here.

[danakj, 2014-11-05 19:19:43]

On Wed, Nov 5, 2014 at 12:13 PM, <ajuma@chromium.org> wrote:

> On 2014/11/05 10:16:39, patro wrote:
>
>> 1) The inability to directly multiply/divide TimeDelta by a double.
>> For this can we add some helper functions inside cc/base?
>> to support double multiply/divide on TimeDelta
>>
>
> This sounds good. It should be sufficient to add a single function that
> takes a
> TimeDelta and a double and scales the TimeDelta by that double.


+1


>
>
>  Create a new TimeDelta TimeTicks inside cc inherited form base::TimeDelta
>> and
>> base::TimeTicks.
>> And add all the functionality required for cc.
>>
>
> I don't think we should do this, since we can implement the function we
> need
> using TimeDelta's existing public API (specifically,
> TimeDelta::FromInternalValue and TimeDelta::ToInternalValue), and since
> we're
> only scaling the internal value, we're not making any assumptions about the
> internal value's units (that is, whether the internal value represents
> microseconds or some other unit of time).
>
> Adding danakj@ and mithro@ since they might have better ideas for what to
> do
> here.
>
>
>
>
> https://codereview.chromium.org/693883004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[patro, 2014-11-06 11:21:16]

Kindly Review

Thanks,
Shyam Patro

[ajuma, 2014-11-06 18:03:23]

Mostly looks fine, but I have a few more comments.

https://codereview.chromium.org/693883004/diff/100001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/693883004/diff/100001/cc/animation/animation....
cc/animation/animation.cc:231: fmod(scaled_active_time.InSecondsF(),
curve_->Duration().InSecondsF()));
Could we add a helper function (e.g. in the TimeUtil class you're adding) so
that we don't need to convert to/from doubles here? (And the helper could use
integer mod rather than fmod, since there won't be any doubles involved.)

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h#new...
cc/base/time_util.h:14: base::TimeDelta operator*(const base::TimeDelta
time_delta, double value) {
Rather than overloading operator* (which will be confusing, since then the
meaning of operator* will change depending on whether this .h file is
#included), we should just define a separate function for this. e.g. define a
TimeUtil class, with a static Scale(base::TimeDelta time_delta, double
scale_factor). That should be all that we need; to multiply time_delta by d,
call Scale(time_delta, d), and to divide by d call Scale(time_delta, 1.0/d).

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h#new...
cc/base/time_util.h:15: return
base::TimeDelta::FromInternalValue(time_delta.ToInternalValue() *
Make the double/int64 conversions explicit, e.g:

base::TimeDelta::FromInternalValue(static_cast<int64>(static_cast<double>(time_delta.ToInternalValue()
* value)));

https://codereview.chromium.org/693883004/diff/100001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/693883004/diff/100001/cc/cc.gyp#newcode95
cc/cc.gyp:95: 'base/time_util.h',
Also add this to BUILD.gn.

[ajuma, 2014-11-06 18:07:20]

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h#new...
cc/base/time_util.h:15: return
base::TimeDelta::FromInternalValue(time_delta.ToInternalValue() *
On 2014/11/06 18:03:23, ajuma wrote:
> Make the double/int64 conversions explicit, e.g:
> 
>
base::TimeDelta::FromInternalValue(static_cast<int64>(static_cast<double>(time_delta.ToInternalValue()
> * value)));

Sorry, messed up the parentheses there. I meant:
base::TimeDelta::FromInternalValue(static_cast<int64>(static_cast<double>(time_delta.ToInternalValue())
* value))

[patro, 2014-11-07 07:03:40]

https://codereview.chromium.org/693883004/diff/100001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/693883004/diff/100001/cc/animation/animation....
cc/animation/animation.cc:231: fmod(scaled_active_time.InSecondsF(),
curve_->Duration().InSecondsF()));
On 2014/11/06 18:03:23, ajuma wrote:
> Could we add a helper function (e.g. in the TimeUtil class you're adding) so
> that we don't need to convert to/from doubles here? (And the helper could use
> integer mod rather than fmod, since there won't be any doubles involved.)

Done.

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h#new...
cc/base/time_util.h:14: base::TimeDelta operator*(const base::TimeDelta
time_delta, double value) {
On 2014/11/06 18:03:23, ajuma wrote:
> Rather than overloading operator* (which will be confusing, since then the
> meaning of operator* will change depending on whether this .h file is
> #included), we should just define a separate function for this. e.g. define a
> TimeUtil class, with a static Scale(base::TimeDelta time_delta, double
> scale_factor). That should be all that we need; to multiply time_delta by d,
> call Scale(time_delta, d), and to divide by d call Scale(time_delta, 1.0/d).
> 

Done.

https://codereview.chromium.org/693883004/diff/100001/cc/base/time_util.h#new...
cc/base/time_util.h:15: return
base::TimeDelta::FromInternalValue(time_delta.ToInternalValue() *
On 2014/11/06 18:03:23, ajuma wrote:
> Make the double/int64 conversions explicit, e.g:
> 
>
base::TimeDelta::FromInternalValue(static_cast<int64>(static_cast<double>(time_delta.ToInternalValue()
> * value)));

Done.

https://codereview.chromium.org/693883004/diff/100001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/693883004/diff/100001/cc/cc.gyp#newcode95
cc/cc.gyp:95: 'base/time_util.h',
On 2014/11/06 18:03:23, ajuma wrote:
> Also add this to BUILD.gn.

Done.

[ajuma, 2014-11-07 15:58:00]

This is pretty close now, just a few more nits.

Also, please update the description to say what the patch does. (e.g. the
summary might be "Make cc::Animation::TrimTimeToCurrentIteration use
TimeTicks/TimeDelta", and then the description would add more details).

https://codereview.chromium.org/693883004/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (left):

https://codereview.chromium.org/693883004/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:6: 
Please leave this blank line as is.

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h#new...
cc/base/time_util.h:13: 
Please define a TimeUtil class and make these functions be static members of
that class (see MathUtil as an example of this pattern).

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h#new...
cc/base/time_util.h:14: static base::TimeDelta Scale(const base::TimeDelta
time_delta, double value) {
I assume you meant "const base::TimeDelta&"? A TimeDelta should be cheap to copy
(it consists of a single int64), so passing it by value should be fine. So this
should just be "base::TimeDelta".

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h#new...
cc/base/time_util.h:20: const base::TimeDelta divisor) {
Same comment as above, please remove the consts here.

[patro, 2014-11-10 09:46:52]

https://codereview.chromium.org/693883004/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (left):

https://codereview.chromium.org/693883004/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:6: 
On 2014/11/07 15:58:00, ajuma wrote:
> Please leave this blank line as is.

Done.

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h#new...
cc/base/time_util.h:13: 
On 2014/11/07 15:58:00, ajuma wrote:
> Please define a TimeUtil class and make these functions be static members of
> that class (see MathUtil as an example of this pattern).

Done.

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h#new...
cc/base/time_util.h:14: static base::TimeDelta Scale(const base::TimeDelta
time_delta, double value) {
On 2014/11/07 15:58:00, ajuma wrote:
> I assume you meant "const base::TimeDelta&"? A TimeDelta should be cheap to
copy
> (it consists of a single int64), so passing it by value should be fine. So
this
> should just be "base::TimeDelta".

Done.

https://codereview.chromium.org/693883004/diff/120001/cc/base/time_util.h#new...
cc/base/time_util.h:20: const base::TimeDelta divisor) {
On 2014/11/07 15:58:00, ajuma wrote:
> Same comment as above, please remove the consts here.

Done.

[ajuma, 2014-11-10 14:32:43]

Thanks, lgtm with a couple nits.

Please wrap the description so that each line is at most 72 characters long.

https://codereview.chromium.org/693883004/diff/140001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/140001/cc/base/time_util.h#new...
cc/base/time_util.h:19: }
Please add a blank line between these functions.

[patro, 2014-11-12 04:09:12]

https://codereview.chromium.org/693883004/diff/140001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/693883004/diff/140001/cc/base/time_util.h#new...
cc/base/time_util.h:19: }
On 2014/11/10 14:32:42, ajuma (OOO till Nov. 13) wrote:
> Please add a blank line between these functions.

Done.

[patro, 2014-11-12 04:10:05]

The CQ bit was checked by behara.ms@samsung.com

[commit-bot: I haz the power, 2014-11-12 04:10:28]

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

[commit-bot: I haz the power, 2014-11-12 05:09:17]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-11-12 05:09:59]

Patchset 9 (id:??) landed as
https://crrev.com/71ff07f3e8b5d1861877cefc046bcea53125c7f7
Cr-Commit-Position: refs/heads/master@{#303790}