[Android cast] Make the local player exit seeking mode on cast start

[Android cast] Make the local player exit seeking mode on cast start

When the local player thinks it's currently seeking, it skips some
tasks, including updating the progress bar. See the linked bug.
This change forces notifying the local player that seek is completed
when starting cast.

BUG=568161
Committed: https://crrev.com/ea16db5b3fde6cc976a0297f4c4adfefe35f8ca0
Cr-Commit-Position: refs/heads/master@{#369381}

[dgn, 2016-01-12 11:46:05]

dgn@chromium.org changed reviewers:
+ aberent@chromium.org

[dgn, 2016-01-12 11:46:05]

PTAL. I tracked the regressing to this
https://codereview.chromium.org/1313613003, but there are other places that do
nothing when seeking()/seeking_ is true, so it must not be the only one.

[dgn, 2016-01-12 14:36:26]

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

[dgn, 2016-01-12 14:36:26]

qinmin@chromium.org: Please review changes in webmediaplayer_android.cc

https://codereview.chromium.org/1580733002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1580733002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:777:
mLastKnownStreamPosition = 0;
mLastKnownStreamPosition to be reset, upon starting a new stream, otherwise that
position is sent back to the local player. The local player has code that makes
sure the position doesn't jump back while playing (see
https://codereview.chromium.org/1313613003) so it makes it be out of sync if a
seek backwards is made before casting.

[aberent, 2016-01-12 15:54:56]

https://codereview.chromium.org/1580733002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1580733002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:777:
mLastKnownStreamPosition = 0;
On 2016/01/12 14:36:26, dgn wrote:
> mLastKnownStreamPosition to be reset, upon starting a new stream, otherwise
that
> position is sent back to the local player. The local player has code that
makes
> sure the position doesn't jump back while playing (see
> https://codereview.chromium.org/1313613003) so it makes it be out of sync if a
> seek backwards is made before casting.

I thought we wanted the position sent back to the local player. We want it to
continue from the position where casting stopped. Have a look at
onRouteUnselectedEvent, which sets mLastKnownStreamPosition immediately before
calling clearStreamState. This will cancel that, so at least one of them must be
wrong.

https://codereview.chromium.org/1580733002/diff/20001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1580733002/diff/20001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1035:
OnSeekComplete(seek_time_);
What effect does this have on local playback, e.g. if the user presses pause
while there is a real seek in progress? What happens when the real seek
completes and calls OnSeekComplete (or does something prevent it from doing so)?

[dgn, 2016-01-13 12:25:55]

https://codereview.chromium.org/1580733002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1580733002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:777:
mLastKnownStreamPosition = 0;
On 2016/01/12 15:54:56, aberent wrote:
> On 2016/01/12 14:36:26, dgn wrote:
> > mLastKnownStreamPosition to be reset, upon starting a new stream, otherwise
> that
> > position is sent back to the local player. The local player has code that
> makes
> > sure the position doesn't jump back while playing (see
> > https://codereview.chromium.org/1313613003) so it makes it be out of sync if
a
> > seek backwards is made before casting.
> 
> I thought we wanted the position sent back to the local player. We want it to
> continue from the position where casting stopped. Have a look at
> onRouteUnselectedEvent, which sets mLastKnownStreamPosition immediately before
> calling clearStreamState. This will cancel that, so at least one of them must
be
> wrong.

Thanks I missed that it was called on disconnect too. Moved to be called only
when starting to cast out of reconnect contexts

https://codereview.chromium.org/1580733002/diff/20001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1580733002/diff/20001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1035:
OnSeekComplete(seek_time_);
On 2016/01/12 15:54:56, aberent wrote:
> What effect does this have on local playback, e.g. if the user presses pause
> while there is a real seek in progress? What happens when the real seek
> completes and calls OnSeekComplete (or does something prevent it from doing
so)?

Removed that line. Tried first to do that when switching to the remote player,
then removed it entirely to react on mediastatelistener, which come as response
to the seek request calls.

[aberent, 2016-01-13 15:53:47]

https://codereview.chromium.org/1580733002/diff/60001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1580733002/diff/60001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:172:
player_id(), base::TimeDelta::FromMilliseconds(position));
Might be better if this just called the existing onSeekComplete() (no argument)
method that already exists in this class because it derives from
AndroidMediaPlayer. Means you don't need the position argument since that gets
the position itself.

[dgn, 2016-01-13 17:05:30]

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

[dgn, 2016-01-13 17:05:42]

PTAL

https://codereview.chromium.org/1580733002/diff/60001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1580733002/diff/60001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:172:
player_id(), base::TimeDelta::FromMilliseconds(position));
On 2016/01/13 15:53:47, aberent wrote:
> Might be better if this just called the existing onSeekComplete() (no
argument)
> method that already exists in this class because it derives from
> AndroidMediaPlayer. Means you don't need the position argument since that gets
> the position itself.

Done.

[commit-bot: I haz the power, 2016-01-13 17:06:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580733002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580733002/80001

[aberent, 2016-01-13 17:09:22]

lgtm

[commit-bot: I haz the power, 2016-01-13 17:43:11]

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

[commit-bot: I haz the power, 2016-01-13 17:43:11]

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

[dgn, 2016-01-13 20:44:17]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-01-13 20:44:18]

The patchset sent to the CQ was uploaded after l-g-t-m from aberent@chromium.org
Link to the patchset: https://codereview.chromium.org/1580733002/#ps100001
(title: "Rename a function to avoid tripping the clang checker")

[commit-bot: I haz the power, 2016-01-13 20:45:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580733002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580733002/100001

[commit-bot: I haz the power, 2016-01-13 23:51:03]

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

[commit-bot: I haz the power, 2016-01-13 23:51:05]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)

[dgn, 2016-01-14 09:32:27]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-01-14 09:33:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580733002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580733002/100001

[commit-bot: I haz the power, 2016-01-14 09:37:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-14 09:38:07]

Description was changed from

==========
[Android cast] Make the local player exit seeking mode on cast start

When the local player thinks it's currently seeking, it skips some
tasks, including updating the progress bar. See the linked bug.
This change forces notifying the local player that seek is completed
when starting cast.

BUG=568161
==========

to

==========
[Android cast] Make the local player exit seeking mode on cast start

When the local player thinks it's currently seeking, it skips some
tasks, including updating the progress bar. See the linked bug.
This change forces notifying the local player that seek is completed
when starting cast.

BUG=568161

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

[commit-bot: I haz the power, 2016-01-14 09:38:08]

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