Add support for cancellation of demuxer reads.

Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

Note: Also removes some log spam from canceled reads / tests.

BUG=165762, 638018
TEST=manual, new unittests

Committed: https://crrev.com/f299e19db62058180acf4276540670a90ff7b472
Cr-Commit-Position: refs/heads/master@{#415401}

[DaleCurtis, 2016-08-22 21:30:09]

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

[commit-bot: I haz the power, 2016-08-22 21:31:13]

Dry run: 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, 2016-08-22 22:10:42]

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

[commit-bot: I haz the power, 2016-08-22 22:10:42]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[DaleCurtis, 2016-08-22 23:39:13]

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

[commit-bot: I haz the power, 2016-08-22 23:39:48]

Dry run: 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, 2016-08-23 03:50:43]

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

[commit-bot: I haz the power, 2016-08-23 03:50:44]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2016-08-23 20:57:19]

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

[DaleCurtis, 2016-08-23 20:57:20]

Passes tests. Working on adding new ones.

[DaleCurtis, 2016-08-24 23:25:25]

Description was changed from

==========
Skip flush during media suspend; abort overlapping reads.

This speeds up suspend signficantly in the case of a hung read;
though it does not speed up a subsequent resume (we need to modify
the DataSource to support non-fatal aborts to resolve that).

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

BUG=638018
TEST=manual
==========

to

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

BUG=165762,638018
TEST=tbd
==========

[DaleCurtis, 2016-08-24 23:25:25]

dalecurtis@chromium.org changed reviewers:
+ hubbe@chromium.org, wolenetz@chromium.org

[DaleCurtis, 2016-08-24 23:25:56]

+hubbe, wolenetz for sanity check of MultiBuffer and MSE changes. Still needs
tests and perhaps some more thought on consolidation of Abort() and Stop().

[hubbe, 2016-08-24 23:39:44]

