Provide fine grained media playback time thru interpolation

Provide fine grained media playback time thru interpolation

This CL includes the following changes:
1. Reduces the interval of media time update msg to 16 ms
2. Use interpolation to calculate the current playback time, and guarantee that time is always incrementing

BUG=327425
Committed: https://crrev.com/4360c7c14b1ca7e4509015166d44d1e1ad17b4bd
Cr-Commit-Position: refs/heads/master@{#294612}

[qinmin, 2014-09-05 16:48:55]

qinmin@chromium.org changed reviewers:
+ scherkus@chromium.org, xhwang@chromium.org

[qinmin, 2014-09-05 16:48:55]

PTAL

[scherkus (not reviewing), 2014-09-05 20:12:57]

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:171:
interpolator_.SetUpperBound(base::TimeDelta());
is this needed? the docs and code make it look like we'll return 0 after being
created:

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/time_de...

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:889: buffered_[0].end =
current_timestamp.InSecondsF();
it looks like this code was added in https://codereview.chromium.org/496683002
but I'm confused as to what it's supposed to be doing

if it's a local resource, buffered_[0].end should be equal to duration_ because
we should have the entire resource buffered...

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1307: if (!delegate_)
when do we not have a delegate?

if we don't have one, won't |interpolator_| go out of sync with |is_playing_|?

https://codereview.chromium.org/545993002/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/545993002/diff/1/media/media.gyp#newcode1618
media/media.gyp:1618: 'base/android/media_common_android.h',
I believe you forgot this file

[qinmin, 2014-09-05 21:32:19]

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:171:
interpolator_.SetUpperBound(base::TimeDelta());
It will return 0 only if interpolator_.StartInterpolating() is not called.
if we don't do this here, the interpolator_ will starts to return non-zero
values when user hit the play button. However, the browser side mediaplayer may
take several seconds to start rendering data. And when the first OnTimeUpdate
msg came, we may already run too far ahead.


On 2014/09/05 20:12:57, scherkus wrote:
> is this needed? the docs and code make it look like we'll return 0 after being
> created:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/time_de...

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:889: buffered_[0].end =
current_timestamp.InSecondsF();
On 2014/09/05 20:12:57, scherkus wrote:
> it looks like this code was added in https://codereview.chromium.org/496683002
> but I'm confused as to what it's supposed to be doing
> 
> if it's a local resource, buffered_[0].end should be equal to duration_
because
> we should have the entire resource buffered...

Moved this to OnMediaMetadataChanged(), feels wierd that the buffered content is
already the played content for local resources.

https://codereview.chromium.org/545993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1307: if (!delegate_)
If there is no delegate, then the WMPA instance is going away soon since the
delegate is the renderFrameImpl.
But logically we should start/stop the interpolator regardless of the
|delegate_|. Fixed.

On 2014/09/05 20:12:57, scherkus wrote:
> when do we not have a delegate?
> 
> if we don't have one, won't |interpolator_| go out of sync with |is_playing_|?

https://codereview.chromium.org/545993002/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/545993002/diff/1/media/media.gyp#newcode1618
media/media.gyp:1618: 'base/android/media_common_android.h',
Done. Forget to git add the file
On 2014/09/05 20:12:57, scherkus wrote:
> I believe you forgot this file

[qinmin, 2014-09-05 21:34:04]

Patchset #2 (id:20001) has been deleted

[scherkus (not reviewing), 2014-09-05 23:17:45]

so thinking about this more ... are we sure that simply making that change from
250 -> 16 ms isn't enough?

16 ms updates gives us updates at ~60 Hz, but since we're capping the upper
bound we interpolate to by (lower_bound + 16 ms), a web site would have to be
polling currentTime at >60 Hz to take advantage of the interpolation

https://codereview.chromium.org/545993002/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/545993002/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:763: is_playing_ =
false;
relating to UpdatePlayingState() code I previously commented on ... do we not
call delegate_->DidPause() here?

[qinmin, 2014-09-05 23:33:43]

I think if the browser process is busy or if there is a long IPC delay, the
interpolation will give us some benefit over just using 16ms intervals.

On 2014/09/05 23:17:45, scherkus wrote:
> so thinking about this more ... are we sure that simply making that change
from
> 250 -> 16 ms isn't enough?
> 
> 16 ms updates gives us updates at ~60 Hz, but since we're capping the upper
> bound we interpolate to by (lower_bound + 16 ms), a web site would have to be
> polling currentTime at >60 Hz to take advantage of the interpolation
> 
>
https://codereview.chromium.org/545993002/diff/40001/content/renderer/media/a...
> File content/renderer/media/android/webmediaplayer_android.cc (right):
> 
>
https://codereview.chromium.org/545993002/diff/40001/content/renderer/media/a...
> content/renderer/media/android/webmediaplayer_android.cc:763: is_playing_ =
> false;
> relating to UpdatePlayingState() code I previously commented on ... do we not
> call delegate_->DidPause() here?

[qinmin, 2014-09-05 23:45:09]

https://codereview.chromium.org/545993002/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/545993002/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:763: is_playing_ =
false;
On 2014/09/05 23:17:45, scherkus wrote:
> relating to UpdatePlayingState() code I previously commented on ... do we not
> call delegate_->DidPause() here?

Interesting catch here. Actually calling client_->timeChanged() here will result
in a call to WMPA::Pause() because the current time is equal to duration. That's
why we don't need to put delegate_->DidPause() here.
However, that means interpolator_.StopInterpolating() is unncessary. Removed in
this function.

[scherkus (not reviewing), 2014-09-09 21:10:38]

On 2014/09/05 23:33:43, qinmin wrote:
> I think if the browser process is busy or if there is a long IPC delay, the
> interpolation will give us some benefit over just using 16ms intervals.

The thing is that with your current code we'll only interpolate to from
[last_time_update, last_time_update + 16ms] and then stall waiting for the next
IPC, which I don't think is very helpful.

It'd almost be preferable to either:
  a) Use a high frequency and drop interpolation (tradeoff: more IPCs, more
susceptible to time stalling due to browser lagging)
  b) Use a low frequency and use interpolation (tradeoff: might not be as
accurate)

