Change the TimeSource interface to return wallclock time for video.

Change the TimeSource interface to return wallclock time for video.

Mostly a mechanical change, though it involves extending the estimation
abilities of our TimeSource clients to provide estimates for times
outside of the current media time range.

BUG=110814, 439548
TEST=media_unittests still pass.

Committed: https://crrev.com/5755042e9569648ce2d11e18e9d46834bc7395e4
Cr-Commit-Position: refs/heads/master@{#322459}

[DaleCurtis, 2015-03-19 23:09:24]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2015-03-19 23:10:10]

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

[DaleCurtis, 2015-03-19 23:10:11]

PTAL. This is a step towards improving our video rendering path based on vsync
deadlines.  cc: scherkus in case he wants to weigh in on this code.

[xhwang, 2015-03-21 00:26:15]

On 2015/03/19 23:10:11, DaleCurtis wrote:
> PTAL. This is a step towards improving our video rendering path based on vsync
> deadlines.  cc: scherkus in case he wants to weigh in on this code.

I skimmed your CL but I need more time to think about it... sorry for the delay.

[scherkus (not reviewing), 2015-03-23 20:40:53]

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

[scherkus (not reviewing), 2015-03-23 20:40:53]

FWIW this was exactly what I had prototyped (different function names of course)
and was the eventual plan

[xhwang, 2015-03-23 22:20:14]

I like the main idea of this CL. I just have a couple of questions and some
nits.

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

https://codereview.chromium.org/1027553002/diff/20001/media/base/time_source....
media/base/time_source.h:41: virtual base::TimeDelta CurrentMediaTime() = 0;
nit: Since TimeDelta specifies a duration, we probably should make it clear
what's the base time, e.g. since the playback starts.

