Switch GetWallClockTime to using vectors for input and output.

Switch GetWallClockTime to using vectors for input and output.

Ensures that all converted timestamps will be monotonically increasing
within a single call to GetWallClockTime.

Performance seems unchanged (unit tests take the same amount of time).

As part of this change, also does the following:
- VideoRendererImpl unit test switched to WallClockTimeSource to avoid
having to reimplement the vector conversion routine.

BUG=485042
TEST=new unittest.

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

[DaleCurtis, 2015-05-09 01:44:41]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2015-05-09 01:45:49]

dalecurtis@chromium.org changed reviewers:
+ miu@chromium.org

[DaleCurtis, 2015-05-09 01:45:50]

Over to miu@ since I'm overloading xhwang@ :) Unfortunately the crashes didn't
go away after accounting for frames with the same timestamp, so we do need to do
something about this case.

[miu, 2015-05-09 03:47:06]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
I'm trying to understand how the time converter can map two monotonically
non-decreasing video frame timestamps into TimeTicks values going in a backwards
direction.  FWICT, AudioRendererImpl can return the same TimeTicks for two
different media timestamps.  Or, is this being caused by garbage from the audio
track's timing data?

Also, how does retrying help?  Does asking for time conversions alter the state
of the time converter for future identical requests?  Why/how is this supposed
to work?

Most of my questions might just be due to my own ignorance here.  If the answers
to these questions aren't simple, let's have a face-to-face chat Monday.

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:435: delta = average_frame_duration_;
Not sure whether you thought of this, but another approach might be to try:

  delta = std::min(average_frame_duration_, frame.frame->timestamp() -
frame_queue[i - 1].frame->timestamp());

Although, this wouldn't account for the non-1X playback rates, I suppose...

[DaleCurtis, 2015-05-09 04:08:43]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
On 2015/05/09 03:47:06, miu wrote:
> I'm trying to understand how the time converter can map two monotonically
> non-decreasing video frame timestamps into TimeTicks values going in a
backwards
> direction.  FWICT, AudioRendererImpl can return the same TimeTicks for two
> different media timestamps.  Or, is this being caused by garbage from the
audio
> track's timing data?
> 
> Also, how does retrying help?  Does asking for time conversions alter the
state
> of the time converter for future identical requests?  Why/how is this supposed
> to work?
> 
> Most of my questions might just be due to my own ignorance here.  If the
answers
> to these questions aren't simple, let's have a face-to-face chat Monday.

The way times goes backwards is if we've got > 1 future timestamps and the
playback rate changes on the media thread between the calls (this code is
running on the compositor thread).

Say our two timestamps are T1=10, T2=20, base_time=1000. If we query for T1 we
should get back 1010 @ 1.0x, if we query T2 and playback has changed to 5.0x,
we'd get 1004, which would show up as time going backwards.

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:435: delta = average_frame_duration_;
On 2015/05/09 03:47:06, miu wrote:
> Not sure whether you thought of this, but another approach might be to try:
> 
>   delta = std::min(average_frame_duration_, frame.frame->timestamp() -
> frame_queue[i - 1].frame->timestamp());
> 
> Although, this wouldn't account for the non-1X playback rates, I suppose...

Yeah the whole reason we have to retry is non-1.0x playback rates, so I don't
think that's a good idea :)

[xhwang, 2015-05-09 06:51:01]

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

[xhwang, 2015-05-09 06:51:02]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
On 2015/05/09 04:08:43, DaleCurtis wrote:
> On 2015/05/09 03:47:06, miu wrote:
> > I'm trying to understand how the time converter can map two monotonically
> > non-decreasing video frame timestamps into TimeTicks values going in a
> backwards
> > direction.  FWICT, AudioRendererImpl can return the same TimeTicks for two
> > different media timestamps.  Or, is this being caused by garbage from the
> audio
> > track's timing data?
> > 
> > Also, how does retrying help?  Does asking for time conversions alter the
> state
> > of the time converter for future identical requests?  Why/how is this
supposed
> > to work?
> > 
> > Most of my questions might just be due to my own ignorance here.  If the
> answers
> > to these questions aren't simple, let's have a face-to-face chat Monday.
> 
> The way times goes backwards is if we've got > 1 future timestamps and the
> playback rate changes on the media thread between the calls (this code is
> running on the compositor thread).
> 
> Say our two timestamps are T1=10, T2=20, base_time=1000. If we query for T1 we
> should get back 1010 @ 1.0x, if we query T2 and playback has changed to 5.0x,
> we'd get 1004, which would show up as time going backwards.

