Prevent seeking when Android MediaPlayer reports that it's not possible.

Prevent seeking when Android MediaPlayer reports that it's not possible.

For some content (e.g. HLS live streams), attempting to seek will get the
media player stuck in an error state. In
https://codereview.chromium.org/22605013, this was addressed by preventing
seeks if the duration of the video is negative. However, with
https://codereview.chromium.org/617743004, a -1 returned by the Android
MediaPlayer will be converted to media::kInfiniteDuration().

This CL utilizes two seek booleans derived from the Android MediaPlayer to
determine if a seek should be allowed.

BUG=510641
TEST=MediaPlayerBridgeTest.*

Committed: https://crrev.com/513ce3cd3246269db14a5dd2c328ee0b97f505aa
Cr-Commit-Position: refs/heads/master@{#339762}

[keitchen, 2015-07-16 00:05:04]

keitchen@amazon.com changed reviewers:
+ dalecurtis@chromium.org, qinmin@chromium.org

[keitchen, 2015-07-16 00:09:56]

Hi everyone,

Here's a fix for an Android media player issue when resuming HLS live streams.

There are two approaches to fixing this problem:

1. Prevent seeks if the duration of the video is media::kInfiniteDuration().
2. Prevent seeks if the can_seek booleans return false.

The Android MediaPlayer API returns -1 for videos of unknown or infinite length.
In MediaPlayerBridge both of these are converted to kInfiniteDuration().

Because of this ambiguity, I believe using explicit booleans via Option 2 is a
better solution as we don't have to make any assumptions based on the
reported duration value.

qinmin: media/base/android
dalecurtis: media/media.gyp

PTAL

Thanks,
Keith

[DaleCurtis, 2015-07-16 00:29:28]

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge.h:110: bool SeekInternal(const
base::TimeDelta& current_time, base::TimeDelta time);
Pass TimeDelta by value since it's an int64 (see time.h header)

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
File media/base/android/media_player_bridge_unittest.cc (right):

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge_unittest.cc:79: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge_unittest.cc:90: ASSERT_FALSE(
I don't suppose there's a way to intercept the JNI call instead of adding a
return value just for testing?

[keitchen, 2015-07-17 00:00:06]

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge.h:110: bool SeekInternal(const
base::TimeDelta& current_time, base::TimeDelta time);
On 2015/07/16 at 00:29:28, DaleCurtis wrote:
> Pass TimeDelta by value since it's an int64 (see time.h header)

Done.

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
File media/base/android/media_player_bridge_unittest.cc (right):

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge_unittest.cc:79: };
On 2015/07/16 at 00:29:28, DaleCurtis wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/1234483003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge_unittest.cc:90: ASSERT_FALSE(
On 2015/07/16 at 00:29:28, DaleCurtis wrote:
> I don't suppose there's a way to intercept the JNI call instead of adding a
return value just for testing?

As far as I know there isn't a way to intercept the JNI call.

[DaleCurtis, 2015-07-17 00:04:19]

dalecurtis@chromium.org changed required reviewers:
+ qinmin@chromium.org

[DaleCurtis, 2015-07-17 00:04:20]

lgtm, but defer to qinmin@ for final review

[qinmin, 2015-07-17 04:16:28]

https://codereview.chromium.org/1234483003/diff/20001/media/base/android/medi...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/1234483003/diff/20001/media/base/android/medi...
media/base/android/media_player_bridge.h:110: bool SeekInternal(base::TimeDelta
current_time, base::TimeDelta time);
the current_time can always be retrieved from getCurrentTime(), why not just
passing the seek_time in here.

Please also document the return value

[keitchen, 2015-07-20 20:24:24]

https://codereview.chromium.org/1234483003/diff/20001/media/base/android/medi...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/1234483003/diff/20001/media/base/android/medi...
media/base/android/media_player_bridge.h:110: bool SeekInternal(base::TimeDelta
current_time, base::TimeDelta time);
On 2015/07/17 at 04:16:28, qinmin wrote:
> the current_time can always be retrieved from getCurrentTime(), why not just
passing the seek_time in here.
> 
> Please also document the return value

GetCurrentTime() issues a JNI call to the media player to retrieve the current
time value. I wanted the ability to set this value when unit testing
SeekInternal().

Added documentation for the return value.

[qinmin, 2015-07-21 00:05:36]

lgtm

[keitchen, 2015-07-21 00:58:42]

The CQ bit was checked by keitchen@amazon.com

[keitchen, 2015-07-21 00:58:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1234483003/#ps40001
(title: "Addressed comments from qinmin.")

[commit-bot: I haz the power, 2015-07-21 00:58:55]

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

[commit-bot: I haz the power, 2015-07-21 01:59:06]

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

[commit-bot: I haz the power, 2015-07-21 01:59:06]

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

[keitchen, 2015-07-21 17:56:03]

The CQ bit was checked by keitchen@amazon.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-21 17:57:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234483003/40001

[commit-bot: I haz the power, 2015-07-21 18:05:52]

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

[commit-bot: I haz the power, 2015-07-21 18:05:53]

Dry run: This issue passed the CQ dry run.

[keitchen, 2015-07-21 22:17:43]

The CQ bit was checked by keitchen@amazon.com

[commit-bot: I haz the power, 2015-07-21 22:19:12]

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

[commit-bot: I haz the power, 2015-07-21 22:25:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-21 22:26:34]

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