Fix WMPI::currentTime/seek race causing flaky Blink mediasource-redundant-seek.html on desktop

Fix WMPI::currentTime/seek race causing flaky Blink mediasource-redundant-seek.html on desktop

Though repro of bug 418324 was unsuccessful locally, inspection of Blink
HTMLMediaElement's usage of WebMediaPlayer::currentTime() revealed
race between Pipeline time management and WMPI/Blink seek time
knowledge was at least partly responsible for the flakiness.

This change updates WMPI::seek() and WMPI::currentTime() implementations
with logic similar to WebMediaPlayerAndroid to:
1) eliminate some redundant seeks, and
2) reflect the best knowledge of seek target time when responding
   to currentTime() requests. This latter piece is expected to
   fix the race and layout test flakiness.

BUG=418324
TEST=no media_unittest nor blink http/tests/media/ or media/ regression locally on Linux, layout test bots show no flakiness for mediasource-redundant-seek.html

Committed: https://crrev.com/1b2ae7d307faa0d8900a6047be22b71894bba8c0
Cr-Commit-Position: refs/heads/master@{#333621}

[wolenetz, 2015-06-09 00:23:43]

wolenetz@chromium.org changed reviewers:
+ xhwang@chromium.org

[wolenetz, 2015-06-09 00:26:18]

Xiaohan: Please take a look. I don't completely understand if this will fix the
redundant-seek test flakiness, but this is a first obvious step in simplifying
the WMPI::currentTime() path to *not* consult pipeline when WMPI *knows* that it
is seeking.

philipj@: this is just FYI that I'm working on tracking down a flakiness issue
in Chromium desktop for
http/tests/media/media-source/mediasource-redundant-seek.html.

[wolenetz, 2015-06-09 00:28:38]

On 2015/06/09 00:26:18, wolenetz wrote:
> Xiaohan: Please take a look. I don't completely understand if this will fix
the
> redundant-seek test flakiness, but this is a first obvious step in simplifying
> the WMPI::currentTime() path to *not* consult pipeline when WMPI *knows* that
it
> is seeking.
> 
> philipj@: this is just FYI that I'm working on tracking down a flakiness issue
> in Chromium desktop for
> http/tests/media/media-source/mediasource-redundant-seek.html.

In particular, I suspect HTMLMediaElement::seek()'s usage of refreshCachedTime
(which ignores and possibility of an already in-progress seek) might be part of
the flaky path.

[wolenetz, 2015-06-09 16:01:40]

Looking through about half of these PS1 try bot logs for a blink web_tests that
fails mediasource-redundant-seek.html shows it is always succeeding. This
increases my confidence that this CL will mitigate the flakiness for this layout
test. Please take a look. Thanks!
Matt

[Srirama, 2015-06-09 17:08:42]

srirama.m@samsung.com changed reviewers:
+ srirama.m@samsung.com

[Srirama, 2015-06-09 17:08:43]

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:307: pending_seek_ = false;
I think this may not be required, isn't it already false in this case?

[wolenetz, 2015-06-09 19:44:28]

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:307: pending_seek_ = false;
On 2015/06/09 17:08:42, Srirama wrote:
> I think this may not be required, isn't it already false in this case?

Consider the following sequence of seeks without any intervening completion of
the pipeline seek (the key part is annotated by "**"):
(non MSE, of course, since !chunk_demuxer_ is the path in question in this
comment)
Seek(1.0s) ->
* seeking_ becomes true, seek_time_ becomes 1.0s
* pending_seek_ remains false, pending_seek_time_=n/a
Seek(2.0s) ->
* seeking_ remains true, seek_time_ remains 1.0s
* pending_seek_ becomes true, pending_seek_time_ becomes 2.0s
Seek(1.0s) -:
* seeking_ remains true, seek_time_ remains 1.0s
** we no longer need the pending seek to 2.0s, since the most recently requested
seek was to the same time that we're already seeking to)
* pending_seek_ becomes false, pending_seek_time=n/a

[wolenetz, 2015-06-09 20:14:51]

On 2015/06/09 19:44:28, wolenetz wrote:
>
https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:307: pending_seek_ = false;
> On 2015/06/09 17:08:42, Srirama wrote:
> > I think this may not be required, isn't it already false in this case?
> 
> Consider the following sequence of seeks without any intervening completion of
> the pipeline seek (the key part is annotated by "**"):
> (non MSE, of course, since !chunk_demuxer_ is the path in question in this
> comment)
> Seek(1.0s) ->
> * seeking_ becomes true, seek_time_ becomes 1.0s
> * pending_seek_ remains false, pending_seek_time_=n/a
> Seek(2.0s) ->
> * seeking_ remains true, seek_time_ remains 1.0s
> * pending_seek_ becomes true, pending_seek_time_ becomes 2.0s
> Seek(1.0s) -:
> * seeking_ remains true, seek_time_ remains 1.0s
> ** we no longer need the pending seek to 2.0s, since the most recently
requested
> seek was to the same time that we're already seeking to)
> * pending_seek_ becomes false, pending_seek_time=n/a

Note: this CL is a fixed version of https://codereview.chromium.org/24267018/,
and contains the fix for WMPI::currentTime() vs in-progress pipeline::seek()
race.
I'm much more confident about the scenario previously leading to race:
Seek(1.0) --> WMPI::seek(1.0) --> **after thread hop** pipeline::DoSeek(1.0)
seeks the renderers, etc.
Seek(3.0) --> WMPI::seek(3.0) --> (A)**after thread hop** pipeline::DoSeek(3.0)
seeks the renderers, etc.
Seek(1.0) --> WMPI::seek(1.0) --> **after thread hop** pipeline::DoSeek(1.0)
seeks the renderers, etc.

Seems ok for singleton seeks (assuming HTMLMediaElement doesn't ask about
WMPI::currentTime() until WMPI notifies the seek is done).
However, the thread hop is *not* included in WMPI::currentTime() [that consults
pipeline directly, which consults renderers directly], and WMPI::currentTime()
is used by HTMLMediaElement::seek()'s call to refreshCachedTime(). Importantly,
if the last seek(1.0) call occurs prior to (A) completing the thread hop and
seeking the renderers, then WMPI::currentTime() still shows 1.0 (from the first
seek) and HTMLMediaElement::seek() takes a short-cut "noseekrequired" logic and
skips issuing the final WMPI::seek(1.0). The result is the last seek NEVER is
issued to WMPI, and the test flakily fails.

WebMediaPlayerAndroid has had this ::currentTime() fix for quite a while (since
before I refactored the even more previously broken MediaSourcePlayer seek
behavior on Android in late 2013).
While HTMLMediaElement could intercept currentTime() requests while seeking, if
it doesn't (which obviously it currently doesn't in all cases), at a minimum the
media engine's implementation of WebMediaPlayer::currentTime() should reflect
the most recent seek time when a seek is in progress.