Looks reasonably sane, although the threading in multibuffer_data_source.cc
always gives me pause...

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:300: void MultibufferDataSource::Abort()
{
DCHECK() that we're on the right thread?

[DaleCurtis, 2016-08-24 23:41:10]

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:300: void MultibufferDataSource::Abort()
{
On 2016/08/24 at 23:39:43, hubbe wrote:
> DCHECK() that we're on the right thread?

Other data source operations are not DCHECK'd since they happen on the media
thread and not the |render_task_runner_|.

[hubbe, 2016-08-24 23:43:29]

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:300: void MultibufferDataSource::Abort()
{
On 2016/08/24 23:41:10, DaleCurtis wrote:
> On 2016/08/24 at 23:39:43, hubbe wrote:
> > DCHECK() that we're on the right thread?
> 
> Other data source operations are not DCHECK'd since they happen on the media
> thread and not the |render_task_runner_|.

The previous Abort() had a DCHECK(), unless you're intending to call this from
multiple different threads, I think we need to keep that.

[wolenetz, 2016-08-24 23:46:55]

https://codereview.chromium.org/2267963002/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/2267963002/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:92: // read aborted will be aborted with a status of kOk
and an EOS buffer to
Why not re-use kAborted? ISTM even with this patch set, MSE will still return
kAborted (not kOk:EOS) if the stream is still enabled, just aborted.

[DaleCurtis, 2016-08-24 23:49:41]

https://codereview.chromium.org/2267963002/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/2267963002/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:92: // read aborted will be aborted with a status of kOk
and an EOS buffer to
On 2016/08/24 at 23:46:55, wolenetz wrote:
> Why not re-use kAborted? ISTM even with this patch set, MSE will still return
kAborted (not kOk:EOS) if the stream is still enabled, just aborted.

Good point, I think this can just be kAborted without issue. I'll play around
with it, neither MSE nor SRC showed any problems.

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/2267963002/diff/40001/media/blink/multibuffer...
media/blink/multibuffer_data_source.cc:300: void MultibufferDataSource::Abort()
{
On 2016/08/24 at 23:43:29, hubbe wrote:
> On 2016/08/24 23:41:10, DaleCurtis wrote:
> > On 2016/08/24 at 23:39:43, hubbe wrote:
> > > DCHECK() that we're on the right thread?
> > 
> > Other data source operations are not DCHECK'd since they happen on the media
> > thread and not the |render_task_runner_|.
> 
> The previous Abort() had a DCHECK(), unless you're intending to call this from
multiple different threads, I think we need to keep that.

The previous Abort() method was dead code :)

[wolenetz, 2016-08-24 23:54:18]

https://codereview.chromium.org/2267963002/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/2267963002/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:92: // read aborted will be aborted with a status of kOk
and an EOS buffer to
On 2016/08/24 23:49:41, DaleCurtis wrote:
> On 2016/08/24 at 23:46:55, wolenetz wrote:
> > Why not re-use kAborted? ISTM even with this patch set, MSE will still
return
> kAborted (not kOk:EOS) if the stream is still enabled, just aborted.
> 
> Good point, I think this can just be kAborted without issue. I'll play around
> with it, neither MSE nor SRC showed any problems.

SGTM. I'll look again at this in the morning. So far, nothing in MSE portion
otherwise strikes me as problematic.

[wolenetz, 2016-08-25 18:56:14]

MSE LGTM. Potential optimization for ffmpeg multi-seek noted in comment. All %
new tests.

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

https://codereview.chromium.org/2267963002/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:315: demuxer_->AbortPendingReads();
Would this be better integrated with how the MSE path needs the abort already
via PipelineController::Dispatch()'s calls to StartWaitingForSeek() and
CancelPendingSeek() (with appropriate implementation in FFmpegDemuxer of these
two methods to do the necessary abort tracking)?

This would let rapid sequential FFmpeg seeks cancel a previous incomplete seek
to satisfy a more recent one, just like in MSE case.

Perhaps this optimization could be done in a follow-up CL.

[sandersd (OOO until July 31), 2016-08-25 19:01:37]

lgtm

[DaleCurtis, 2016-08-29 21:14:58]

dalecurtis@chromium.org changed reviewers:
+ thestig@chromium.org

[DaleCurtis, 2016-08-29 21:14:59]

+thestig for trivial media_galleries changes.

hubbe@ PTAL at multibuffer changes.
sandersd@ PTAL at ffmpegdemuxer changes.

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

https://codereview.chromium.org/2267963002/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:315: demuxer_->AbortPendingReads();
On 2016/08/25 at 18:56:14, wolenetz wrote:
> Would this be better integrated with how the MSE path needs the abort already
via PipelineController::Dispatch()'s calls to StartWaitingForSeek() and
CancelPendingSeek() (with appropriate implementation in FFmpegDemuxer of these
two methods to do the necessary abort tracking)?
> 
> This would let rapid sequential FFmpeg seeks cancel a previous incomplete seek
to satisfy a more recent one, just like in MSE case.
> 
> Perhaps this optimization could be done in a follow-up CL.

Good point. I think we just need to add AbortPendingReads() to
CancelPendingSeek() -- which is what I've done.

[Lei Zhang, 2016-08-29 21:17:32]

lgtm

[DaleCurtis, 2016-08-29 21:19:41]

Description was changed from

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

BUG=165762,638018
TEST=tbd
==========

to

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

BUG=165762,638018
TEST=manual, new unittests
==========

[DaleCurtis, 2016-08-29 21:20:07]

Description was changed from

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

BUG=165762,638018
TEST=manual, new unittests
==========

to

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

Note: Also removes some log spam from canceled reads / tests.

BUG=165762,638018
TEST=manual, new unittests
==========

[sandersd (OOO until July 31), 2016-08-29 22:40:21]

still lgtm

[DaleCurtis, 2016-08-29 23:46:12]

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

[commit-bot: I haz the power, 2016-08-30 00:21:13]

Dry run: 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, 2016-08-30 03:49:51]

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

[commit-bot: I haz the power, 2016-08-30 03:49:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hubbe, 2016-08-30 18:50:27]

lgtm

[wolenetz, 2016-08-30 18:52:12]

MSE still LGTM. ffmpeg multi-seek optimization also LGTM.

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

https://codereview.chromium.org/2267963002/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:315: demuxer_->AbortPendingReads();
On 2016/08/29 21:14:58, DaleCurtis wrote:
> On 2016/08/25 at 18:56:14, wolenetz wrote:
> > Would this be better integrated with how the MSE path needs the abort
already
> via PipelineController::Dispatch()'s calls to StartWaitingForSeek() and
> CancelPendingSeek() (with appropriate implementation in FFmpegDemuxer of these
> two methods to do the necessary abort tracking)?
> > 
> > This would let rapid sequential FFmpeg seeks cancel a previous incomplete
seek
> to satisfy a more recent one, just like in MSE case.
> > 
> > Perhaps this optimization could be done in a follow-up CL.
> 
> Good point. I think we just need to add AbortPendingReads() to
> CancelPendingSeek() -- which is what I've done.

Acknowledged.

[DaleCurtis, 2016-08-30 18:53:38]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-08-30 18:53:47]

Thanks for review!

[commit-bot: I haz the power, 2016-08-30 18:54:33]

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, 2016-08-30 19:54:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-30 19:55:59]

Description was changed from

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

Note: Also removes some log spam from canceled reads / tests.

BUG=165762,638018
TEST=manual, new unittests
==========

to

==========
Add support for cancellation of demuxer reads.

This speeds up seek and suspend significantly in the case of a hung
read; on slow networks this may shave hundreds of milliseconds or more
off the seek and suspend times.

Notably this also resolves a CHECK() crash for overlapping reads
by aborting any existing reads when a new read comes in.

Note: Also removes some log spam from canceled reads / tests.

BUG=165762,638018
TEST=manual, new unittests

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

[commit-bot: I haz the power, 2016-08-30 19:56:01]

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