General comment: The video rendering path has no concept/knowledge of playback
rates. The time source handles playback rate changes and that's where this
problem comes from. So Is it possible that we fix the source of the problem
(i.e. the time source, e.g. audio renderer) to make sure time never goes back?

I haven't really thought about details, but the time source know the previous
time it has returned (which is monotonically increasing), it can do
extrapolation if it find that a returned time could go backward from a previous
returned time (1004 < 1010 in your example). This is actually what you are
doing. The effect of this is that the playback rate change will be delayed
slightly, but I don't think that'll affect any user experience.

[DaleCurtis, 2015-05-09 16:28:49]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
On 2015/05/09 06:51:01, xhwang wrote:
> On 2015/05/09 04:08:43, DaleCurtis wrote:
> > On 2015/05/09 03:47:06, miu wrote:
> > > I'm trying to understand how the time converter can map two monotonically
> > > non-decreasing video frame timestamps into TimeTicks values going in a
> > backwards
> > > direction.  FWICT, AudioRendererImpl can return the same TimeTicks for two
> > > different media timestamps.  Or, is this being caused by garbage from the
> > audio
> > > track's timing data?
> > > 
> > > Also, how does retrying help?  Does asking for time conversions alter the
> > state
> > > of the time converter for future identical requests?  Why/how is this
> supposed
> > > to work?
> > > 
> > > Most of my questions might just be due to my own ignorance here.  If the
> > answers
> > > to these questions aren't simple, let's have a face-to-face chat Monday.
> > 
> > The way times goes backwards is if we've got > 1 future timestamps and the
> > playback rate changes on the media thread between the calls (this code is
> > running on the compositor thread).
> > 
> > Say our two timestamps are T1=10, T2=20, base_time=1000. If we query for T1
we
> > should get back 1010 @ 1.0x, if we query T2 and playback has changed to
5.0x,
> > we'd get 1004, which would show up as time going backwards.
> 
> General comment: The video rendering path has no concept/knowledge of playback
> rates. The time source handles playback rate changes and that's where this
> problem comes from. So Is it possible that we fix the source of the problem
> (i.e. the time source, e.g. audio renderer) to make sure time never goes back?
> 
> I haven't really thought about details, but the time source know the previous
> time it has returned (which is monotonically increasing), it can do
> extrapolation if it find that a returned time could go backward from a
previous
> returned time (1004 < 1010 in your example). This is actually what you are
> doing. The effect of this is that the playback rate change will be delayed
> slightly, but I don't think that'll affect any user experience.

Tracking max returned TimeTicks doesn't work for two reasons: 1) VRA will
requery time with an earlier timestamp every Render(). 2) Frames may be received
out of order, so even if it tracked timestamps, trouble can occur.

Even if those issues were solved, there's a third reason it may not work well,
for low frame rate videos (which are probably the likeliest to have playback
rate changes), the duration between frames is long enough to make the playback
rate switch noticeable if we never let time go backwards.

I don't see a way around a retry outside of switching GetWallClockTime to
vectors of input and output or (something I just thought of) modifying
GetWallClockTime to have acquire, release states for the lock. I.e.

GetWallClockTime(ts, RequestType=Single); // Lock acquired and released.
GetWallClockTime(ts, RequestType=MultiBegin); // Lock acquired.
GetWallClockTime(ts, RequestType=MultiEnd); // Lock released.

I kind of like this solution, it avoids repeated locking and allows for
monotonicity to be easily maintained with less complexity than my current
approach. WDYT?

