Fix MSE garbage collection for disabled media tracks

Fix MSE garbage collection for disabled media tracks

Renderers don't read from disabled demuxer streams, thus leaving
demuxer stream read position unchanged. But MSE garbage collection
currently stops removing data from the front of buffered ranges when
it reaches the last read position. This results in QuotaExceeded
exceptions being thrown when more data is appended to the disabled
stream. The fix is to seek the disabled stream before running MSE
GC, this will update the read position to the current media_time
and should allow the GC algorithm to remove data as expected.

BUG=713423
Review-Url: https://codereview.chromium.org/2827983004
Cr-Commit-Position: refs/heads/master@{#466136}
Committed: https://chromium.googlesource.com/chromium/src/+/ac9a37e939ced07b8d5f4f74f4829013895f0cf4

[servolk, 2017-04-20 00:53:36]

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

[servolk, 2017-04-20 00:53:49]

Description was changed from

==========
Fix MSE garbage collection for disabled media tracks

Renderers don't read from disabled demuxer streams, thus leaving
demuxer stream read position unchanged. But MSE garbage collection
currently stops removing data from the front of buffered ranges when
it reaches the last read position. This results in QuotaExceeded
exceptions being thrown when more data is appended to the disabled
stream. The fix is to seek the disabled stream before running MSE
GC, this will update the read position to the current media_time
and should allow the GC algorithm to remove data as expected.

BUG=713423
==========

to

==========
Fix MSE garbage collection for disabled media tracks

Renderers don't read from disabled demuxer streams, thus leaving
demuxer stream read position unchanged. But MSE garbage collection
currently stops removing data from the front of buffered ranges when
it reaches the last read position. This results in QuotaExceeded
exceptions being thrown when more data is appended to the disabled
stream. The fix is to seek the disabled stream before running MSE
GC, this will update the read position to the current media_time
and should allow the GC algorithm to remove data as expected.

BUG=713423
==========

[servolk, 2017-04-20 00:53:50]

servolk@chromium.org changed reviewers:
+ avayvod@chromium.org, dalecurtis@chromium.org, wolenetz@chromium.org

[commit-bot: I haz the power, 2017-04-20 00:54:20]

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, 2017-04-20 02:51:47]

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

[commit-bot: I haz the power, 2017-04-20 02:51:48]

Dry run: This issue passed the CQ dry run.

[wolenetz, 2017-04-20 18:30:18]

https://codereview.chromium.org/2827983004/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/2827983004/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:133: stream_->Seek(media_time);
Interesting. I think this will work. The comment does expose a bit of the
internals of SourceBufferStream's GC implementation, though I'm unclear of a
better way, except perhaps telling the stream itself if it is enabled or
disabled and letting the GC heuristic internals in SourceBufferStream handle the
rest internally. WDYT?

[DaleCurtis, 2017-04-20 18:38:41]

patch lgtm, though defer to wolenetz@ for final review. I think it's natural for
streams to manage their own enabled/disabled state.

[wolenetz, 2017-04-20 19:01:09]

https://codereview.chromium.org/2827983004/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/2827983004/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:133: stream_->Seek(media_time);
On 2017/04/20 18:30:18, wolenetz wrote:
> Interesting. I think this will work. The comment does expose a bit of the
> internals of SourceBufferStream's GC implementation, though I'm unclear of a
> better way, except perhaps telling the stream itself if it is enabled or
> disabled and letting the GC heuristic internals in SourceBufferStream handle
the
> rest internally. WDYT?

Hmm. Actually I like the way you did this better, since the consequence of not
explicitly seeking the stream here might be harder to understand from
ChunkDemuxerStream perspective:

Since the SourceBufferStream needs to seek away from the "current read position"
if that position is stale, to avoid unnecessary retaining of the stale "current
GOP", it's better to explicitly expose the Seek intent here rather than suffer
side-effects of hidden seeks that could risk maintainability worse than this
comment's minor layer violation.

So, LGTM!

Thanks :)

[servolk, 2017-04-20 20:12:32]

https://codereview.chromium.org/2827983004/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/2827983004/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:133: stream_->Seek(media_time);
On 2017/04/20 19:01:09, wolenetz wrote:
> On 2017/04/20 18:30:18, wolenetz wrote:
> > Interesting. I think this will work. The comment does expose a bit of the
> > internals of SourceBufferStream's GC implementation, though I'm unclear of a
> > better way, except perhaps telling the stream itself if it is enabled or
> > disabled and letting the GC heuristic internals in SourceBufferStream handle
> the
> > rest internally. WDYT?
> 
> Hmm. Actually I like the way you did this better, since the consequence of not
> explicitly seeking the stream here might be harder to understand from
> ChunkDemuxerStream perspective:
> 
> Since the SourceBufferStream needs to seek away from the "current read
position"
> if that position is stale, to avoid unnecessary retaining of the stale
"current
> GOP", it's better to explicitly expose the Seek intent here rather than suffer
> side-effects of hidden seeks that could risk maintainability worse than this
> comment's minor layer violation.
> 
> So, LGTM!
> 
> Thanks :)

Thanks for diving deep into this with me, Matt :) I have considered other
options yesterday, but eventually came to the same conclusion as you - calling
Seek here seems to be the cleanest way to ensure all the corner cases are
properly handled. Glad we agree on that!

[servolk, 2017-04-20 21:15:23]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:15:59]

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-04-20 21:23:09]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492722923665490, "parent_rev":
"526500689c11b2c71e983c0e71a177a6665007bb", "commit_rev":
"ac9a37e939ced07b8d5f4f74f4829013895f0cf4"}

[commit-bot: I haz the power, 2017-04-20 21:23:20]

Description was changed from

==========
Fix MSE garbage collection for disabled media tracks

Renderers don't read from disabled demuxer streams, thus leaving
demuxer stream read position unchanged. But MSE garbage collection
currently stops removing data from the front of buffered ranges when
it reaches the last read position. This results in QuotaExceeded
exceptions being thrown when more data is appended to the disabled
stream. The fix is to seek the disabled stream before running MSE
GC, this will update the read position to the current media_time
and should allow the GC algorithm to remove data as expected.

BUG=713423
==========

to

==========
Fix MSE garbage collection for disabled media tracks

Renderers don't read from disabled demuxer streams, thus leaving
demuxer stream read position unchanged. But MSE garbage collection
currently stops removing data from the front of buffered ranges when
it reaches the last read position. This results in QuotaExceeded
exceptions being thrown when more data is appended to the disabled
stream. The fix is to seek the disabled stream before running MSE
GC, this will update the read position to the current media_time
and should allow the GC algorithm to remove data as expected.

BUG=713423

Review-Url: https://codereview.chromium.org/2827983004
Cr-Commit-Position: refs/heads/master@{#466136}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac9a37e939ced07b8d5f4f74f482...
==========

[commit-bot: I haz the power, 2017-04-20 21:23:22]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/ac9a37e939ced07b8d5f4f74f482...