I propose to add media::RendererClient::OnTimeUpdate, similar to the mojo pipeline and do time interpolation in PipelineImpl::GetMediaTime.

We may also want to deprecate media::Renderer::GetMediaTime. Such synchronous calls do not fit well into mojo model and having to support these kind of functions requires a complicated shim layer. Both - MojoRendererService and MojoRendererImpl - could be much simpler if not for GetMediaTime.

Another nice side-effect of asynchnously reporting time updates vs synchronously calling Renderer::GetMediaTime is that we can use media::Renderer exclusively on the media thread. No locking required in PipelineImpl.

I have a prototype in progress here: https://codereview.chromium.org/1999893004/

+dalecurtis, sandersd

Thanks for filing this bug. I am getting excited.

We had talked about this before multiple times and I think this is the right direction to do. I'll love to get rid of that synchronous call, which caused a lot of headache before.

The only issue is about the precision of the media time reported to JS. AFAICT, we have multiple sources of errors that could be introduced:
- Interpolation error. If the time update is frequent enough this should be small.
- If we always need to post the time update then there will be some delay. Again I suppose those delays will be small. Even if it's not, we can always record the post time and send it along with the time update. Then we will know definitely what the delay is, and compensate it when calculate the media time.

So the ultimate question will be, do we want to sacrifice the precision a little bit on desktop to make this change to have all the benefits described above? I feel it's definitely worth trying.

Alternatively we can have mixed modes. For example, if a Renderer supports sync GetMediaTime call, call it. Otherwise, wait for time update and do interpolation.

I would not rely on a posted task for time updates; decoding and other things block the media thread too frequently in constrained playback situations. There are use cases which rely on a fairly accurate audio time, so it'd be nice to avoid breaking those; see discussion and changes at the end of  issue 263654 .

For MojoRenderer it makes sense to use the TimeDeltaInterpolator we use in WMPA.

Dale: Thanks for pointing out the usage in WMPA. I wanted to do something similar but also send the upper bound in time-update notification.

Do you know of any stress tests I can try?

For the current time accuracy cases:
http://mcorp.no/examples/accessibility/index.html
https://bugs.chromium.org/p/chromium/issues/detail?id=263654#c62

For general sync issues, I'd try playing 60fps high-complexity content (1080p or 4K) and then checking to ensure that your time spent at the upper bound is ~0; i.e. that you always get the next timeupdate before the upper bound expires during normal playback.

Thanks Dale. I will look into those examples.

For now I have reverted the media time changes from my patch: https://codereview.chromium.org/1999893004/

It is ready for review in case you are interested :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1116967f7c15965538b42c2a402712279d4e5662

commit 1116967f7c15965538b42c2a402712279d4e5662
Author: alokp <alokp@chromium.org>
Date: Sat Jun 11 17:30:56 2016

Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread. This sets the stage to get rid of WaitableEvent
and Locks used in PipelineImpl.

This patch should not change anything functionality wise except for
timing of the notifications received from Demuxer and Renderer. In
response to the notifications we used to change member variables
inside a lock. Now we post a task to change those variables to the
main thread. The list of variables affected by this change are:
- buffered_time_ranges_
- did_loading_progress_
- duration_
- statistics_

BUG= 616959 

Review-Url: https://codereview.chromium.org/1999893004
Cr-Commit-Position: refs/heads/master@{#399376}

[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/base/pipeline_impl.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/base/pipeline_impl.h
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/base/pipeline_impl_unittest.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/filters/decoder_stream.h
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/filters/media_source_state.h
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/formats/webm/webm_stream_parser_unittest.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/test/pipeline_integration_test.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/test/pipeline_integration_test_base.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1116967f7c15965538b42c2a402712279d4e5662

commit 1116967f7c15965538b42c2a402712279d4e5662
Author: alokp <alokp@chromium.org>
Date: Sat Jun 11 17:30:56 2016

Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread. This sets the stage to get rid of WaitableEvent
and Locks used in PipelineImpl.

This patch should not change anything functionality wise except for
timing of the notifications received from Demuxer and Renderer. In
response to the notifications we used to change member variables
inside a lock. Now we post a task to change those variables to the
main thread. The list of variables affected by this change are:
- buffered_time_ranges_
- did_loading_progress_
- duration_
- statistics_

BUG= 616959 

Review-Url: https://codereview.chromium.org/1999893004
Cr-Commit-Position: refs/heads/master@{#399376}

[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/base/pipeline_impl.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/base/pipeline_impl.h
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/base/pipeline_impl_unittest.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/filters/decoder_stream.h
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/filters/media_source_state.h
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/formats/webm/webm_stream_parser_unittest.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/test/pipeline_integration_test.cc
[modify] https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662/media/test/pipeline_integration_test_base.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/805e13d4c5ebe2f1f76c5493c5ec4f44e9b3e364

commit 805e13d4c5ebe2f1f76c5493c5ec4f44e9b3e364
Author: alokp <alokp@chromium.org>
Date: Thu Jun 23 21:29:54 2016

Rearrange function definitions in the same order as declared.

During the last big refactor in r1116967, they were arranged in
a reviewer-friendly order. This patch simply arranges the
definitions in the same order as they are declared.
No functional change.

BUG= 616959 

Review-Url: https://codereview.chromium.org/2097623002
Cr-Commit-Position: refs/heads/master@{#401727}

[modify] https://crrev.com/805e13d4c5ebe2f1f76c5493c5ec4f44e9b3e364/media/base/pipeline_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb5961572eabfc98de77337a52ae4c0be2fef1ed

commit fb5961572eabfc98de77337a52ae4c0be2fef1ed
Author: alokp <alokp@chromium.org>
Date: Wed Sep 07 18:11:22 2016

Make default media::Renderer::playback_rate = 0.

BUG= 616959 

Review-Url: https://codereview.chromium.org/2305113002
Cr-Commit-Position: refs/heads/master@{#416997}

[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc
[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/chromecast/media/cma/pipeline/media_pipeline_impl.cc
[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/media/base/android/media_source_player.cc
[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/media/base/renderer.h
[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/media/base/time_delta_interpolator.cc
[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/media/base/time_delta_interpolator.h
[modify] https://crrev.com/fb5961572eabfc98de77337a52ae4c0be2fef1ed/media/base/time_delta_interpolator_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a452e2357b4cbd498bb728fb7b5f4006123d111d

commit a452e2357b4cbd498bb728fb7b5f4006123d111d
Author: alokp <alokp@chromium.org>
Date: Fri Sep 09 23:15:40 2016

Interpolate media time for mojo rendering pipeline.

BUG= 616959 
TEST=MojoRendererTest.GetMediaTime

Review-Url: https://codereview.chromium.org/2239243002
Cr-Commit-Position: refs/heads/master@{#417754}

[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/content/renderer/media/android/webmediaplayer_android.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/android/media_source_player.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/pipeline_impl.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/pipeline_impl.h
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/pipeline_impl_unittest.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/time_delta_interpolator.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/time_delta_interpolator.h
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/base/time_delta_interpolator_unittest.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/clients/mojo_renderer.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/clients/mojo_renderer.h
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/clients/mojo_renderer_unittest.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/interfaces/renderer.mojom
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/services/media_mojo_unittest.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/services/mojo_renderer_service.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/mojo/services/mojo_renderer_service.h
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/renderers/audio_renderer_impl.cc
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/renderers/audio_renderer_impl.h
[modify] https://crrev.com/a452e2357b4cbd498bb728fb7b5f4006123d111d/media/renderers/audio_renderer_impl_unittest.cc