[DaleCurtis, 2015-05-09 21:44:42]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
On 2015/05/09 16:28:48, DaleCurtis wrote:
> On 2015/05/09 06:51:01, xhwang wrote:
> > On 2015/05/09 04:08:43, DaleCurtis wrote:
> > > On 2015/05/09 03:47:06, miu wrote:
> > > > I'm trying to understand how the time converter can map two
monotonically
> > > > non-decreasing video frame timestamps into TimeTicks values going in a
> > > backwards
> > > > direction.  FWICT, AudioRendererImpl can return the same TimeTicks for
two
> > > > different media timestamps.  Or, is this being caused by garbage from
the
> > > audio
> > > > track's timing data?
> > > > 
> > > > Also, how does retrying help?  Does asking for time conversions alter
the
> > > state
> > > > of the time converter for future identical requests?  Why/how is this
> > supposed
> > > > to work?
> > > > 
> > > > Most of my questions might just be due to my own ignorance here.  If the
> > > answers
> > > > to these questions aren't simple, let's have a face-to-face chat Monday.
> > > 
> > > The way times goes backwards is if we've got > 1 future timestamps and the
> > > playback rate changes on the media thread between the calls (this code is
> > > running on the compositor thread).
> > > 
> > > Say our two timestamps are T1=10, T2=20, base_time=1000. If we query for
T1
> we
> > > should get back 1010 @ 1.0x, if we query T2 and playback has changed to
> 5.0x,
> > > we'd get 1004, which would show up as time going backwards.
> > 
> > General comment: The video rendering path has no concept/knowledge of
playback
> > rates. The time source handles playback rate changes and that's where this
> > problem comes from. So Is it possible that we fix the source of the problem
> > (i.e. the time source, e.g. audio renderer) to make sure time never goes
back?
> > 
> > I haven't really thought about details, but the time source know the
previous
> > time it has returned (which is monotonically increasing), it can do
> > extrapolation if it find that a returned time could go backward from a
> previous
> > returned time (1004 < 1010 in your example). This is actually what you are
> > doing. The effect of this is that the playback rate change will be delayed
> > slightly, but I don't think that'll affect any user experience.
> 
> Tracking max returned TimeTicks doesn't work for two reasons: 1) VRA will
> requery time with an earlier timestamp every Render(). 2) Frames may be
received
> out of order, so even if it tracked timestamps, trouble can occur.
> 
> Even if those issues were solved, there's a third reason it may not work well,
> for low frame rate videos (which are probably the likeliest to have playback
> rate changes), the duration between frames is long enough to make the playback
> rate switch noticeable if we never let time go backwards.
> 
> I don't see a way around a retry outside of switching GetWallClockTime to
> vectors of input and output or (something I just thought of) modifying
> GetWallClockTime to have acquire, release states for the lock. I.e.
> 
> GetWallClockTime(ts, RequestType=Single); // Lock acquired and released.
> GetWallClockTime(ts, RequestType=MultiBegin); // Lock acquired.
> GetWallClockTime(ts, RequestType=MultiEnd); // Lock released.
> 
> I kind of like this solution, it avoids repeated locking and allows for
> monotonicity to be easily maintained with less complexity than my current
> approach. WDYT?

For my own edification I was curious what this approach would look like, it's
not too bad https://codereview.chromium.org/1129323003/ but exposing lock state
like this is always a little scary. I do prefer it to this retry and clamp code
though.

[miu, 2015-05-09 22:33:00]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
On 2015/05/09 21:44:42, DaleCurtis wrote:
> On 2015/05/09 16:28:48, DaleCurtis wrote:
> > On 2015/05/09 06:51:01, xhwang wrote:
> > > On 2015/05/09 04:08:43, DaleCurtis wrote:
> > > > On 2015/05/09 03:47:06, miu wrote:
> > > > > I'm trying to understand how the time converter can map two
> monotonically
> > > > > non-decreasing video frame timestamps into TimeTicks values going in a
> > > > backwards
> > > > > direction.  FWICT, AudioRendererImpl can return the same TimeTicks for
> > 
> > ...
> >
> > GetWallClockTime(ts, RequestType=Single); // Lock acquired and released.
> > GetWallClockTime(ts, RequestType=MultiBegin); // Lock acquired.
> > GetWallClockTime(ts, RequestType=MultiEnd); // Lock released.
> 
> For my own edification I was curious what this approach would look like, it's
> not too bad https://codereview.chromium.org/1129323003/ but exposing lock
state
> like this is always a little scary. I do prefer it to this retry and clamp
code
> though.