https://codereview.chromium.org/1027553002/diff/20001/media/base/time_source....
media/base/time_source.h:48: virtual base::TimeTicks
GetWallclockTimeForMediaTime(
s/Wallclock/WallClock/

At least in WallClockTimeSource, we treat wall and clock two separate words.

https://codereview.chromium.org/1027553002/diff/20001/media/base/time_source....
media/base/time_source.h:48: virtual base::TimeTicks
GetWallclockTimeForMediaTime(
nit: how about just GetWallClockTime(base::TimeDelta media_time) ?

https://codereview.chromium.org/1027553002/diff/20001/media/base/video_render...
File media/base/video_renderer.h (right):

https://codereview.chromium.org/1027553002/diff/20001/media/base/video_render...
media/base/video_renderer.h:27: // Used to query the current time or duration of
the media.
"time or duration" is not accurate now...

https://codereview.chromium.org/1027553002/diff/20001/media/base/video_render...
media/base/video_renderer.h:28: typedef
base::Callback<base::TimeTicks(base::TimeDelta)> TimeConverterCB;
nit: s/TimeConverterCB/WallClockTimeCB/ ?

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
File media/base/wall_clock_time_source.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
media/base/wall_clock_time_source.cc:9: #include "media/base/buffers.h"
not needed?

BTW, media/base/buffers.h should be renamed or cleaned...

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
media/base/wall_clock_time_source.cc:67: if (!ticking_ || playback_rate_ == 0.0
|| time < base_time_)
Can we do DCHECK(time >= base_time_)?

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:178: // has changed recently... Is this
actually a problem?
scherkus showed me how time could go backwards in this case. If that happens,
will we have jittering in video frame scheduling?

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:465: }
So for a audio/video content, if the video is way way behind, we will still not
trigger underflow. Is this expected?

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:234: base::TimeTicks
latest_paint_timestamp;
bikesheddings not related to your change:

|latest| can also mean "most recent", which is a bit confusing given we also
have "last_painted_timestamp_"....

We seem to use timestamp for all time variables. I feel it's a bit weird to use
"timestamp" for a future wall clock time (TimeTicks)... Also, by looking at the
implementation, it's hard to tell which is media time (TimeDelta) and which is
wall time (TimeTicks). How about we use "timestamp" for media time, and just
"time" for wall clock time?

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:281:
time_converter_cb_.Run(next_frame->timestamp());
hmm, do we allow chained assignment? I don't see it banned anywhere, but I
rarely (if ever) see it in our code base.

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:437: last_timestamp_ =
last_painted_timestamp_ = base::TimeTicks();
ditto

[DaleCurtis, 2015-03-25 00:31:45]

cc: strobe regarding his query on when we declare HAVE_NOTHING yesterday.

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

https://codereview.chromium.org/1027553002/diff/20001/media/base/time_source....
media/base/time_source.h:41: virtual base::TimeDelta CurrentMediaTime() = 0;
On 2015/03/23 22:20:13, xhwang wrote:
> nit: Since TimeDelta specifies a duration, we probably should make it clear
> what's the base time, e.g. since the playback starts.

The current media time is relative to 0?

https://codereview.chromium.org/1027553002/diff/20001/media/base/time_source....
media/base/time_source.h:48: virtual base::TimeTicks
GetWallclockTimeForMediaTime(
On 2015/03/23 22:20:13, xhwang wrote:
> s/Wallclock/WallClock/
> 
> At least in WallClockTimeSource, we treat wall and clock two separate words.

Done.

https://codereview.chromium.org/1027553002/diff/20001/media/base/video_render...
File media/base/video_renderer.h (right):

https://codereview.chromium.org/1027553002/diff/20001/media/base/video_render...
media/base/video_renderer.h:27: // Used to query the current time or duration of
the media.
On 2015/03/23 22:20:13, xhwang wrote:
> "time or duration" is not accurate now...

Done.

https://codereview.chromium.org/1027553002/diff/20001/media/base/video_render...
media/base/video_renderer.h:28: typedef
base::Callback<base::TimeTicks(base::TimeDelta)> TimeConverterCB;
On 2015/03/23 22:20:13, xhwang wrote:
> nit: s/TimeConverterCB/WallClockTimeCB/ ?

Done.

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
File media/base/wall_clock_time_source.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
media/base/wall_clock_time_source.cc:9: #include "media/base/buffers.h"
On 2015/03/23 22:20:14, xhwang wrote:
> not needed?
> 
> BTW, media/base/buffers.h should be renamed or cleaned...

Done.

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
media/base/wall_clock_time_source.cc:67: if (!ticking_ || playback_rate_ == 0.0
|| time < base_time_)
On 2015/03/23 22:20:14, xhwang wrote:
> Can we do DCHECK(time >= base_time_)?

Actually this condition was incorrect, it needs to support time < base_time_ if
playback rate changes yet the frame is still in the queue for some reason.

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:178: // has changed recently... Is this
actually a problem?
On 2015/03/23 22:20:14, xhwang wrote:
> scherkus showed me how time could go backwards in this case. If that happens,
> will we have jittering in video frame scheduling?

If someone is rapidly oscillating the playback rate then yes, scheduling could
get a bit ugly; otherwise I'd expect just a momentary glitch on some playback
rate changes, which I think is acceptable.

I've clarified the comments on TimeSource about the potential issues that can
occur if a time before the current media time is requested.

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:465: }
On 2015/03/23 22:20:14, xhwang wrote:
> So for a audio/video content, if the video is way way behind, we will still
not
> trigger underflow. Is this expected?

Correct, though in a meeting yesterday strobe@ brought up the desire to  run a
finch experiment on the "time to declare have nothing" value. +strobe to get his
thoughts on this change.

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:234: base::TimeTicks
latest_paint_timestamp;
On 2015/03/23 22:20:14, xhwang wrote:
> bikesheddings not related to your change:
> 
> |latest| can also mean "most recent", which is a bit confusing given we also
> have "last_painted_timestamp_"....
> 
> We seem to use timestamp for all time variables. I feel it's a bit weird to
use
> "timestamp" for a future wall clock time (TimeTicks)... Also, by looking at
the
> implementation, it's hard to tell which is media time (TimeDelta) and which is
> wall time (TimeTicks). How about we use "timestamp" for media time, and just
> "time" for wall clock time?

Done; great suggestion!

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:281:
time_converter_cb_.Run(next_frame->timestamp());
On 2015/03/23 22:20:14, xhwang wrote:
> hmm, do we allow chained assignment? I don't see it banned anywhere, but I
> rarely (if ever) see it in our code base.

Hmm, I tend to use it in the audio pathways, but I'm happy to change it if you
prefer otherwise.

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:437: last_timestamp_ =
last_painted_timestamp_ = base::TimeTicks();
On 2015/03/23 22:20:14, xhwang wrote:
> ditto

Acknowledged.

[xhwang, 2015-03-25 04:10:50]

lgtm % comments

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
File media/base/wall_clock_time_source.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/base/wall_clock_t...
media/base/wall_clock_time_source.cc:67: if (!ticking_ || playback_rate_ == 0.0
|| time < base_time_)
On 2015/03/25 00:31:45, DaleCurtis wrote:
> On 2015/03/23 22:20:14, xhwang wrote:
> > Can we do DCHECK(time >= base_time_)?
> 
> Actually this condition was incorrect, it needs to support time < base_time_
if
> playback rate changes yet the frame is still in the queue for some reason.

Acknowledged.

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:178: // has changed recently... Is this
actually a problem?
On 2015/03/25 00:31:45, DaleCurtis wrote:
> On 2015/03/23 22:20:14, xhwang wrote:
> > scherkus showed me how time could go backwards in this case. If that
happens,
> > will we have jittering in video frame scheduling?
> 
> If someone is rapidly oscillating the playback rate then yes, scheduling could
> get a bit ugly; otherwise I'd expect just a momentary glitch on some playback
> rate changes, which I think is acceptable.
> 
> I've clarified the comments on TimeSource about the potential issues that can
> occur if a time before the current media time is requested.

Just OCC, when time goes backwards, could we render two video frames in the
wrong order? I wonder whether there's a way to make sure wall time never goes
back when media time advances. We can chat on this offline....

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:465: }
On 2015/03/25 00:31:45, DaleCurtis wrote:
> On 2015/03/23 22:20:14, xhwang wrote:
> > So for a audio/video content, if the video is way way behind, we will still
> not
> > trigger underflow. Is this expected?
> 
> Correct, though in a meeting yesterday strobe@ brought up the desire to  run a
> finch experiment on the "time to declare have nothing" value. +strobe to get
his
> thoughts on this change.