WDYT?

[qinmin, 2014-09-09 21:33:04]

On 2014/09/09 21:10:38, scherkus wrote:
> On 2014/09/05 23:33:43, qinmin wrote:
> > I think if the browser process is busy or if there is a long IPC delay, the
> > interpolation will give us some benefit over just using 16ms intervals.
> 
> The thing is that with your current code we'll only interpolate to from
> [last_time_update, last_time_update + 16ms] and then stall waiting for the
next
> IPC, which I don't think is very helpful.
> 
> It'd almost be preferable to either:
>   a) Use a high frequency and drop interpolation (tradeoff: more IPCs, more
> susceptible to time stalling due to browser lagging)
>   b) Use a low frequency and use interpolation (tradeoff: might not be as
> accurate)
> 
> WDYT?

Alright, switched the time update latency to 100 ms. Does that sounds good with
you? thanks.

[scherkus (not reviewing), 2014-09-10 19:57:31]

lgtm assuming you've tried this out ... it's better than what we have today and
I'd rather spend our collective resources trying to fix things The Right Way
(e.g., MediaCodec) than to keep iterating on this code

[qinmin, 2014-09-11 01:18:06]

On 2014/09/10 19:57:31, scherkus wrote:
> lgtm assuming you've tried this out ... it's better than what we have today
and
> I'd rather spend our collective resources trying to fix things The Right Way
> (e.g., MediaCodec) than to keep iterating on this code

Thanks, Andrew.
However, even with MediaCodec, we will still have the same issue unless we can
move decoding into the render process.

[qinmin, 2014-09-11 01:19:22]

qinmin@chromium.org changed reviewers:
+ palmer@chromium.org

[qinmin, 2014-09-11 01:19:22]

+palmer for IPC OWNER

[palmer, 2014-09-11 18:26:29]

lgtm

[qinmin, 2014-09-11 18:27:53]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:33:13]

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

[commit-bot: I haz the power, 2014-09-11 19:04:49]

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

[commit-bot: I haz the power, 2014-09-11 19:04:50]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[qinmin, 2014-09-11 21:35:58]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-09-11 22:23:53]

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

[commit-bot: I haz the power, 2014-09-11 22:31:26]

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

[commit-bot: I haz the power, 2014-09-11 22:31:28]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54936)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[qinmin, 2014-09-11 22:58:43]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-09-11 23:22:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/545993002/140001

[commit-bot: I haz the power, 2014-09-12 00:47:17]

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

[commit-bot: I haz the power, 2014-09-12 00:47:18]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54992)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59885)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[qinmin, 2014-09-12 16:45:56]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:48:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/545993002/160001

[commit-bot: I haz the power, 2014-09-12 18:01:06]

Committed patchset #8 (id:160001) as 529d12e4bd7e1e48655c858cb831e025041fd502

[commit-bot: I haz the power, 2014-09-12 18:07:28]

Patchset 8 (id:??) landed as
https://crrev.com/4360c7c14b1ca7e4509015166d44d1e1ad17b4bd
Cr-Commit-Position: refs/heads/master@{#294612}