Another idea to consider: What if we resolve the out-of-order TimeTicks by
dropping the frame(s) with the older VideoFrame::timestamp from the
|frame_queue_|?  Dropping a frame might not be a bad option since:

1. It should only be a momentary thing, and shouldn't result in any kind of jank
that would diminish the user experience.  I'd expect that typically only one
frame would need to be dropped to resolve the issue.

2. Dropping an older frame allows the newer, more relevant frames to have more
influence on the rendering timing calculations.

3. This is simpler than the "retry" approach.

4. This is safer than the "externally hold a lock owned by the time converter"
approach.

[DaleCurtis, 2015-05-09 23:02:42]

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:420: if (delta < base::TimeDelta()) {
On 2015/05/09 22:33:00, miu wrote:
> On 2015/05/09 21:44:42, DaleCurtis wrote:
> > On 2015/05/09 16:28:48, DaleCurtis wrote:
> > > On 2015/05/09 06:51:01, xhwang wrote:
> > > > On 2015/05/09 04:08:43, DaleCurtis wrote:
> > > > > On 2015/05/09 03:47:06, miu wrote:
> > > > > > I'm trying to understand how the time converter can map two
> > monotonically
> > > > > > non-decreasing video frame timestamps into TimeTicks values going in
a
> > > > > backwards
> > > > > > direction.  FWICT, AudioRendererImpl can return the same TimeTicks
for
> > > 
> > > ...
> > >
> > > GetWallClockTime(ts, RequestType=Single); // Lock acquired and released.
> > > GetWallClockTime(ts, RequestType=MultiBegin); // Lock acquired.
> > > GetWallClockTime(ts, RequestType=MultiEnd); // Lock released.
> > 
> > For my own edification I was curious what this approach would look like,
it's
> > not too bad https://codereview.chromium.org/1129323003/ but exposing lock
> state
> > like this is always a little scary. I do prefer it to this retry and clamp
> code
> > though.
> 
> Another idea to consider: What if we resolve the out-of-order TimeTicks by
> dropping the frame(s) with the older VideoFrame::timestamp from the
> |frame_queue_|?  Dropping a frame might not be a bad option since:
> 
> 1. It should only be a momentary thing, and shouldn't result in any kind of
jank
> that would diminish the user experience.  I'd expect that typically only one
> frame would need to be dropped to resolve the issue.
> 
> 2. Dropping an older frame allows the newer, more relevant frames to have more
> influence on the rendering timing calculations.
> 
> 3. This is simpler than the "retry" approach.
> 
> 4. This is safer than the "externally hold a lock owned by the time converter"
> approach.

Since these are predicted timestamps, they may also go back in time as the audio
renderer plays them out and gets a more accurate ts.  I don't think you can
assume they won't cause a glitch unless you make some restrictions about how the
time changed relative to the current deadline, which seems more complicated than
simply retrying.

[miu, 2015-05-11 21:45:45]

Took a look at the other approach.  IMO, I don't care for either.  Mainly, I
don't like burdening the client with solving this problem, whether that's "retry
until it works" or "be responsible for managing a lock across multiple calls."

I think you should provide two APIs: The first is the
GetWallClockTime(TimeDelta) unchanged.  This is the common-case usage.  The
second, needed for the new use case, is GetWallClockTimes(vector<TimeDelta>*). 
I think this is a good approach because:

1. Code simplicity and readability w/o much overhead.  If you're concerned about
copying values into a vector and then back out of the result vector, you could
make the GetWallClockTimes() method take iterator arguments, like
std::transform().  If you're concerned about the cost of allocating vectors, the
client could just keep a reusable instance around (as a class member).

2. Perhaps more importantly, the implementation of GetWallClockTimes() can now
guarantee the TimeTicks results it generates are monotonically non-decreasing.

WDYT?

[DaleCurtis, 2015-05-11 21:50:28]

Hmm, I'll see about keeping the other method around, but there's really only one
client which is VRI, so it makes sense to just collapse this to a single method
which takes a vector<TimeDelta> in and returns a vector<TimeTicks> or if we want
to save on the vectors a single vector<int64_t> in/out which wraps and unwraps
TimeDelta -> TimeTicks....

[DaleCurtis, 2015-05-11 22:20:59]

One more possibility instead of using a vector:

  // MediaTimeCB will should return each media time for conversion as well as
  // an output pointer to store the converted TimeTicks in.  Conversion will
  // stop if kNoTimestamp() is returned.
  typedef base::Callback<base::TimeDelta(base::TimeTicks*)> MediaTimeCB;
  virtual bool GetWallClockTime(const MediaTimeCB& time_cb);

[xhwang, 2015-05-11 22:35:38]

On 2015/05/11 22:20:59, DaleCurtis wrote:
> One more possibility instead of using a vector:
> 
>   // MediaTimeCB will should return each media time for conversion as well as
>   // an output pointer to store the converted TimeTicks in.  Conversion will
>   // stop if kNoTimestamp() is returned.
>   typedef base::Callback<base::TimeDelta(base::TimeTicks*)> MediaTimeCB;
>   virtual bool GetWallClockTime(const MediaTimeCB& time_cb);

Passing in a vector<TimeDelta> and returning a vector<TimeTicks> in
GetWallClockTimes() sgtm. We will make sure the returned times are monotonically
increasing within this vector. But it doesn't guarantee the time returned in
different GetWallClockTimes() calls are monotonically increasing. This is not
perfect, but is simple and should be good enough.

The MediaTimeCB approach seems overkill and IMHO a bit confusing.

[DaleCurtis, 2015-05-12 00:41:44]

Patchset #2 (id:40001) has been deleted

[DaleCurtis, 2015-05-12 01:27:12]

Patchset #2 (id:60001) has been deleted

[DaleCurtis, 2015-05-12 01:32:34]

PTAL, this uses an input and output vector as discussed. It's not too hairy and
the UpdateFrameStatistics() code is actually a little nicer.

https://codereview.chromium.org/1136513004/diff/80001/media/base/wall_clock_t...
File media/base/wall_clock_time_source.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/base/wall_clock_t...
media/base/wall_clock_time_source.h:32: void
set_tick_clock_for_testing(base::TickClock* tick_clock) {
Required for use in VideoRendererImpl unittest since another class is already
supposed to be the TickClock owner.

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.h:163: // True if this frame's end time
is based on the average frame duration and
This simplifies the code a bit and makes it easier to reason about the
correctness of one sample per frame.

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm_unittest.cc (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm_unittest.cc:608: EXPECT_EQ(1u,
algorithm_.EffectiveFramesQueued());
Since we return false and keep all previously calculated frame times, this is
now 1u since the last frame still counts as ineffective.

[miu, 2015-05-12 02:46:57]

lgtm

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:19: estimated_end_time(true),
Do you need this boolean?  It looks like estimated_end_time will always be equal
to end_time.is_null().

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:421: frame_queue_.back().start_time =
wall_clock_times.back();
nit: DCHECK(frame_queue_.back().end_time.is_null());

[DaleCurtis, 2015-05-12 02:48:49]

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:19: estimated_end_time(true),
On 2015/05/12 02:46:57, miu wrote:
> Do you need this boolean?  It looks like estimated_end_time will always be
equal
> to end_time.is_null().

end_time is set to start_time + average_frame_duration for the last frame in the
queue, so that won't work.

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:421: frame_queue_.back().start_time =
wall_clock_times.back();
On 2015/05/12 02:46:57, miu wrote:
> nit: DCHECK(frame_queue_.back().end_time.is_null());

See l.431, this is not correct.

[miu, 2015-05-12 02:52:46]

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:19: estimated_end_time(true),
On 2015/05/12 02:48:49, DaleCurtis wrote:
> On 2015/05/12 02:46:57, miu wrote:
> > Do you need this boolean?  It looks like estimated_end_time will always be
> equal
> > to end_time.is_null().
> 
> end_time is set to start_time + average_frame_duration for the last frame in
the
> queue, so that won't work.

OIC.  So, my only comment now is naming nit: Can we call this
has_estimated_end_time or end_time_was_estimated?

[DaleCurtis, 2015-05-12 02:58:59]

Thanks for review!

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:19: estimated_end_time(true),
On 2015/05/12 02:52:46, miu wrote:
> On 2015/05/12 02:48:49, DaleCurtis wrote:
> > On 2015/05/12 02:46:57, miu wrote:
> > > Do you need this boolean?  It looks like estimated_end_time will always be
> > equal
> > > to end_time.is_null().
> > 
> > end_time is set to start_time + average_frame_duration for the last frame in
> the
> > queue, so that won't work.
> 
> OIC.  So, my only comment now is naming nit: Can we call this
> has_estimated_end_time or end_time_was_estimated?

has_estimated_end_time sgtm, I'll change it shortly.

[xhwang, 2015-05-12 04:46:53]

lgtm with nits

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:55: // wall clock time does not go backwards for
monotonically increasing media
Q for "does not go backwards": Can f(a) == f(b) for a < b?

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:70: virtual bool GetWallClockTime(
bike shedding: should it be GetWallClockTime_s_?

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.h:48: const TimeSource::WallClockTimeCB&
time_converter_cb);
We are using WallClockTimeCB and wall_clock_time_cb_, just
s/time_converter_cb/wall_clock_time_cb/?

https://codereview.chromium.org/1136513004/diff/100001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1136513004/diff/100001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:190:
audio_clock_->TimeUntilPlayback(media_timestamp));
nit: will base_time = media_timestamp work? Or are you worried about
performance?

[DaleCurtis, 2015-05-12 05:35:42]

Thanks for the reviews!

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:55: // wall clock time does not go backwards for
monotonically increasing media
On 2015/05/12 04:46:53, xhwang wrote:
> Q for "does not go backwards": Can f(a) == f(b) for a < b? 

Not unless there's a rounding error, f(a) != f(b) if a != b.

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:70: virtual bool GetWallClockTime(
On 2015/05/12 04:46:53, xhwang wrote:
> bike shedding: should it be GetWallClockTime_s_?

Done.

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/filters/video_ren...
media/filters/video_renderer_algorithm.h:48: const TimeSource::WallClockTimeCB&
time_converter_cb);
On 2015/05/12 04:46:53, xhwang wrote:
> We are using WallClockTimeCB and wall_clock_time_cb_, just
> s/time_converter_cb/wall_clock_time_cb/?

Done.

https://codereview.chromium.org/1136513004/diff/100001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1136513004/diff/100001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:190:
audio_clock_->TimeUntilPlayback(media_timestamp));
On 2015/05/12 04:46:53, xhwang wrote:
> nit: will base_time = media_timestamp work? Or are you worried about
> performance?

It would, but I felt it was misleading to have the playback rate calculation
used in this case since there may be multiple playback rates within the
AudioClock bounds.

[DaleCurtis, 2015-05-12 05:38:42]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-05-12 05:38:42]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1136513004/#ps120001
(title: "Fix comments.")