[xhwang, 2015-06-09 21:33:57]

lgtm with nits

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:307: pending_seek_ = false;
reset |pending_seek_time_| as well?

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.h:290: base::TimeDelta seek_time_;
Move |seek_time_| to after |seeking_|?

[wolenetz, 2015-06-09 22:09:59]

Thanks for the review!

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:307: pending_seek_ = false;
On 2015/06/09 21:33:57, xhwang wrote:
> reset |pending_seek_time_| as well?

Sure, done. |pending_seek_time_| has no meaning when |pending_seek_| is false,
but such reset shouldn't hurt or confuse. I did similar for |seek_time_| and
|seeking_|.

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1164343003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.h:290: base::TimeDelta seek_time_;
On 2015/06/09 21:33:57, xhwang wrote:
> Move |seek_time_| to after |seeking_|?

Done, with comments added, and |playback_rate_| moved closer to its comment.

[wolenetz, 2015-06-09 22:10:59]

The CQ bit was checked by wolenetz@chromium.org

[wolenetz, 2015-06-09 22:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1164343003/#ps20001
(title: "Address nits")

[commit-bot: I haz the power, 2015-06-09 22:13:56]

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

[commit-bot: I haz the power, 2015-06-10 00:44:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-10 00:46:39]

Patchset 2 (id:??) landed as
https://crrev.com/1b2ae7d307faa0d8900a6047be22b71894bba8c0
Cr-Commit-Position: refs/heads/master@{#333621}