I can think of some corner cases where we want to rebuffer when video lag
behind. For example, video decoding speed can just keep up with the speed of
playback. Then for some reason CPU gets busy and video decoding starts to fall
behind. If we don't rebuffer, it'll take long (or forever in the theoretical
case) for video to catch up. If we can declare have nothing and rebuffer, we can
easily recover. It sounds a good idea to  design some finch experiment to
measure the performance in both cases....

[DaleCurtis, 2015-03-26 18:27:30]

Patchset #3 (id:60001) has been deleted

[DaleCurtis, 2015-03-26 18:31:17]

Patchset #3 (id:80001) has been deleted

[DaleCurtis, 2015-03-26 18:32:56]

PTAL, thanks!

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1027553002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:465: }
On 2015/03/25 04:10:50, xhwang wrote:
> On 2015/03/25 00:31:45, DaleCurtis wrote:
> > On 2015/03/23 22:20:14, xhwang wrote:
> > > So for a audio/video content, if the video is way way behind, we will
still
> > not
> > > trigger underflow. Is this expected?
> > 
> > Correct, though in a meeting yesterday strobe@ brought up the desire to  run
a
> > finch experiment on the "time to declare have nothing" value. +strobe to get
> his
> > thoughts on this change.
> 
> I can think of some corner cases where we want to rebuffer when video lag
> behind. For example, video decoding speed can just keep up with the speed of
> playback. Then for some reason CPU gets busy and video decoding starts to fall
> behind. If we don't rebuffer, it'll take long (or forever in the theoretical
> case) for video to catch up. If we can declare have nothing and rebuffer, we
can
> easily recover. It sounds a good idea to  design some finch experiment to
> measure the performance in both cases....

Actually it turns out I misunderstood the code here, this is now a pure
mechanical change, we'll continue to use media time for determining underflow.

That said, per our discussions yesterday, we're okay with using wallclock time
for underflow, so I'll upload a separate CL which resolves
http://crbug.com/423801 and moves us to using wallclock time within the
RendererImpl.

[xhwang, 2015-03-26 19:24:00]

lgtm

[DaleCurtis, 2015-03-26 19:34:13]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-03-26 19:34:23]

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

[commit-bot: I haz the power, 2015-03-26 20:31:35]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-03-26 20:32:18]

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