Updates WebMediaPlayerAndroid and MediaSourceDelegate to respect

Fixes WebMediaPlayerAndroid to respect infinite-duration media streams.

Tested primarily on Android from the following stream:
https://www.youtube.com/tv#/watch?v=xrM34fdmloc

R=damienv@chromium.org,qinmin@chromium.org
BUG=b/13981784

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265119

[damienv1, 2014-04-16 21:57:51]

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

https://codereview.chromium.org/239743005/diff/1/content/renderer/media/andro...
content/renderer/media/android/media_source_delegate.cc:699: return
base::TimeDelta::FromSeconds(duration_seconds);
By doing that you have only a "1 second" accuracy !

[damienv1, 2014-04-16 22:00:59]

https://codereview.chromium.org/239743005/diff/1/media/base/android/demuxer_s...
File media/base/android/demuxer_stream_player_params.cc (right):

https://codereview.chromium.org/239743005/diff/1/media/base/android/demuxer_s...
media/base/android/demuxer_stream_player_params.cc:16:
duration(base::TimeDelta()) {}
Could be simply removed: duration will use the default constructor.

https://codereview.chromium.org/239743005/diff/1/media/base/android/media_sou...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/239743005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:254: configs.duration =
base::TimeDelta::FromMilliseconds(kDefaultDurationInMs);
Cleaner: change:
const base::TimeDelta kDefaultDuration = base::TimeDelta::FromMilliseconds(...);

and use kDefaultDuration here.

[gunsch, 2014-04-16 22:01:05]

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

https://codereview.chromium.org/239743005/diff/1/content/renderer/media/andro...
content/renderer/media/android/media_source_delegate.cc:699: return
base::TimeDelta::FromSeconds(duration_seconds);
On 2014/04/16 21:57:52, damienv1 wrote:
> By doing that you have only a "1 second" accuracy !

Good catch, I missed the int cast. Fixed.

[gunsch, 2014-04-16 22:05:18]

https://codereview.chromium.org/239743005/diff/1/media/base/android/demuxer_s...
File media/base/android/demuxer_stream_player_params.cc (right):

https://codereview.chromium.org/239743005/diff/1/media/base/android/demuxer_s...
media/base/android/demuxer_stream_player_params.cc:16:
duration(base::TimeDelta()) {}
On 2014/04/16 22:01:00, damienv1 wrote:
> Could be simply removed: duration will use the default constructor.

Done.

https://codereview.chromium.org/239743005/diff/1/media/base/android/media_sou...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/239743005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:254: configs.duration =
base::TimeDelta::FromMilliseconds(kDefaultDurationInMs);
On 2014/04/16 22:01:00, damienv1 wrote:
> Cleaner: change:
> const base::TimeDelta kDefaultDuration =
base::TimeDelta::FromMilliseconds(...);
> 
> and use kDefaultDuration here.

Done.

[ddorwin, 2014-04-17 03:48:57]

The BUG= entry is incorrect. I also fixed the line break in the first line of
the description.

Do we need a similar change in WMPI?

[damienv1, 2014-04-17 15:56:55]

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:694: double
duration_seconds = chunk_demuxer_->GetDuration();
nit: Naming: would just use "duration" as seconds is the official base unit of
time ;)

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:695: if
(duration_seconds == std::numeric_limits<double>::infinity()) {
nit: Should use the same convention as the one in this file:
no curly brace for single line "if" statement.

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:699: return
base::TimeDelta::FromMilliseconds(duration_seconds * 1000);
Why not using the full resolution of a TimeDelta ?
FromMicroseconds(...)

+ You could use Time::kMicrosecondsPerSecond instead of 1000*1000

[gunsch, 2014-04-17 18:05:53]

@David:

1) Hm, I thought first-line needed to be <= 70 characters long?

2) Is there a convention for referring to Google internal bugs? More details are
at b/13981784 (this was initially reported as a YouTube livestreaming issue).

3) This already works correctly in webmediaplayer_impl. The issue is that video
with infinite duration (e.g. the link in the CL description) plays correctly on
Chrome-desktop but not Chrome-Android. With this CL, it also plays on
Chrome-Android.

