Interpolate media time for mojo rendering pipeline.

Interpolate media time for mojo rendering pipeline.

BUG=616959
TEST=MojoRendererTest.GetMediaTime

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

[alokp, 2016-08-12 05:27:23]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 05:27:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 05:36:30]

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

[commit-bot: I haz the power, 2016-08-12 05:36:30]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alokp, 2016-08-12 05:57:18]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 05:57:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 06:07:53]

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

[commit-bot: I haz the power, 2016-08-12 06:07:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alokp, 2016-08-12 13:47:14]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 13:47:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 14:02:15]

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

[commit-bot: I haz the power, 2016-08-12 14:02:16]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alokp, 2016-08-12 16:07:53]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 16:08:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 16:26:21]

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

[commit-bot: I haz the power, 2016-08-12 16:26:22]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alokp, 2016-08-12 17:16:00]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 17:16:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 17:38:38]

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

[commit-bot: I haz the power, 2016-08-12 17:38:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alokp, 2016-08-22 19:15:24]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 19:16:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 19:59:29]

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

[commit-bot: I haz the power, 2016-08-22 19:59:30]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[alokp, 2016-08-22 20:52:47]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 20:53:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 22:55:25]

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

[commit-bot: I haz the power, 2016-08-22 22:55:25]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alokp, 2016-09-02 22:40:34]

Description was changed from

==========
Interpolate media time for mojo rendering pipeline.

BUG=616959
==========

to

==========
Interpolate media time for mojo rendering pipeline.

BUG=616959
TEST=MojoRendererTest.GetMediaTime
==========

[alokp, 2016-09-02 22:51:17]

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

[alokp, 2016-09-02 22:51:18]

https://codereview.chromium.org/2239243002/diff/220001/media/base/time_delta_...
File media/base/time_delta_interpolator.h (right):

https://codereview.chromium.org/2239243002/diff/220001/media/base/time_delta_...
media/base/time_delta_interpolator.h:54: base::TimeTicks capture_time);
Added to avoid calling TimeTicks::Now multiple times and introducing
imprecision.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer_unittest.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:248: Play();
It is illegal to call Flush when Renderer is not playing. I ran into
TimeDeltaInterpolator DCHECK here:
https://cs.chromium.org/chromium/src/media/base/time_delta_interpolator.cc?dr...

Because I unconditionally call TimeDeltaInterpolator::StopInterpolating on Flush
because I assume that it must have been started on StartPlayingFrom. This is
consistent with:
https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?dr=CSs&...

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:256: Play();
ditto.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:408: Play();
ditto

[DaleCurtis, 2016-09-06 19:21:25]

