Rename media::Clock to media::TimeDeltaInterpolator and update API.

Rename media::Clock to media::TimeDeltaInterpolator and update API.

Mostly cosmetic API changes. The guts of TimeDeltaInterpolator were
spruced up a bit.

BUG=370634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282186

[scherkus (not reviewing), 2014-07-08 22:35:24]

acolwell/xhwang: can you take a look?

hoping to get some feedback on naming / APIs. if you like it, then feel free to
do a full review

[scherkus (not reviewing), 2014-07-08 22:36:50]

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.h#ne...
media/base/pipeline.h:325: void PauseClockAndStopRendering_Locked();
note to self: I need to update these method names

[xhwang, 2014-07-08 23:16:10]

lgtm

[acolwell GONE FROM CHROMIUM, 2014-07-09 00:42:46]

https://codereview.chromium.org/376013003/diff/20001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/android/media...
media/base/android/media_source_player.cc:675:
interpoatlor_.StartInterpolating();
s/poatl/polat/

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:693: interpolator_->SetTime(time, time);
This form appears to be used in a bunch of places. It seems like this should be
something along the lines of Reset(time) instead since it is pretty much always
used to set & pin the time to a specific value.

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:887: interpolator_->SetMaxTime(duration_);
Yeah.. this stuff should be in the interpolator_

How about having StartInterpolating(duration_) and
StartInterpolating(kNoTimestamp()) or something to differentiate the audio &
video situations.

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.h#ne...
media/base/pipeline.h:367: enum InterpolationState {
It seems weird that we are keeping track of this state outside of the
interpolator. It seems like we are missing a call on the interpolator_ object so
that we can differentiate the audio & video case inside that object.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
File media/base/time_delta_interpolator.cc (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.cc:57: if (!interpolating_ || underflow_)
hmm.. seems like we should merge these into a single state variable. Once we
underflow, we aren't technically interpolating anymore since we clamp to the
upper bound.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
File media/base/time_delta_interpolator.h (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:51: void SetTime(base::TimeDelta
current_time, base::TimeDelta max_time);
s/SetTime/SetInterpolationRange/ , s/SetTime/SetBounds/ or something like that.
ISTM it is better to think of this in terms of start and end points of the
interpolation instead of using the old clock concepts.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:56: void SetMaxTime(base::TimeDelta
max_time);
s/SetMaxTime/SetUpperBound/ since that is what the description says.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:92: base::TimeDelta media_time_;
nit: s/media_time_/lower_bound_/?

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:98: base::TimeDelta max_time_;
nit: s/max_time_/upper_bound_/?

[scherkus (not reviewing), 2014-07-09 01:54:32]

https://codereview.chromium.org/376013003/diff/20001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/android/media...
media/base/android/media_source_player.cc:675:
interpoatlor_.StartInterpolating();
On 2014/07/09 00:42:46, acolwell wrote:
> s/poatl/polat/

Done.

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:693: interpolator_->SetTime(time, time);
On 2014/07/09 00:42:46, acolwell wrote:
> This form appears to be used in a bunch of places. It seems like this should
be
> something along the lines of Reset(time) instead since it is pretty much
always
> used to set & pin the time to a specific value.

I'm going to the keep the API roughly intact for this CL, but I agree there is,
uhm, a lot of opportunity to make things better :)

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:887: interpolator_->SetMaxTime(duration_);
On 2014/07/09 00:42:46, acolwell wrote:
> Yeah.. this stuff should be in the interpolator_
> 
> How about having StartInterpolating(duration_) and
> StartInterpolating(kNoTimestamp()) or something to differentiate the audio &
> video situations.

Ditto.

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/pipeline.h#ne...
media/base/pipeline.h:367: enum InterpolationState {
On 2014/07/09 00:42:46, acolwell wrote:
> It seems weird that we are keeping track of this state outside of the
> interpolator. It seems like we are missing a call on the interpolator_ object
so
> that we can differentiate the audio & video case inside that object.

Yeah I have some future CLs in mind to help clean more of this up. Going to hold
off for now.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
File media/base/time_delta_interpolator.cc (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.cc:57: if (!interpolating_ || underflow_)
On 2014/07/09 00:42:46, acolwell wrote:
> hmm.. seems like we should merge these into a single state variable. Once we
> underflow, we aren't technically interpolating anymore since we clamp to the
> upper bound.

I noticed the same thing but wanted to keep this CL mostly cosmetic ... but I
decided to change it anyway. Unit tests should help provide coverage.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
File media/base/time_delta_interpolator.h (right):

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:51: void SetTime(base::TimeDelta
current_time, base::TimeDelta max_time);
On 2014/07/09 00:42:46, acolwell wrote:
> s/SetTime/SetInterpolationRange/ , s/SetTime/SetBounds/ or something like
that.
> ISTM it is better to think of this in terms of start and end points of the
> interpolation instead of using the old clock concepts.

Agreed -- done.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:56: void SetMaxTime(base::TimeDelta
max_time);
On 2014/07/09 00:42:46, acolwell wrote:
> s/SetMaxTime/SetUpperBound/ since that is what the description says.

Done.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:92: base::TimeDelta media_time_;
On 2014/07/09 00:42:46, acolwell wrote:
> nit: s/media_time_/lower_bound_/?

Done.

https://codereview.chromium.org/376013003/diff/20001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:98: base::TimeDelta max_time_;
On 2014/07/09 00:42:46, acolwell wrote:
> nit: s/max_time_/upper_bound_/?

Done.

[acolwell GONE FROM CHROMIUM, 2014-07-09 20:17:30]

lgtm

https://codereview.chromium.org/376013003/diff/80001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/376013003/diff/80001/media/base/android/media...
media/base/android/media_source_player.cc:674: if
(!interpolator_.interpolating())
:)

https://codereview.chromium.org/376013003/diff/80001/media/base/time_delta_in...
File media/base/time_delta_interpolator.h (right):

https://codereview.chromium.org/376013003/diff/80001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:52: void
SetInterpolationRange(base::TimeDelta lower_bound,
nit: How about SetBounds()? Sorry to change my bind here, but now that both
parameters have bound in their name it seems more appropriate.

[scherkus (not reviewing), 2014-07-09 20:34:21]

https://codereview.chromium.org/376013003/diff/80001/media/base/time_delta_in...
File media/base/time_delta_interpolator.h (right):

https://codereview.chromium.org/376013003/diff/80001/media/base/time_delta_in...
media/base/time_delta_interpolator.h:52: void
SetInterpolationRange(base::TimeDelta lower_bound,
On 2014/07/09 20:17:30, acolwell wrote:
> nit: How about SetBounds()? Sorry to change my bind here, but now that both
> parameters have bound in their name it seems more appropriate.

no worries -- nothing a little "git grep -l SetInterpolationRange -- media/ |
xargs sed -i 's/SetInterpolationRange/SetBounds/g' && git cl format" can't fix!

[scherkus (not reviewing), 2014-07-09 20:34:24]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-07-09 20:37:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/376013003/100001

[commit-bot: I haz the power, 2014-07-10 00:10:15]

Change committed as 282186