The failure was in two places:
A) Duration was not correctly being set
B) WMPA::maxTimeSeekable was returning 0.0 for infinite duration, which WebKit
took to mean that there was no seekable range. Therefore, "[video].currentTime =
11" triggered a HTMLMediaElement::Seek(11), which bailed out because
[video].seekable.length == 0, though video data started being queued from t=10.

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:694: double
duration_seconds = chunk_demuxer_->GetDuration();
On 2014/04/17 15:56:56, damienv1 wrote:
> nit: Naming: would just use "duration" as seconds is the official base unit of
> time ;)

Done.

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:695: if
(duration_seconds == std::numeric_limits<double>::infinity()) {
On 2014/04/17 15:56:56, damienv1 wrote:
> nit: Should use the same convention as the one in this file:
> no curly brace for single line "if" statement.

Done.

https://codereview.chromium.org/239743005/diff/30001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:699: return
base::TimeDelta::FromMilliseconds(duration_seconds * 1000);
On 2014/04/17 15:56:56, damienv1 wrote:
> Why not using the full resolution of a TimeDelta ?
> FromMicroseconds(...)
> 
> + You could use Time::kMicrosecondsPerSecond instead of 1000*1000

Better: discovered ConvertSecondsToTimestamp(double) in webmediaplayer_util
(already included)

[ddorwin, 2014-04-17 18:15:09]

On 2014/04/17 18:05:53, gunsch wrote:
> @David:
> 
> 1) Hm, I thought first-line needed to be <= 70 characters long?

Some people prefer even shorter for Git-related reasons. The reason for that is
that someone can see what the CL does without scrolling/wrapping and maybe some
tool limitations. However, in this case the first line provided no useful
details about the patch. You could rewrite that line to be more descriptive in a
shorter sentence, but truncating such that the important part is lost is not
helpful because the ability to quickly see what the patch does is lost for
everyone.

> 2) Is there a convention for referring to Google internal bugs? More details
are
> at b/13981784 (this was initially reported as a YouTube livestreaming issue).

b/ is fine.

> 3) This already works correctly in webmediaplayer_impl. The issue is that
video
> with infinite duration (e.g. the link in the CL description) plays correctly
on
> Chrome-desktop but not Chrome-Android. With this CL, it also plays on
> Chrome-Android.

Okay, thanks.

[qinmin, 2014-04-17 22:33:07]

https://codereview.chromium.org/239743005/diff/50001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/239743005/diff/50001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.h:171: base::TimeDelta
GetDuration();
base::TimeDelta GetDuration() const ?

[gunsch, 2014-04-17 22:46:50]

https://codereview.chromium.org/239743005/diff/50001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/239743005/diff/50001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.h:171: base::TimeDelta
GetDuration();
On 2014/04/17 22:33:07, qinmin wrote:
> base::TimeDelta GetDuration() const ?

Done.

[qinmin, 2014-04-17 23:03:56]

lgtm

[damienv1, 2014-04-17 23:40:35]

lgtm

[gunsch, 2014-04-18 00:09:09]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-04-18 00:11:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gunsch@chromium.org/239743005/70001

[commit-bot: I haz the power, 2014-04-18 00:50:56]

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

[commit-bot: I haz the power, 2014-04-18 00:50:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gunsch, 2014-04-18 00:51:48]

Oops, need an OWNER from content/common/media. acolwell@, ddorwin@?

[ddorwin, 2014-04-18 00:59:29]

On 2014/04/18 00:51:48, gunsch wrote:
> Oops, need an OWNER from content/common/media. acolwell@, ddorwin@?

You need a security team owner for changes to IPC messages:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/med...

[gunsch, 2014-04-18 01:04:21]

On 2014/04/18 00:59:29, ddorwin wrote:
> On 2014/04/18 00:51:48, gunsch wrote:
> > Oops, need an OWNER from content/common/media. acolwell@, ddorwin@?
> 
> You need a security team owner for changes to IPC messages:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/med...

Got it, thanks for the pointer.

[palmer, 2014-04-21 19:22:50]

IPC security LGTM.

[gunsch-google, 2014-04-21 20:08:29]

The CQ bit was checked by gunsch@google.com

[commit-bot: I haz the power, 2014-04-21 20:08:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gunsch@chromium.org/239743005/70001

[commit-bot: I haz the power, 2014-04-22 00:09:06]

Change committed as 265119