lgtm

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer_unittest.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:48: void WaitFor(base::TimeDelta
duration) {
Does RunLoop().RunUntilIdle() not suffice?

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:248: Play();
On 2016/09/02 at 22:51:18, alokp wrote:
> It is illegal to call Flush when Renderer is not playing. I ran into
TimeDeltaInterpolator DCHECK here:
>
https://cs.chromium.org/chromium/src/media/base/time_delta_interpolator.cc?dr...
> 
> Because I unconditionally call TimeDeltaInterpolator::StopInterpolating on
Flush because I assume that it must have been started on StartPlayingFrom. This
is consistent with:
>
https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?dr=CSs&...

Add DCHECK in Mojo renderer?

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:32: playback_rate_(0.0),
Can just use zero.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:198: if
(time_update_timer_.IsRunning() && (playback_rate_ > 0)) {
Drop unnecessary parens.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:200: max_time +=
base::TimeDelta::FromMilliseconds(2 * kTimeUpdateIntervalMs);
Hmm, seems like something that may be implementation specific? Add some note
about how the value was chosen.

[alokp, 2016-09-07 16:46:22]

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer_unittest.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:48: void WaitFor(base::TimeDelta
duration) {
On 2016/09/06 19:21:25, DaleCurtis wrote:
> Does RunLoop().RunUntilIdle() not suffice?

I want a few of the time-update tasks to be posted, which happen on a timer:
https://cs.chromium.org/chromium/src/media/mojo/services/mojo_renderer_servic...

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:248: Play();
On 2016/09/06 19:21:25, DaleCurtis wrote:
> On 2016/09/02 at 22:51:18, alokp wrote:
> > It is illegal to call Flush when Renderer is not playing. I ran into
> TimeDeltaInterpolator DCHECK here:
> >
>
https://cs.chromium.org/chromium/src/media/base/time_delta_interpolator.cc?dr...
> > 
> > Because I unconditionally call TimeDeltaInterpolator::StopInterpolating on
> Flush because I assume that it must have been started on StartPlayingFrom.
This
> is consistent with:
> >
>
https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?dr=CSs&...
> 
> Add DCHECK in Mojo renderer?

I was wrong about Flush being illegal. RendererImpl gets into STATE_PLAYING
after a successful initialization. StartPlayingFrom does not need to get called:
https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?dr=CSs&...

We already have a DCHECK in MojoRendererService:
https://cs.chromium.org/chromium/src/media/mojo/services/mojo_renderer_servic...

In the new patch I avoid calling StopInterpolating if the interpolator is not
interpolating to avoid the DCHECK. But I wonder if StopInterpolating should even
DCHECK. IMO it should be a no-op if it is not interpolating.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:32: playback_rate_(0.0),
On 2016/09/06 19:21:25, DaleCurtis wrote:
> Can just use zero.

Done.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:198: if
(time_update_timer_.IsRunning() && (playback_rate_ > 0)) {
On 2016/09/06 19:21:25, DaleCurtis wrote:
> Drop unnecessary parens.

Done.

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:200: max_time +=
base::TimeDelta::FromMilliseconds(2 * kTimeUpdateIntervalMs);
On 2016/09/06 19:21:25, DaleCurtis wrote:
> Hmm, seems like something that may be implementation specific? Add some note
> about how the value was chosen.

Are you referring to kTimeUpdateIntervalMs? This patch does not change that. It
is currently hardcoded to 50ms.

[alokp, 2016-09-07 18:09:48]

alokp@chromium.org changed reviewers:
+ tsepez@chromium.org

[alokp, 2016-09-07 18:09:49]

tsepez@: renderer.mojom

[Tom Sepez, 2016-09-07 20:53:54]

mojom LGTM

[DaleCurtis, 2016-09-08 16:58:23]

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:200: max_time +=
base::TimeDelta::FromMilliseconds(2 * kTimeUpdateIntervalMs);
On 2016/09/07 at 16:46:21, alokp wrote:
> On 2016/09/06 19:21:25, DaleCurtis wrote:
> > Hmm, seems like something that may be implementation specific? Add some note
> > about how the value was chosen.
> 
> Are you referring to kTimeUpdateIntervalMs? This patch does not change that.
It is currently hardcoded to 50ms.

No I meant the choice of 2*interval as the allowed wiggle room.

[alokp, 2016-09-08 17:01:42]

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:200: max_time +=
base::TimeDelta::FromMilliseconds(2 * kTimeUpdateIntervalMs);
On 2016/09/08 16:58:23, DaleCurtis wrote:
> On 2016/09/07 at 16:46:21, alokp wrote:
> > On 2016/09/06 19:21:25, DaleCurtis wrote:
> > > Hmm, seems like something that may be implementation specific? Add some
note
> > > about how the value was chosen.
> > 
> > Are you referring to kTimeUpdateIntervalMs? This patch does not change that.
> It is currently hardcoded to 50ms.
> 
> No I meant the choice of 2*interval as the allowed wiggle room.

I just used whatever was being used for the cma pipeline. Did you have any
concerns/suggestions?

[DaleCurtis, 2016-09-08 17:09:01]

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:200: max_time +=
base::TimeDelta::FromMilliseconds(2 * kTimeUpdateIntervalMs);
On 2016/09/08 at 17:01:42, alokp wrote:
> On 2016/09/08 16:58:23, DaleCurtis wrote:
> > On 2016/09/07 at 16:46:21, alokp wrote:
> > > On 2016/09/06 19:21:25, DaleCurtis wrote:
> > > > Hmm, seems like something that may be implementation specific? Add some
note
> > > > about how the value was chosen.
> > > 
> > > Are you referring to kTimeUpdateIntervalMs? This patch does not change
that.
> > It is currently hardcoded to 50ms.
> > 
> > No I meant the choice of 2*interval as the allowed wiggle room.
> 
> I just used whatever was being used for the cma pipeline. Did you have any
concerns/suggestions?

100ms just seems like a lot of wiggle room for a timer that is running every
50ms so I was looking for an explanation of how that was chosen. That means if I
send a timeupdate of 0,50 the max time can be 0,150.

We don't allow currenttime to go backwards so if there's a rendering hitch this
means time may desync significantly IIUC. I don't recall if this is used for
captions or not.

[alokp, 2016-09-08 17:36:28]

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/2239243002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.cc:200: max_time +=
base::TimeDelta::FromMilliseconds(2 * kTimeUpdateIntervalMs);
On 2016/09/08 17:09:01, DaleCurtis wrote:
> On 2016/09/08 at 17:01:42, alokp wrote:
> > On 2016/09/08 16:58:23, DaleCurtis wrote:
> > > On 2016/09/07 at 16:46:21, alokp wrote:
> > > > On 2016/09/06 19:21:25, DaleCurtis wrote:
> > > > > Hmm, seems like something that may be implementation specific? Add
some
> note
> > > > > about how the value was chosen.
> > > > 
> > > > Are you referring to kTimeUpdateIntervalMs? This patch does not change
> that.
> > > It is currently hardcoded to 50ms.
> > > 
> > > No I meant the choice of 2*interval as the allowed wiggle room.
> > 
> > I just used whatever was being used for the cma pipeline. Did you have any
> concerns/suggestions?
> 
> 100ms just seems like a lot of wiggle room for a timer that is running every
> 50ms so I was looking for an explanation of how that was chosen. That means if
I
> send a timeupdate of 0,50 the max time can be 0,150.

This is not accurate. We always send a time-update of [current_time,
current_time + 100ms]. So if the updates are firing at expected rate, we will
send [0, 100], [50, 150], and so on.

The amount of allowance should be >= kTimeUpdateIntervalMs. If you think
2*kTimeUpdateIntervalMs is too much, we can experiment with something smaller.
Ideally we should check how much decoded data is buffered by the underlying
renderer, but am not sure if it worth the trouble.

> We don't allow currenttime to go backwards so if there's a rendering hitch
this
> means time may desync significantly IIUC. I don't recall if this is used for
> captions or not.

I agree about the concern for currenttime going backward irrespective of the
amount of allowance. I added some code on the client to prevent that.

[alokp, 2016-09-08 17:37:52]

https://codereview.chromium.org/2239243002/diff/320001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2239243002/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:222: DVLOG(2) << __func__ << ": actual=" <<
media_time
We have the same logic in AudioRendererImpl. In fact I copied this code from
there. Should we move it to PipelineImpl so that they can be shared?

[DaleCurtis, 2016-09-08 17:45:29]

https://codereview.chromium.org/2239243002/diff/320001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2239243002/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:222: DVLOG(2) << __func__ << ": actual=" <<
media_time
On 2016/09/08 at 17:37:52, alokp wrote:
> We have the same logic in AudioRendererImpl. In fact I copied this code from
there. Should we move it to PipelineImpl so that they can be shared?

That seems reasonable to me.

[alokp, 2016-09-08 19:30:50]

https://codereview.chromium.org/2239243002/diff/320001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2239243002/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:222: DVLOG(2) << __func__ << ": actual=" <<
media_time
On 2016/09/08 17:45:29, DaleCurtis wrote:
> On 2016/09/08 at 17:37:52, alokp wrote:
> > We have the same logic in AudioRendererImpl. In fact I copied this code from
> there. Should we move it to PipelineImpl so that they can be shared?
> 
> That seems reasonable to me.

Done. Please check PipelineImpl and AudioRendererImpl unittests.

[DaleCurtis, 2016-09-09 17:42:22]

lgtm

[alokp, 2016-09-09 17:43:05]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-09-09 17:43:06]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2239243002/#ps340001
(title: "moves media-time clamping to PipelineImpl")

[commit-bot: I haz the power, 2016-09-09 17:43:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 21:29:58]

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

[commit-bot: I haz the power, 2016-09-09 21:30:00]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alokp, 2016-09-09 21:51:21]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-09-09 21:52:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 23:15:59]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-09-09 23:18:28]

Description was changed from

==========
Interpolate media time for mojo rendering pipeline.

BUG=616959
TEST=MojoRendererTest.GetMediaTime
==========

to

==========
Interpolate media time for mojo rendering pipeline.

BUG=616959
TEST=MojoRendererTest.GetMediaTime

Committed: https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d
Cr-Commit-Position: refs/heads/master@{#417754}
==========

[commit-bot: I haz the power, 2016-09-09 23:18:30]

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