[commit-bot: I haz the power, 2015-05-12 05:38:55]

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

[xhwang, 2015-05-12 05:45:42]

A nit. You can always update that later (if you agree).

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:55: // wall clock time does not go backwards for
monotonically increasing media
On 2015/05/12 05:35:42, DaleCurtis wrote:
> On 2015/05/12 04:46:53, xhwang wrote:
> > Q for "does not go backwards": Can f(a) == f(b) for a < b? 
> 
> Not unless there's a rounding error, f(a) != f(b) if a != b.

nit: "does not go backwards" means a >= b, which should be a > b in this case.
Sorry for not being clear.

[commit-bot: I haz the power, 2015-05-12 06:38:11]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-05-12 06:38:50]

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

[DaleCurtis, 2015-05-12 16:50:04]

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:55: // wall clock time does not go backwards for
monotonically increasing media
On 2015/05/12 05:45:42, xhwang wrote:
> On 2015/05/12 05:35:42, DaleCurtis wrote:
> > On 2015/05/12 04:46:53, xhwang wrote:
> > > Q for "does not go backwards": Can f(a) == f(b) for a < b? 
> > 
> > Not unless there's a rounding error, f(a) != f(b) if a != b.
> 
> nit: "does not go backwards" means a >= b, which should be a > b in this case.
> Sorry for not being clear.

