Fix race condition in media Pipeline::GetMediaTime() after seeking.

Fix race condition in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG=675556
TEST=New unit test && manually verfied bug fix.

Committed: https://crrev.com/454da202ef172f3ecbfb48c154ff97c8c10011b0
Cr-Commit-Position: refs/heads/master@{#441526}

[chcunningham, 2017-01-04 21:01:37]

chcunningham@chromium.org changed reviewers:
+ sandersd@chromium.org

[chcunningham, 2017-01-04 21:01:37]

Hey Dan, Here's change we discussed :)

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1310: pending_seek_time_ = kNoTimestamp;
Should I also be clearing this in PipelineImp::Resume()? I notice
last_media_time_ is cleared there, but its not obvious why.

[sandersd (OOO until July 31), 2017-01-04 21:10:24]

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1130: base::TimeDelta media_time =
pending_seek_time_ == kNoTimestamp
Nit: Swap order.

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1310: pending_seek_time_ = kNoTimestamp;
On 2017/01/04 21:01:37, chcunningham wrote:
> Should I also be clearing this in PipelineImp::Resume()? I notice
> last_media_time_ is cleared there, but its not obvious why.

Resume() should also set the seek time, I believe that OnSeekDone() completes
both operations.

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.h:179: base::TimeDelta pending_seek_time_;
Nit: rename to |seek_time_| to match WMPI.

[chcunningham, 2017-01-04 21:15:47]

Thanks Dan

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1130: base::TimeDelta media_time =
pending_seek_time_ == kNoTimestamp
On 2017/01/04 21:10:23, sandersd wrote:
> Nit: Swap order.

Done.

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1310: pending_seek_time_ = kNoTimestamp;
On 2017/01/04 21:10:23, sandersd wrote:
> On 2017/01/04 21:01:37, chcunningham wrote:
> > Should I also be clearing this in PipelineImp::Resume()? I notice
> > last_media_time_ is cleared there, but its not obvious why.
> 
> Resume() should also set the seek time, I believe that OnSeekDone() completes
> both operations.

Done.

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

https://codereview.chromium.org/2616703002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.h:179: base::TimeDelta pending_seek_time_;
On 2017/01/04 21:10:23, sandersd wrote:
> Nit: rename to |seek_time_| to match WMPI.

Done.

[chcunningham, 2017-01-04 21:18:32]

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

[commit-bot: I haz the power, 2017-01-04 21:18:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sandersd (OOO until July 31), 2017-01-04 21:49:26]

lgtm

https://codereview.chromium.org/2616703002/diff/60001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2616703002/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1147: DVLOG(3) << __func__ << ": " <<
media_time.InMilliseconds() << " ms";
Probably best to not set this until the seek completes, since it is conceivable
to have a renderer that starts playback earlier than the target seek time (eg.
fastSeek()).

It looks like all the current impls are monotonic starting from the seek time,
so I don't think this can reintroduce a time-going-backwards bug.

[chcunningham, 2017-01-04 22:33:47]

Thanks :)

https://codereview.chromium.org/2616703002/diff/60001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2616703002/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1147: DVLOG(3) << __func__ << ": " <<
media_time.InMilliseconds() << " ms";
On 2017/01/04 21:49:26, sandersd wrote:
> Probably best to not set this until the seek completes, since it is
conceivable
> to have a renderer that starts playback earlier than the target seek time (eg.
> fastSeek()).
> 
> It looks like all the current impls are monotonic starting from the seek time,
> so I don't think this can reintroduce a time-going-backwards bug.

Done.

[chcunningham, 2017-01-04 22:33:53]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-01-04 22:33:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2616703002/#ps80001
(title: "Feedback 2")

[commit-bot: I haz the power, 2017-01-04 22:34:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 00:28:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483569233771960,
"parent_rev": "9d267dedec9b645bfc59de5982d2e8b38f5d0a8c", "commit_rev":
"18269791f577770c3c0a25ddef378be42a364c5b"}

[commit-bot: I haz the power, 2017-01-05 00:28:59]

Description was changed from

==========
Fix race condition in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG=675556
TEST=New unit test && manually verfied bug fix.
==========

to

==========
Fix race condition in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG=675556
TEST=New unit test && manually verfied bug fix.

Review-Url: https://codereview.chromium.org/2616703002
==========

[commit-bot: I haz the power, 2017-01-05 00:29:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-05 00:31:15]

Description was changed from

==========
Fix race condition in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG=675556
TEST=New unit test && manually verfied bug fix.

Review-Url: https://codereview.chromium.org/2616703002
==========

to

==========
Fix race condition in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG=675556
TEST=New unit test && manually verfied bug fix.

Committed: https://crrev.com/454da202ef172f3ecbfb48c154ff97c8c10011b0
Cr-Commit-Position: refs/heads/master@{#441526}
==========

[commit-bot: I haz the power, 2017-01-05 00:31:16]

Patchset 5 (id:??) landed as
https://crrev.com/454da202ef172f3ecbfb48c154ff97c8c10011b0
Cr-Commit-Position: refs/heads/master@{#441526}