I'm still not clear on your nit here :) Within a single run, the code will
ensure f(a) > f(b) for a > b, f(a) < (b)  for a < b, and f(a) == f(b) for a ==
b.

Is your comment asking me to clarify that more exactly here?

[xhwang, 2015-05-12 16:57:47]

On 2015/05/12 16:50:04, DaleCurtis wrote:
> https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
> File media/base/time_source.h (right):
> 
>
https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
> media/base/time_source.h:55: // wall clock time does not go backwards for
> monotonically increasing media
> On 2015/05/12 05:45:42, xhwang wrote:
> > On 2015/05/12 05:35:42, DaleCurtis wrote:
> > > On 2015/05/12 04:46:53, xhwang wrote:
> > > > Q for "does not go backwards": Can f(a) == f(b) for a < b? 
> > > 
> > > Not unless there's a rounding error, f(a) != f(b) if a != b.
> > 
> > nit: "does not go backwards" means a >= b, which should be a > b in this
case.
> > Sorry for not being clear.
> 
> I'm still not clear on your nit here :) Within a single run, the code will
> ensure f(a) > f(b) for a > b, f(a) < (b)  for a < b, and f(a) == f(b) for a ==
> b.
> 
> Is your comment asking me to clarify that more exactly here?

"go backwards" means f(a) < f(b) for a > b. So "not go backwards for
monotonically increasing media" means f(a) >= f(b) for a > b. I think the
problem is that when f(a) == f(b) for a > b, it didn't go backwards, which isn't
what you want to support.

[DaleCurtis, 2015-05-12 17:03:35]

On 2015/05/12 16:57:47, xhwang wrote:
> On 2015/05/12 16:50:04, DaleCurtis wrote:
> >
https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
> > File media/base/time_source.h (right):
> > 
> >
>
https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
> > media/base/time_source.h:55: // wall clock time does not go backwards for
> > monotonically increasing media
> > On 2015/05/12 05:45:42, xhwang wrote:
> > > On 2015/05/12 05:35:42, DaleCurtis wrote:
> > > > On 2015/05/12 04:46:53, xhwang wrote:
> > > > > Q for "does not go backwards": Can f(a) == f(b) for a < b? 
> > > > 
> > > > Not unless there's a rounding error, f(a) != f(b) if a != b.
> > > 
> > > nit: "does not go backwards" means a >= b, which should be a > b in this
> case.
> > > Sorry for not being clear.
> > 
> > I'm still not clear on your nit here :) Within a single run, the code will
> > ensure f(a) > f(b) for a > b, f(a) < (b)  for a < b, and f(a) == f(b) for a
==
> > b.
> > 
> > Is your comment asking me to clarify that more exactly here?
> 
> "go backwards" means f(a) < f(b) for a > b. So "not go backwards for
> monotonically increasing media" means f(a) >= f(b) for a > b. I think the
> problem is that when f(a) == f(b) for a > b, it didn't go backwards, which
isn't
> what you want to support.

Hmmm, it sounds like you're asking me to clarify the comment with a more
concrete definition of "not go backwards" (which I intended to mean the axiom
set listed in the previous comment). Do you have a suggestion for better
language here or would you prefer that I just list the axioms? :)

[xhwang, 2015-05-12 17:20:55]

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/1136513004/diff/80001/media/base/time_source....
media/base/time_source.h:55: // wall clock time does not go backwards for
monotonically increasing media
On 2015/05/12 16:50:04, DaleCurtis wrote:
> On 2015/05/12 05:45:42, xhwang wrote:
> > On 2015/05/12 05:35:42, DaleCurtis wrote:
> > > On 2015/05/12 04:46:53, xhwang wrote:
> > > > Q for "does not go backwards": Can f(a) == f(b) for a < b? 
> > > 
> > > Not unless there's a rounding error, f(a) != f(b) if a != b.
> > 
> > nit: "does not go backwards" means a >= b, which should be a > b in this
case.
> > Sorry for not being clear.
> 
> I'm still not clear on your nit here :) Within a single run, the code will
> ensure f(a) > f(b) for a > b, f(a) < (b)  for a < b, and f(a) == f(b) for a ==
> b.
> 
> Is your comment asking me to clarify that more exactly here?

Just say the wall clock time is a strictly increasing function of media
timestamps in one GetWallClockTime call?
(http://en.wikipedia.org/wiki/Monotonic_function)

[DaleCurtis, 2015-05-12 17:37:52]

Reworded here: https://codereview.chromium.org/1138283002/