Fix MSE GC, make it less aggressive, more spec-compliant

Fix MSE GC, make it less aggressive, more spec-compliant

There are a few a problems with current MSE Garbage Collection (GC from
now on):
1. It doesn't take into account current media_time as MSE spec requires
(https://codereview.chromium.org/341083004/ was supposed to fix that,
but it was never merged).  The MSE spec clearly mentions what should be
the state of the media element based on the buffered ranges (see MSE
spec 2.4.4 SourceBuffer Monitoring).  Since there is a latency between
the read position in the source buffer stream and the playback position,
the MSE garbage collection must be adjusted so as to not remove the
playback position from the buffered ranges.

2. It is too aggressive, i.e. sometimes it throws away chunks of data
from SourceBufferStream that have not been read from DemuxerStream yet,
have not been consumed by the media pipeline and many apps don't expect
that appended data will be thrown away prematurely, so they don't
attempt to fill in the gaps created by throwing away data (see
crbug.com/421694).

This CL fixes those issues by:
1. Making GC algorithm (SourceBufferStream::GarbageCollectIfNeeded)
callable from blink level via WebSourceBuffer::evictCodedFrames/
ChunkDemuxer::EvictCodedFrames.
This will allow blink to initiate GC from outside of append loop as
discussed in CL 341083004 and will allow blink to know that GC has
failed to evict enough data and throw QuotaExceededErr in that case as
prescribed by MSE spec (see step #6 in section 3.5.4 of
https://w3c.github.io/media-source/).  Related CL for blink:
https://codereview.chromium.org/1013923002/

2. Passing HTMLMediaElement::currentTime from blink level into
ChunkDemuxer::EvictCodedFrames and into MSE GC algorithm. The GC
algorithm is adjusted to stop throwing away data when the current
playback position is reached.

3. Making the GC algorithm stop when freeing data from the back of
MSE SourceBuffer and when reaching the last append position.

BUG=421694, 440173, 474806
Committed: https://crrev.com/e1f21238dfd533a3e8e5e738c134822a65a40ecf
Cr-Commit-Position: refs/heads/master@{#344908}

[servolk, 2015-03-12 18:25:13]

servolk@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org, philipj@opera.com,
wolenetz@chromium.org

[servolk, 2015-03-12 18:27:11]

On 2015/03/12 18:25:13, servolk wrote:
> mailto:servolk@chromium.org changed reviewers:
> + mailto:gunsch@chromium.org, mailto:lcwu@chromium.org,
mailto:philipj@opera.com,
> mailto:wolenetz@chromium.org

Kyle has left Google, so starting a new CL to continue work that has began as
CLs
https://codereview.chromium.org/791723003/
https://codereview.chromium.org/789983003/
https://codereview.chromium.org/794343003/

[philipj_slow, 2015-03-13 03:35:52]

Is there a Blink CL that adds WebSourceBuffer::evictFrames() and calls it to
show how this all fits together?

[servolk, 2015-03-13 17:53:20]

On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> Is there a Blink CL that adds WebSourceBuffer::evictFrames() and calls it to
> show how this all fits together?

Yes, here is Kyle's old CL for Blink:
https://codereview.chromium.org/785343002/

Although if that needs any changes I guess I'll have to create a separate CL,
since when I try to upload  new patchsets to those old CLs, I get an error
message, saying I'm not the owner of those CLs.

[philipj_slow, 2015-03-13 18:03:46]

On 2015/03/13 17:53:20, servolk wrote:
> On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> > Is there a Blink CL that adds WebSourceBuffer::evictFrames() and calls it to
> > show how this all fits together?
> 
> Yes, here is Kyle's old CL for Blink:
> https://codereview.chromium.org/785343002/
> 
> Although if that needs any changes I guess I'll have to create a separate CL,
> since when I try to upload  new patchsets to those old CLs, I get an error
> message, saying I'm not the owner of those CLs.

Yeah, it looks like you'll have to upload a new CL, since that needs a test and
probably conflicts given how old it is.

[servolk, 2015-03-13 20:35:16]

On 2015/03/13 18:03:46, philipj_UTC7 wrote:
> On 2015/03/13 17:53:20, servolk wrote:
> > On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> > > Is there a Blink CL that adds WebSourceBuffer::evictFrames() and calls it
to
> > > show how this all fits together?
> > 
> > Yes, here is Kyle's old CL for Blink:
> > https://codereview.chromium.org/785343002/
> > 
> > Although if that needs any changes I guess I'll have to create a separate
CL,
> > since when I try to upload  new patchsets to those old CLs, I get an error
> > message, saying I'm not the owner of those CLs.
> 
> Yeah, it looks like you'll have to upload a new CL, since that needs a test
and
> probably conflicts given how old it is.

There has not been a lot of changes in this area in the last couple of months
(this CL, for example, applied cleanly to the ToT), so hopefully there's no
conflicts. I'll try rebasing it to ToT just in case though.
Re tests - there are new unit tests in this CL and changes to the existing tests
to exercise the behavior being changed (less agressive GC). I can't seem to find
any existing tests to SourceBuffer in blink (under WebKit/LayoutTests), can you
point me to the the existing tests? Since this is a relatively small change, it
will probably be much easier to modify existing tests instead of adding
completely new ones.

[philipj_slow, 2015-03-15 03:45:40]

On 2015/03/13 20:35:16, servolk wrote:
> On 2015/03/13 18:03:46, philipj_UTC7 wrote:
> > On 2015/03/13 17:53:20, servolk wrote:
> > > On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> > > > Is there a Blink CL that adds WebSourceBuffer::evictFrames() and calls
it
> to
> > > > show how this all fits together?
> > > 
> > > Yes, here is Kyle's old CL for Blink:
> > > https://codereview.chromium.org/785343002/
> > > 
> > > Although if that needs any changes I guess I'll have to create a separate
> CL,
> > > since when I try to upload  new patchsets to those old CLs, I get an error
> > > message, saying I'm not the owner of those CLs.
> > 
> > Yeah, it looks like you'll have to upload a new CL, since that needs a test
> and
> > probably conflicts given how old it is.
> 
> There has not been a lot of changes in this area in the last couple of months
> (this CL, for example, applied cleanly to the ToT), so hopefully there's no
> conflicts. I'll try rebasing it to ToT just in case though.
> Re tests - there are new unit tests in this CL and changes to the existing
tests
> to exercise the behavior being changed (less agressive GC). I can't seem to
find
> any existing tests to SourceBuffer in blink (under WebKit/LayoutTests), can
you
> point me to the the existing tests? Since this is a relatively small change,
it
> will probably be much easier to modify existing tests instead of adding
> completely new ones.

The tests are in LayoutTests/http/tests/media/media-source/

The unit tests won't ensure that
http://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction is called
at the correct times per spec, which is in SourceBuffer.appendBuffer() and
SourceBuffer.appendStream().

[servolk, 2015-05-29 23:19:39]

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

[servolk, 2015-05-29 23:23:50]

On 2015/03/15 03:45:40, philipj wrote:
> On 2015/03/13 20:35:16, servolk wrote:
> > On 2015/03/13 18:03:46, philipj_UTC7 wrote:
> > > On 2015/03/13 17:53:20, servolk wrote:
> > > > On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> > > > > Is there a Blink CL that adds WebSourceBuffer::evictFrames() and calls
> it
> > to
> > > > > show how this all fits together?
> > > > 
> > > > Yes, here is Kyle's old CL for Blink:
> > > > https://codereview.chromium.org/785343002/
> > > > 
> > > > Although if that needs any changes I guess I'll have to create a
separate
> > CL,
> > > > since when I try to upload  new patchsets to those old CLs, I get an
error
> > > > message, saying I'm not the owner of those CLs.
> > > 
> > > Yeah, it looks like you'll have to upload a new CL, since that needs a
test
> > and
> > > probably conflicts given how old it is.
> > 
> > There has not been a lot of changes in this area in the last couple of
months
> > (this CL, for example, applied cleanly to the ToT), so hopefully there's no
> > conflicts. I'll try rebasing it to ToT just in case though.
> > Re tests - there are new unit tests in this CL and changes to the existing
> tests
> > to exercise the behavior being changed (less agressive GC). I can't seem to
> find
> > any existing tests to SourceBuffer in blink (under WebKit/LayoutTests), can
> you
> > point me to the the existing tests? Since this is a relatively small change,
> it
> > will probably be much easier to modify existing tests instead of adding
> > completely new ones.
> 
> The tests are in LayoutTests/http/tests/media/media-source/
> 
> The unit tests won't ensure that
> http://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction is called
> at the correct times per spec, which is in SourceBuffer.appendBuffer() and
> SourceBuffer.appendStream().

PTAL, I've updated CL description, rebased it to ToT and added basic blink
layout test to the blink CL related to this. I believe it's ready to land.

[servolk, 2015-06-02 19:21:05]

On 2015/05/29 23:23:50, servolk wrote:
> On 2015/03/15 03:45:40, philipj wrote:
> > On 2015/03/13 20:35:16, servolk wrote:
> > > On 2015/03/13 18:03:46, philipj_UTC7 wrote:
> > > > On 2015/03/13 17:53:20, servolk wrote:
> > > > > On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> > > > > > Is there a Blink CL that adds WebSourceBuffer::evictFrames() and
calls
> > it
> > > to
> > > > > > show how this all fits together?
> > > > > 
> > > > > Yes, here is Kyle's old CL for Blink:
> > > > > https://codereview.chromium.org/785343002/
> > > > > 
> > > > > Although if that needs any changes I guess I'll have to create a
> separate
> > > CL,
> > > > > since when I try to upload  new patchsets to those old CLs, I get an
> error
> > > > > message, saying I'm not the owner of those CLs.
> > > > 
> > > > Yeah, it looks like you'll have to upload a new CL, since that needs a
> test
> > > and
> > > > probably conflicts given how old it is.
> > > 
> > > There has not been a lot of changes in this area in the last couple of
> months
> > > (this CL, for example, applied cleanly to the ToT), so hopefully there's
no
> > > conflicts. I'll try rebasing it to ToT just in case though.
> > > Re tests - there are new unit tests in this CL and changes to the existing
> > tests
> > > to exercise the behavior being changed (less agressive GC). I can't seem
to
> > find
> > > any existing tests to SourceBuffer in blink (under WebKit/LayoutTests),
can
> > you
> > > point me to the the existing tests? Since this is a relatively small
change,
> > it
> > > will probably be much easier to modify existing tests instead of adding
> > > completely new ones.
> > 
> > The tests are in LayoutTests/http/tests/media/media-source/
> > 
> > The unit tests won't ensure that
> > http://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction is
called
> > at the correct times per spec, which is in SourceBuffer.appendBuffer() and
> > SourceBuffer.appendStream().
> 
> PTAL, I've updated CL description, rebased it to ToT and added basic blink
> layout test to the blink CL related to this. I believe it's ready to land.

ping

[wolenetz, 2015-06-03 18:01:54]

On 2015/06/02 19:21:05, servolk wrote:
> On 2015/05/29 23:23:50, servolk wrote:
> > On 2015/03/15 03:45:40, philipj wrote:
> > > On 2015/03/13 20:35:16, servolk wrote:
> > > > On 2015/03/13 18:03:46, philipj_UTC7 wrote:
> > > > > On 2015/03/13 17:53:20, servolk wrote:
> > > > > > On 2015/03/13 03:35:52, philipj_UTC7 wrote:
> > > > > > > Is there a Blink CL that adds WebSourceBuffer::evictFrames() and
> calls
> > > it
> > > > to
> > > > > > > show how this all fits together?
> > > > > > 
> > > > > > Yes, here is Kyle's old CL for Blink:
> > > > > > https://codereview.chromium.org/785343002/
> > > > > > 
> > > > > > Although if that needs any changes I guess I'll have to create a
> > separate
> > > > CL,
> > > > > > since when I try to upload  new patchsets to those old CLs, I get an
> > error
> > > > > > message, saying I'm not the owner of those CLs.
> > > > > 
> > > > > Yeah, it looks like you'll have to upload a new CL, since that needs a
> > test
> > > > and
> > > > > probably conflicts given how old it is.
> > > > 
> > > > There has not been a lot of changes in this area in the last couple of
> > months
> > > > (this CL, for example, applied cleanly to the ToT), so hopefully there's
> no
> > > > conflicts. I'll try rebasing it to ToT just in case though.
> > > > Re tests - there are new unit tests in this CL and changes to the
existing
> > > tests
> > > > to exercise the behavior being changed (less agressive GC). I can't seem
> to
> > > find
> > > > any existing tests to SourceBuffer in blink (under WebKit/LayoutTests),
> can
> > > you
> > > > point me to the the existing tests? Since this is a relatively small
> change,
> > > it
> > > > will probably be much easier to modify existing tests instead of adding
> > > > completely new ones.
> > > 
> > > The tests are in LayoutTests/http/tests/media/media-source/
> > > 
> > > The unit tests won't ensure that
> > > http://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction is
> called
> > > at the correct times per spec, which is in SourceBuffer.appendBuffer() and
> > > SourceBuffer.appendStream().
> > 
> > PTAL, I've updated CL description, rebased it to ToT and added basic blink
> > layout test to the blink CL related to this. I believe it's ready to land.
> 
> ping

I'll take a look today. Thanks,
Matt

[wolenetz, 2015-06-04 00:38:57]

Looking pretty good. Some nits and questions:

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:138: // |media_time| parameter should indicate
the current playback position,
nit: s/position, if/position. If/

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:388: if(audio_)
nit: spacing

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1356:
DecodeTimestamp::FromPresentationTime(get_media_time_cb_.Run());
I'm not convinced the conversion from PTS to DTS is good for out-of-order
decodes and the scenario this CL is trying to fix. Are you also trying to fix
this scenario with out-of-order decode streams (like h264)?

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:165: //   is used to ensure unconsumed data in not
garbage collected.
nit: s/in/is

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2417: NewSegmentAppend(10, 10,
&kDataA);
nice work!

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2484: // Seek to position 21.
drive-by nit: s/21/20 :)

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2491: // Append 5 buffers at
positions 40 through 44.
nit: s/40 through 44/30 through 34/

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2649: // GC should save the
keyframe before the current seek position and the data
nit: there is no seek_pending_, right? s/seek position/next buffer position/ ?

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2656: // Now fulfill the seek at
position 10. This will make GC delete the data
similar nit ditto: no seek_pending_, right?

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2679: // Seek ahead to position
16.
nit ditto: hmm. This "Seek" comment terminology kind of makes sense in this
context, just was confusing when considering there is also an explicit
SBS::Seek() operation. Replace with something like "next buffer"?

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2685: // 4 buffers in final GOP,
which is more than 3 buffers left in buffer, so GC
nit: s/4/5 ? buffers left in buffer is also confusing.

[wolenetz, 2015-06-04 00:39:00]

Looking pretty good. Some nits and questions:

[wolenetz, 2015-06-04 00:42:10]

On 2015/06/04 00:39:00, wolenetz wrote:
> Looking pretty good. Some nits and questions:

I'm not sure what happened here. See my real nits and questions in #12.

[servolk, 2015-06-04 03:03:02]

Sorry, the change to media/blink/webmediaplayer_impl.cc in the last patchset is
due to rebase, not related to this CL.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:138: // |media_time| parameter should indicate
the current playback position,
On 2015/06/04 00:38:56, wolenetz wrote:
> nit: s/position, if/position. If/

Done.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:388: if(audio_)
On 2015/06/04 00:38:56, wolenetz wrote:
> nit: spacing

Done.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1356:
DecodeTimestamp::FromPresentationTime(get_media_time_cb_.Run());
On 2015/06/04 00:38:57, wolenetz wrote:
> I'm not convinced the conversion from PTS to DTS is good for out-of-order
> decodes and the scenario this CL is trying to fix. Are you also trying to fix
> this scenario with out-of-order decode streams (like h264)?

The reason we are converting to DTS here is because buffered ranges are defined
in terms of DTS (SourceBufferRange::GetStartTimestamp/GetEndTimestamp return DTS
and most other processing in SBS seems to be in terms of DTS), but media
renderer returns current playback position as PTS. In order to figure out which
buffered range PTS belongs to, we need to compare it with range start/end DTS
values, so we need a DTS. I understand that PTS to DTS conversion doesn't make
actual adjustment for out-of-order decode streams, but I think in this case
that's fine, since we only need to figure out which buffered range playback
position belongs to. And as far as I understand single GOP always belongs
completely to some SourceBufferRange (i.e. a single GOP is never split between
two different SourceBufferRange objects). So I believe this should work
correctly both for regular in-order streams and out-of-order H.264 (or HEVC)
streams.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:165: //   is used to ensure unconsumed data in not
garbage collected.
On 2015/06/04 00:38:57, wolenetz wrote:
> nit: s/in/is

Done.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2484: // Seek to position 21.
On 2015/06/04 00:38:57, wolenetz wrote:
> drive-by nit: s/21/20 :)

Done.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2491: // Append 5 buffers at
positions 40 through 44.
On 2015/06/04 00:38:57, wolenetz wrote:
> nit: s/40 through 44/30 through 34/

Done.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2649: // GC should save the
keyframe before the current seek position and the data
On 2015/06/04 00:38:57, wolenetz wrote:
> nit: there is no seek_pending_, right? s/seek position/next buffer position/ ?

Yep, this will hit SBS::ShouldSeekToStartOfBuffered=true code path, so
seek_pending_ will be false, only selected_range will be update. I'll update the
wording in the comments.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2656: // Now fulfill the seek at
position 10. This will make GC delete the data
On 2015/06/04 00:38:57, wolenetz wrote:
> similar nit ditto: no seek_pending_, right?

Yep, see above.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2679: // Seek ahead to position
16.
On 2015/06/04 00:38:57, wolenetz wrote:
> nit ditto: hmm. This "Seek" comment terminology kind of makes sense in this
> context, just was confusing when considering there is also an explicit
> SBS::Seek() operation. Replace with something like "next buffer"?

Done.

https://codereview.chromium.org/1008463002/diff/100001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2685: // 4 buffers in final GOP,
which is more than 3 buffers left in buffer, so GC
On 2015/06/04 00:38:57, wolenetz wrote:
> nit: s/4/5 ? buffers left in buffer is also confusing.

Done.

[wolenetz, 2015-06-04 20:06:50]

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1356:
DecodeTimestamp::FromPresentationTime(get_media_time_cb_.Run());
On 2015/06/04 03:03:01, servolk wrote:
> On 2015/06/04 00:38:57, wolenetz wrote:
> > I'm not convinced the conversion from PTS to DTS is good for out-of-order
> > decodes and the scenario this CL is trying to fix. Are you also trying to
fix
> > this scenario with out-of-order decode streams (like h264)?
> 
> The reason we are converting to DTS here is because buffered ranges are
defined
> in terms of DTS (SourceBufferRange::GetStartTimestamp/GetEndTimestamp return
DTS
> and most other processing in SBS seems to be in terms of DTS), but media
> renderer returns current playback position as PTS. In order to figure out
which
> buffered range PTS belongs to, we need to compare it with range start/end DTS
> values, so we need a DTS. I understand that PTS to DTS conversion doesn't make
> actual adjustment for out-of-order decode streams, but I think in this case
> that's fine, since we only need to figure out which buffered range playback
> position belongs to. And as far as I understand single GOP always belongs
> completely to some SourceBufferRange (i.e. a single GOP is never split between
> two different SourceBufferRange objects). So I believe this should work
> correctly both for regular in-order streams and out-of-order H.264 (or HEVC)
> streams.

Ok. Thanks for considering this detail. As FYI, the incorrect usage of DTS
instead of PTS in older parts of Chromium MSE impl such as SourceBufferStream is
a known issue and tracked at least bug 373039 (plus bug 398130 and bug 402502).
Hence my original question since I didn't want this CL to regress behavior
further.

https://codereview.chromium.org/1008463002/diff/140001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/140001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:3440: // create gaps in source buffer
stream. This could break playback for many
nit: s/This/Gaps/

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2720:
DecodeTimestamp::FromMilliseconds(0)));
Hah! Great catch to add this test. I previously missed that no other SBSTests
were testing a non-kNoDecodeTimestamp() media_time parameter to SBS::GC.

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2748: // last second and we are
back under memory limit of 30 buffers, so GCInNeeded
nit: s/GCInNeeded/GCIfNeeded/

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2759: }
Is it ever possible for media_time (PTS converted directly to DTS) to no longer
be buffered? Perhaps the renderer clock could have advanced slightly beyond last
buffered frame's DTS+duration (an out-of-order decode stream's media_time (in
PTS) could be just past the last buffered frame's DTS (+duration).
To concretize a bit, what is expected behavior if line 2759 added
EXPECT_????(stream_->GarbageCollectIfNeeded(DecodeTimestamp::FromMilliseconds(10000
or 10001)); CheckExpectedRanges( ???? ); ?

[servolk, 2015-06-04 20:22:47]

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/100001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1356:
DecodeTimestamp::FromPresentationTime(get_media_time_cb_.Run());
On 2015/06/04 20:06:50, wolenetz wrote:
> On 2015/06/04 03:03:01, servolk wrote:
> > On 2015/06/04 00:38:57, wolenetz wrote:
> > > I'm not convinced the conversion from PTS to DTS is good for out-of-order
> > > decodes and the scenario this CL is trying to fix. Are you also trying to
> fix
> > > this scenario with out-of-order decode streams (like h264)?
> > 
> > The reason we are converting to DTS here is because buffered ranges are
> defined
> > in terms of DTS (SourceBufferRange::GetStartTimestamp/GetEndTimestamp return
> DTS
> > and most other processing in SBS seems to be in terms of DTS), but media
> > renderer returns current playback position as PTS. In order to figure out
> which
> > buffered range PTS belongs to, we need to compare it with range start/end
DTS
> > values, so we need a DTS. I understand that PTS to DTS conversion doesn't
make
> > actual adjustment for out-of-order decode streams, but I think in this case
> > that's fine, since we only need to figure out which buffered range playback
> > position belongs to. And as far as I understand single GOP always belongs
> > completely to some SourceBufferRange (i.e. a single GOP is never split
between
> > two different SourceBufferRange objects). So I believe this should work
> > correctly both for regular in-order streams and out-of-order H.264 (or HEVC)
> > streams.
> 
> Ok. Thanks for considering this detail. As FYI, the incorrect usage of DTS
> instead of PTS in older parts of Chromium MSE impl such as SourceBufferStream
is
> a known issue and tracked at least bug 373039 (plus bug 398130 and bug
402502).
> Hence my original question since I didn't want this CL to regress behavior
> further.

Acknowledged.

https://codereview.chromium.org/1008463002/diff/140001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/140001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:3440: // create gaps in source buffer
stream. This could break playback for many
On 2015/06/04 20:06:50, wolenetz wrote:
> nit: s/This/Gaps/

Done.

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2720:
DecodeTimestamp::FromMilliseconds(0)));
On 2015/06/04 20:06:50, wolenetz wrote:
> Hah! Great catch to add this test. I previously missed that no other SBSTests
> were testing a non-kNoDecodeTimestamp() media_time parameter to SBS::GC.

Acknowledged.

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2748: // last second and we are
back under memory limit of 30 buffers, so GCInNeeded
On 2015/06/04 20:06:50, wolenetz wrote:
> nit: s/GCInNeeded/GCIfNeeded/

Done.

https://codereview.chromium.org/1008463002/diff/140001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2759: }
On 2015/06/04 20:06:50, wolenetz wrote:
> Is it ever possible for media_time (PTS converted directly to DTS) to no
longer
> be buffered? Perhaps the renderer clock could have advanced slightly beyond
last
> buffered frame's DTS+duration (an out-of-order decode stream's media_time (in
> PTS) could be just past the last buffered frame's DTS (+duration).
> To concretize a bit, what is expected behavior if line 2759 added
>
EXPECT_????(stream_->GarbageCollectIfNeeded(DecodeTimestamp::FromMilliseconds(10000
> or 10001)); CheckExpectedRanges( ???? ); ?

This should not happen during regular playback, since media_time is always
supposed to be in some buffered range. But it should still not cause any
problems to GC algorithm, it should not crash/fail, so I've added a test case to
cover this as well.

[wolenetz, 2015-06-04 20:42:06]

LGTM

[servolk, 2015-06-04 20:43:08]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2015-06-04 20:44:33]

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

[commit-bot: I haz the power, 2015-06-04 21:04:41]

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

[commit-bot: I haz the power, 2015-06-04 21:04:43]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[wolenetz, 2015-06-04 21:31:04]

It looks like the android bots are complaining because the Android
media-source-player fork of the media pipeline has separate construction of
ChunkDemuxer that needs updating based on the rest of this CL.

[servolk, 2015-06-04 21:42:03]

On 2015/06/04 21:31:04, wolenetz wrote:
> It looks like the android bots are complaining because the Android
> media-source-player fork of the media pipeline has separate construction of
> ChunkDemuxer that needs updating based on the rest of this CL.

Yeah, I'm looking into this. Unfortunately it looks like WebMediaPlayerAndroid
is not using media::Pipeline object, and WebMediaPlayerAndroid::currentTime
wants to be called only on main thread, which is probably too strong of a
limitation for ChunkDemuxer.
But I think we can either cache current_timestamp values in
WebMediaPlayerAndroid::OnTimeUpdate or pass OnTimeUpdate events to
media_source_delegate_. We don't need super-precise time for GC, all we need to
know for GC is 'all media data up to this position is guaranteed to be consumed
already', so we can probably use the current_timestamp given to
WebMediaPlayerAndroid::OnTimeUpdate without doing any adjustments for IPC delay,
etc.

[wolenetz, 2015-06-04 21:57:30]

On 2015/06/04 21:42:03, servolk wrote:
> On 2015/06/04 21:31:04, wolenetz wrote:
> > It looks like the android bots are complaining because the Android
> > media-source-player fork of the media pipeline has separate construction of
> > ChunkDemuxer that needs updating based on the rest of this CL.
> 
> Yeah, I'm looking into this. Unfortunately it looks like WebMediaPlayerAndroid
> is not using media::Pipeline object, and WebMediaPlayerAndroid::currentTime
> wants to be called only on main thread, which is probably too strong of a
> limitation for ChunkDemuxer.
> But I think we can either cache current_timestamp values in
> WebMediaPlayerAndroid::OnTimeUpdate or pass OnTimeUpdate events to
> media_source_delegate_. We don't need super-precise time for GC, all we need
to
> know for GC is 'all media data up to this position is guaranteed to be
consumed
> already', so we can probably use the current_timestamp given to
> WebMediaPlayerAndroid::OnTimeUpdate without doing any adjustments for IPC
delay,
> etc.

Interesting. What, if any, consideration needs to be made if the player is in
the middle of processing a seek request (whereby the currentTime visible to the
web app is the most recently seeked-to time)?

[servolk, 2015-06-04 22:12:40]

On 2015/06/04 21:57:30, wolenetz wrote:
> On 2015/06/04 21:42:03, servolk wrote:
> > On 2015/06/04 21:31:04, wolenetz wrote:
> > > It looks like the android bots are complaining because the Android
> > > media-source-player fork of the media pipeline has separate construction
of
> > > ChunkDemuxer that needs updating based on the rest of this CL.
> > 
> > Yeah, I'm looking into this. Unfortunately it looks like
WebMediaPlayerAndroid
> > is not using media::Pipeline object, and WebMediaPlayerAndroid::currentTime
> > wants to be called only on main thread, which is probably too strong of a
> > limitation for ChunkDemuxer.
> > But I think we can either cache current_timestamp values in
> > WebMediaPlayerAndroid::OnTimeUpdate or pass OnTimeUpdate events to
> > media_source_delegate_. We don't need super-precise time for GC, all we need
> to
> > know for GC is 'all media data up to this position is guaranteed to be
> consumed
> > already', so we can probably use the current_timestamp given to
> > WebMediaPlayerAndroid::OnTimeUpdate without doing any adjustments for IPC
> delay,
> > etc.
> 
> Interesting. What, if any, consideration needs to be made if the player is in
> the middle of processing a seek request (whereby the currentTime visible to
the
> web app is the most recently seeked-to time)?

That's a very good point, yes, seeking is somewhat special case, since as soon
as seeking starts, or if there is a pending seek, we should prioritize saving
data around new seek position during GC, and throw away data from the old
playback position, it's not useful anymore. So in case of WebMediaPlayerAndroid
we probably still want to use currentTime, only perhaps introduce
currentTimeInternal callable from any thread. In case of WebMediaPlayerImpl, if
there is a pending seek, we should use the new seek position given to us through
ChunkDemuxer::StartWaitingForSeek, instead of the old/current playback position
obtained from media::Renderer. I'll try to make those adjustments.

[servolk, 2015-06-05 22:02:20]

On 2015/06/04 22:12:40, servolk wrote:
> On 2015/06/04 21:57:30, wolenetz wrote:
> > On 2015/06/04 21:42:03, servolk wrote:
> > > On 2015/06/04 21:31:04, wolenetz wrote:
> > > > It looks like the android bots are complaining because the Android
> > > > media-source-player fork of the media pipeline has separate construction
> of
> > > > ChunkDemuxer that needs updating based on the rest of this CL.
> > > 
> > > Yeah, I'm looking into this. Unfortunately it looks like
> WebMediaPlayerAndroid
> > > is not using media::Pipeline object, and
WebMediaPlayerAndroid::currentTime
> > > wants to be called only on main thread, which is probably too strong of a
> > > limitation for ChunkDemuxer.
> > > But I think we can either cache current_timestamp values in
> > > WebMediaPlayerAndroid::OnTimeUpdate or pass OnTimeUpdate events to
> > > media_source_delegate_. We don't need super-precise time for GC, all we
need
> > to
> > > know for GC is 'all media data up to this position is guaranteed to be
> > consumed
> > > already', so we can probably use the current_timestamp given to
> > > WebMediaPlayerAndroid::OnTimeUpdate without doing any adjustments for IPC
> > delay,
> > > etc.
> > 
> > Interesting. What, if any, consideration needs to be made if the player is
in
> > the middle of processing a seek request (whereby the currentTime visible to
> the
> > web app is the most recently seeked-to time)?
> 
> That's a very good point, yes, seeking is somewhat special case, since as soon
> as seeking starts, or if there is a pending seek, we should prioritize saving
> data around new seek position during GC, and throw away data from the old
> playback position, it's not useful anymore. So in case of
WebMediaPlayerAndroid
> we probably still want to use currentTime, only perhaps introduce
> currentTimeInternal callable from any thread. In case of WebMediaPlayerImpl,
if
> there is a pending seek, we should use the new seek position given to us
through
> ChunkDemuxer::StartWaitingForSeek, instead of the old/current playback
position
> obtained from media::Renderer. I'll try to make those adjustments.

FYI: After thinking some more about this (in particular what would be the proper
way to handle GC after seeking) and reading ChunkDemuxer/ChunkDemuxerStream code
I think we might be able to implement proper GC without relying on the current
media position as reported by media::Pipeline or WebMediaPlayer. ChunkDemuxer
should already have enough information to figure out what data has been consumed
(since ChunkDemuxer owns all ChunkDemuxerStreams and each ChunkDemuxerStream
knows exact timestamp of each data buffer returned from the Read operation, i.e.
ChunkDemuxerStream should know precisely which data has already been read and
consumed by the media pipeline, and that's exactly the kind of information that
we need to take into account during GC to ensure we don't throw away some
unconsumed data prematurely. ChunkDemuxer also gets notified about upcoming seek
operations via ChunkDemuxer::StartWaitingForSeek, so it should be able to decide
which data to keep without relying on external time sources.

[wolenetz, 2015-06-05 22:41:24]

On 2015/06/05 22:02:20, servolk wrote:
> On 2015/06/04 22:12:40, servolk wrote:
> > On 2015/06/04 21:57:30, wolenetz wrote:
> > > On 2015/06/04 21:42:03, servolk wrote:
> > > > On 2015/06/04 21:31:04, wolenetz wrote:
> > > > > It looks like the android bots are complaining because the Android
> > > > > media-source-player fork of the media pipeline has separate
construction
> > of
> > > > > ChunkDemuxer that needs updating based on the rest of this CL.
> > > > 
> > > > Yeah, I'm looking into this. Unfortunately it looks like
> > WebMediaPlayerAndroid
> > > > is not using media::Pipeline object, and
> WebMediaPlayerAndroid::currentTime
> > > > wants to be called only on main thread, which is probably too strong of
a
> > > > limitation for ChunkDemuxer.
> > > > But I think we can either cache current_timestamp values in
> > > > WebMediaPlayerAndroid::OnTimeUpdate or pass OnTimeUpdate events to
> > > > media_source_delegate_. We don't need super-precise time for GC, all we
> need
> > > to
> > > > know for GC is 'all media data up to this position is guaranteed to be
> > > consumed
> > > > already', so we can probably use the current_timestamp given to
> > > > WebMediaPlayerAndroid::OnTimeUpdate without doing any adjustments for
IPC
> > > delay,
> > > > etc.
> > > 
> > > Interesting. What, if any, consideration needs to be made if the player is
> in
> > > the middle of processing a seek request (whereby the currentTime visible
to
> > the
> > > web app is the most recently seeked-to time)?
> > 
> > That's a very good point, yes, seeking is somewhat special case, since as
soon
> > as seeking starts, or if there is a pending seek, we should prioritize
saving
> > data around new seek position during GC, and throw away data from the old
> > playback position, it's not useful anymore. So in case of
> WebMediaPlayerAndroid
> > we probably still want to use currentTime, only perhaps introduce
> > currentTimeInternal callable from any thread. In case of WebMediaPlayerImpl,
> if
> > there is a pending seek, we should use the new seek position given to us
> through
> > ChunkDemuxer::StartWaitingForSeek, instead of the old/current playback
> position
> > obtained from media::Renderer. I'll try to make those adjustments.
> 
> FYI: After thinking some more about this (in particular what would be the
proper
> way to handle GC after seeking) and reading ChunkDemuxer/ChunkDemuxerStream
code
> I think we might be able to implement proper GC without relying on the current
> media position as reported by media::Pipeline or WebMediaPlayer. ChunkDemuxer
> should already have enough information to figure out what data has been
consumed
> (since ChunkDemuxer owns all ChunkDemuxerStreams and each ChunkDemuxerStream
> knows exact timestamp of each data buffer returned from the Read operation,
i.e.
> ChunkDemuxerStream should know precisely which data has already been read and
> consumed by the media pipeline, and that's exactly the kind of information
that
> we need to take into account during GC to ensure we don't throw away some
> unconsumed data prematurely. ChunkDemuxer also gets notified about upcoming
seek
> operations via ChunkDemuxer::StartWaitingForSeek, so it should be able to
decide
> which data to keep without relying on external time sources.

You're absolutely correct. ChunkDemuxer knows the every stream backing a
MediaSource object, and every stream knows their buffered ranges and last
appended info and next read and last read info, and ChunkDemuxer also knows
precisely the seek information when seeking is triggered. What ChunkDemuxer
*doesn't* know is the currentTime of the renderers; due to arbitrary pipeline
depths on various platforms, there could be some large latency between a buffer
being read out of a ChunkDemuxerStream and that buffer's decoded contents being
rendered such that currentTime has advanced and reached the time of that buffer.

aside: I was considering for a moment earlier this week that this CL could
eliminate the hacky "BrowserSeek" on Chrome for Android. That's still a
possibility, but it depends not necessarily on this CL, rather on the
MediaSourcePlayer/MediaDecoderJob (or upcoming MediaCodecPlayer/etc) in the
Browser process caching buffers since the last keyframe, inclusive, to be able
to restart the decoder. However, such caching, especially on mobile, is
problematic in terms of memory consumption, especially if SBS is *also* holding
those buffers until GC. So, for now, BrowserSeek will probably remain.

Lots of detail here, I know. In short, can GC be done correctly without knowing
the actual renderer currentTime, but instead just the info known by ChunkDemuxer
and its ChunkDemuxerStreams/SourceBufferStreams? If so, that approach sounds
MUCH cleaner since it doesn't depend on renderer time management (and thanks for
rethinking!)

[servolk, 2015-06-15 18:23:08]

servolk@chromium.org changed reviewers:
+ damienv@chromium.org

[servolk, 2015-06-15 18:24:57]

On 2015/06/05 22:41:24, wolenetz wrote:
> On 2015/06/05 22:02:20, servolk wrote:
> > On 2015/06/04 22:12:40, servolk wrote:
> > > On 2015/06/04 21:57:30, wolenetz wrote:
> > > > On 2015/06/04 21:42:03, servolk wrote:
> > > > > On 2015/06/04 21:31:04, wolenetz wrote:
> > > > > > It looks like the android bots are complaining because the Android
> > > > > > media-source-player fork of the media pipeline has separate
> construction
> > > of
> > > > > > ChunkDemuxer that needs updating based on the rest of this CL.
> > > > > 
> > > > > Yeah, I'm looking into this. Unfortunately it looks like
> > > WebMediaPlayerAndroid
> > > > > is not using media::Pipeline object, and
> > WebMediaPlayerAndroid::currentTime
> > > > > wants to be called only on main thread, which is probably too strong
of
> a
> > > > > limitation for ChunkDemuxer.
> > > > > But I think we can either cache current_timestamp values in
> > > > > WebMediaPlayerAndroid::OnTimeUpdate or pass OnTimeUpdate events to
> > > > > media_source_delegate_. We don't need super-precise time for GC, all
we
> > need
> > > > to
> > > > > know for GC is 'all media data up to this position is guaranteed to be
> > > > consumed
> > > > > already', so we can probably use the current_timestamp given to
> > > > > WebMediaPlayerAndroid::OnTimeUpdate without doing any adjustments for
> IPC
> > > > delay,
> > > > > etc.
> > > > 
> > > > Interesting. What, if any, consideration needs to be made if the player
is
> > in
> > > > the middle of processing a seek request (whereby the currentTime visible
> to
> > > the
> > > > web app is the most recently seeked-to time)?
> > > 
> > > That's a very good point, yes, seeking is somewhat special case, since as
> soon
> > > as seeking starts, or if there is a pending seek, we should prioritize
> saving
> > > data around new seek position during GC, and throw away data from the old
> > > playback position, it's not useful anymore. So in case of
> > WebMediaPlayerAndroid
> > > we probably still want to use currentTime, only perhaps introduce
> > > currentTimeInternal callable from any thread. In case of
WebMediaPlayerImpl,
> > if
> > > there is a pending seek, we should use the new seek position given to us
> > through
> > > ChunkDemuxer::StartWaitingForSeek, instead of the old/current playback
> > position
> > > obtained from media::Renderer. I'll try to make those adjustments.
> > 
> > FYI: After thinking some more about this (in particular what would be the
> proper
> > way to handle GC after seeking) and reading ChunkDemuxer/ChunkDemuxerStream
> code
> > I think we might be able to implement proper GC without relying on the
current
> > media position as reported by media::Pipeline or WebMediaPlayer.
ChunkDemuxer
> > should already have enough information to figure out what data has been
> consumed
> > (since ChunkDemuxer owns all ChunkDemuxerStreams and each ChunkDemuxerStream
> > knows exact timestamp of each data buffer returned from the Read operation,
> i.e.
> > ChunkDemuxerStream should know precisely which data has already been read
and
> > consumed by the media pipeline, and that's exactly the kind of information
> that
> > we need to take into account during GC to ensure we don't throw away some
> > unconsumed data prematurely. ChunkDemuxer also gets notified about upcoming
> seek
> > operations via ChunkDemuxer::StartWaitingForSeek, so it should be able to
> decide
> > which data to keep without relying on external time sources.
> 
> You're absolutely correct. ChunkDemuxer knows the every stream backing a
> MediaSource object, and every stream knows their buffered ranges and last
> appended info and next read and last read info, and ChunkDemuxer also knows
> precisely the seek information when seeking is triggered. What ChunkDemuxer
> *doesn't* know is the currentTime of the renderers; due to arbitrary pipeline
> depths on various platforms, there could be some large latency between a
buffer
> being read out of a ChunkDemuxerStream and that buffer's decoded contents
being
> rendered such that currentTime has advanced and reached the time of that
buffer.
> 
> aside: I was considering for a moment earlier this week that this CL could
> eliminate the hacky "BrowserSeek" on Chrome for Android. That's still a
> possibility, but it depends not necessarily on this CL, rather on the
> MediaSourcePlayer/MediaDecoderJob (or upcoming MediaCodecPlayer/etc) in the
> Browser process caching buffers since the last keyframe, inclusive, to be able
> to restart the decoder. However, such caching, especially on mobile, is
> problematic in terms of memory consumption, especially if SBS is *also*
holding
> those buffers until GC. So, for now, BrowserSeek will probably remain.
> 
> Lots of detail here, I know. In short, can GC be done correctly without
knowing
> the actual renderer currentTime, but instead just the info known by
ChunkDemuxer
> and its ChunkDemuxerStreams/SourceBufferStreams? If so, that approach sounds
> MUCH cleaner since it doesn't depend on renderer time management (and thanks
for
> rethinking!)

Sorry for the delay, you've posed a very good question and I spent some time
researching and pondering the answer. When I made my previous comment about
using only demuxer read position for GC, my reasoning was that demuxer reads are
linear (i.e. if DemuxerStream::Read at some point has returned a buffer with
dts=t0, then all subsequent reads are going to return buffers with dts > t0, if
there was no seeking in between the reads), and so purely from
Demuxer/DemuxerStream point of view it should be safe to use demuxer read
position for GC, regardless of the medie pipeline depth. BUT I was also
discussing this with Damien and he brought up a very good point that even though
it's safe and correct from DemuxerStream point of view to keep only unread
buffers during GC, we still need to use currentTime for GC. The reason is that
Javascript player is not aware of media pipeline internals, all it sees is
currentTime and buffered ranges, and so if we GC some buffers after currentTime
position, JS player might notice that currentTime position has no buffered data
and might try to pause playback and/or rebuffer the data for the currentTime
position, even though these buffers have been already read from DemuxerStream
and are undergoing processing somewhere in the internals of media pipeline.
So I partially retract my previous comment. We'll still need to use
WebMediaPlayer::currentTime, i.e. position visible to Javascript player, for
determining which buffers we need to keep during garbage collection. I will
update the current CL to use WebMediaPlayer::currentTime instead of
media::Pipeline::GetMediaTime that it currently uses (and I'll make sure we are
also using currentTime for GC with WebMediaPlayerAndroid too).

[servolk, 2015-06-26 18:14:01]

Please take another look.
I've reworked this CL a bit to pass the current playback position
(HTMLMediaElement::currentTime) from blink level, instead of providing a
separate get_media_time_cb to ChunkDemuxer. This allows us to avoid changing
public ChunkDemuxer interface and therefore will work identically on all
platforms, including Android.
Also, since we are now using HTMLMediaElement::currentTime, that changes to the
new seek position as soon as seek is started, we'll get better at protecting
newly appended data at the new seek position, which should fix crbug.com/440173

[servolk, 2015-07-07 23:03:02]

On 2015/06/26 18:14:01, servolk wrote:
> Please take another look.
> I've reworked this CL a bit to pass the current playback position
> (HTMLMediaElement::currentTime) from blink level, instead of providing a
> separate get_media_time_cb to ChunkDemuxer. This allows us to avoid changing
> public ChunkDemuxer interface and therefore will work identically on all
> platforms, including Android.
> Also, since we are now using HTMLMediaElement::currentTime, that changes to
the
> new seek position as soon as seek is started, we'll get better at protecting
> newly appended data at the new seek position, which should fix
crbug.com/440173

ping

[servolk, 2015-07-13 20:51:43]

On 2015/07/07 23:03:02, servolk wrote:
> On 2015/06/26 18:14:01, servolk wrote:
> > Please take another look.
> > I've reworked this CL a bit to pass the current playback position
> > (HTMLMediaElement::currentTime) from blink level, instead of providing a
> > separate get_media_time_cb to ChunkDemuxer. This allows us to avoid changing
> > public ChunkDemuxer interface and therefore will work identically on all
> > platforms, including Android.
> > Also, since we are now using HTMLMediaElement::currentTime, that changes to
> the
> > new seek position as soon as seek is started, we'll get better at protecting
> > newly appended data at the new seek position, which should fix
> crbug.com/440173
> 
> ping

ping. Matt, could you please take another look at this and the associated blink
CL (https://codereview.chromium.org/1013923002/) ?

[wolenetz, 2015-07-13 22:26:17]

On 2015/07/13 20:51:43, servolk wrote:
> On 2015/07/07 23:03:02, servolk wrote:
> > On 2015/06/26 18:14:01, servolk wrote:
> > > Please take another look.
> > > I've reworked this CL a bit to pass the current playback position
> > > (HTMLMediaElement::currentTime) from blink level, instead of providing a
> > > separate get_media_time_cb to ChunkDemuxer. This allows us to avoid
changing
> > > public ChunkDemuxer interface and therefore will work identically on all
> > > platforms, including Android.
> > > Also, since we are now using HTMLMediaElement::currentTime, that changes
to
> > the
> > > new seek position as soon as seek is started, we'll get better at
protecting
> > > newly appended data at the new seek position, which should fix
> > crbug.com/440173
> > 
> > ping
> 
> ping. Matt, could you please take another look at this and the associated
blink
> CL (https://codereview.chromium.org/1013923002/) ?

My apologies. I'll do review of both tomorrow at latest.

[servolk, 2015-07-14 21:07:12]

On 2015/07/13 22:26:17, wolenetz wrote:
> On 2015/07/13 20:51:43, servolk wrote:
> > On 2015/07/07 23:03:02, servolk wrote:
> > > On 2015/06/26 18:14:01, servolk wrote:
> > > > Please take another look.
> > > > I've reworked this CL a bit to pass the current playback position
> > > > (HTMLMediaElement::currentTime) from blink level, instead of providing a
> > > > separate get_media_time_cb to ChunkDemuxer. This allows us to avoid
> changing
> > > > public ChunkDemuxer interface and therefore will work identically on all
> > > > platforms, including Android.
> > > > Also, since we are now using HTMLMediaElement::currentTime, that changes
> to
> > > the
> > > > new seek position as soon as seek is started, we'll get better at
> protecting
> > > > newly appended data at the new seek position, which should fix
> > > crbug.com/440173
> > > 
> > > ping
> > 
> > ping. Matt, could you please take another look at this and the associated
> blink
> > CL (https://codereview.chromium.org/1013923002/) ?
> 
> My apologies. I'll do review of both tomorrow at latest.

Hi Matt,

To facilitate the review of my recent CLs related to MSE GC and to discuss some
future MSE GC work that needs to be done to bring it closer to spec, I've
started putting together a document describing the current design and know
issues with the current GC implementation:
https://docs.google.com/a/google.com/document/d/1kR2n0zt1Azq-7WyvRNPn2uBZCP1A...
Please feel free to comment/add your thoughts.

[servolk, 2015-07-28 01:44:29]

On 2015/07/14 21:07:12, servolk wrote:
> On 2015/07/13 22:26:17, wolenetz wrote:
> > On 2015/07/13 20:51:43, servolk wrote:
> > > On 2015/07/07 23:03:02, servolk wrote:
> > > > On 2015/06/26 18:14:01, servolk wrote:
> > > > > Please take another look.
> > > > > I've reworked this CL a bit to pass the current playback position
> > > > > (HTMLMediaElement::currentTime) from blink level, instead of providing
a
> > > > > separate get_media_time_cb to ChunkDemuxer. This allows us to avoid
> > changing
> > > > > public ChunkDemuxer interface and therefore will work identically on
all
> > > > > platforms, including Android.
> > > > > Also, since we are now using HTMLMediaElement::currentTime, that
changes
> > to
> > > > the
> > > > > new seek position as soon as seek is started, we'll get better at
> > protecting
> > > > > newly appended data at the new seek position, which should fix
> > > > crbug.com/440173
> > > > 
> > > > ping
> > > 
> > > ping. Matt, could you please take another look at this and the associated
> > blink
> > > CL (https://codereview.chromium.org/1013923002/) ?
> > 
> > My apologies. I'll do review of both tomorrow at latest.
> 
> Hi Matt,
> 
> To facilitate the review of my recent CLs related to MSE GC and to discuss
some
> future MSE GC work that needs to be done to bring it closer to spec, I've
> started putting together a document describing the current design and know
> issues with the current GC implementation:
>
https://docs.google.com/a/google.com/document/d/1kR2n0zt1Azq-7WyvRNPn2uBZCP1A...
> Please feel free to comment/add your thoughts.

ping

[servolk, 2015-07-30 23:06:02]

On 2015/07/28 01:44:29, servolk wrote:
> On 2015/07/14 21:07:12, servolk wrote:
> > On 2015/07/13 22:26:17, wolenetz wrote:
> > > On 2015/07/13 20:51:43, servolk wrote:
> > > > On 2015/07/07 23:03:02, servolk wrote:
> > > > > On 2015/06/26 18:14:01, servolk wrote:
> > > > > > Please take another look.
> > > > > > I've reworked this CL a bit to pass the current playback position
> > > > > > (HTMLMediaElement::currentTime) from blink level, instead of
providing
> a
> > > > > > separate get_media_time_cb to ChunkDemuxer. This allows us to avoid
> > > changing
> > > > > > public ChunkDemuxer interface and therefore will work identically on
> all
> > > > > > platforms, including Android.
> > > > > > Also, since we are now using HTMLMediaElement::currentTime, that
> changes
> > > to
> > > > > the
> > > > > > new seek position as soon as seek is started, we'll get better at
> > > protecting
> > > > > > newly appended data at the new seek position, which should fix
> > > > > crbug.com/440173
> > > > > 
> > > > > ping
> > > > 
> > > > ping. Matt, could you please take another look at this and the
associated
> > > blink
> > > > CL (https://codereview.chromium.org/1013923002/) ?
> > > 
> > > My apologies. I'll do review of both tomorrow at latest.
> > 
> > Hi Matt,
> > 
> > To facilitate the review of my recent CLs related to MSE GC and to discuss
> some
> > future MSE GC work that needs to be done to bring it closer to spec, I've
> > started putting together a document describing the current design and know
> > issues with the current GC implementation:
> >
>
https://docs.google.com/a/google.com/document/d/1kR2n0zt1Azq-7WyvRNPn2uBZCP1A...
> > Please feel free to comment/add your thoughts.
> 
> ping

ping

[servolk, 2015-08-04 20:22:43]

On 2015/07/30 23:06:02, servolk wrote:
> On 2015/07/28 01:44:29, servolk wrote:
> > On 2015/07/14 21:07:12, servolk wrote:
> > > On 2015/07/13 22:26:17, wolenetz wrote:
> > > > On 2015/07/13 20:51:43, servolk wrote:
> > > > > On 2015/07/07 23:03:02, servolk wrote:
> > > > > > On 2015/06/26 18:14:01, servolk wrote:
> > > > > > > Please take another look.
> > > > > > > I've reworked this CL a bit to pass the current playback position
> > > > > > > (HTMLMediaElement::currentTime) from blink level, instead of
> providing
> > a
> > > > > > > separate get_media_time_cb to ChunkDemuxer. This allows us to
avoid
> > > > changing
> > > > > > > public ChunkDemuxer interface and therefore will work identically
on
> > all
> > > > > > > platforms, including Android.
> > > > > > > Also, since we are now using HTMLMediaElement::currentTime, that
> > changes
> > > > to
> > > > > > the
> > > > > > > new seek position as soon as seek is started, we'll get better at
> > > > protecting
> > > > > > > newly appended data at the new seek position, which should fix
> > > > > > crbug.com/440173
> > > > > > 
> > > > > > ping
> > > > > 
> > > > > ping. Matt, could you please take another look at this and the
> associated
> > > > blink
> > > > > CL (https://codereview.chromium.org/1013923002/) ?
> > > > 
> > > > My apologies. I'll do review of both tomorrow at latest.
> > > 
> > > Hi Matt,
> > > 
> > > To facilitate the review of my recent CLs related to MSE GC and to discuss
> > some
> > > future MSE GC work that needs to be done to bring it closer to spec, I've
> > > started putting together a document describing the current design and know
> > > issues with the current GC implementation:
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1kR2n0zt1Azq-7WyvRNPn2uBZCP1A...
> > > Please feel free to comment/add your thoughts.
> > 
> > ping
> 
> ping

ping

[wolenetz, 2015-08-04 21:33:23]

My sincere apologies for slow review on this. I'll get to this soon.

[servolk, 2015-08-11 17:23:37]

On 2015/08/04 21:33:23, wolenetz wrote:
> My sincere apologies for slow review on this. I'll get to this soon.

ping. Matt, is there anything I can do to assist with reviewing this CL?

[wolenetz, 2015-08-11 17:27:20]

On 2015/08/11 17:23:37, servolk wrote:
> On 2015/08/04 21:33:23, wolenetz wrote:
> > My sincere apologies for slow review on this. I'll get to this soon.
> 
> ping. Matt, is there anything I can do to assist with reviewing this CL?

It's on my stack to do _today_. I'll let you know if there's anything I need
during CR. APologies again.

[wolenetz, 2015-08-12 20:13:43]

On 2015/08/11 17:27:20, wolenetz wrote:
> On 2015/08/11 17:23:37, servolk wrote:
> > On 2015/08/04 21:33:23, wolenetz wrote:
> > > My sincere apologies for slow review on this. I'll get to this soon.
> > 
> > ping. Matt, is there anything I can do to assist with reviewing this CL?
> 
> It's on my stack to do _today_. I'll let you know if there's anything I need
> during CR. APologies again.

I've made comments in the aforementioned doc and am proceeding with CR of this
now.

[wolenetz, 2015-08-12 21:09:57]

At long last: next round of review. In general, this looks good, though I would
like to know if major Chrome MSE users are aware of this upcoming set of change
to GC behavior.

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:85: size_t newDataSize) {
newDataSize is unused. Please provide comment in code that this is expected
(since current impl only collects *currently* buffered media until each stream
independently has buffered amount less than a hard-coded cap, and the size of
about-to-be-appended media is not yet taken into account during coded frame
eviction).
If there is not a crbug already tracking correct implementation of eviction
including making headroom (across the streams for this SourceBuffer) for
newDataSize, please file one and TODO it here.

https://codereview.chromium.org/1008463002/diff/460001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:134: // Attempts to perform garbage collection in
SourceBuffers associated with
This comment seems ChunkDemuxer-focused. Move it to
ChunkDemuxer::EvictCodedFrames in the .h?

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
media/filters/source_buffer_stream.cc:681: media_time =
(*range_for_next_append_)->GetStartTimestamp();
This may be too lax, though web apps can follow-up a failed (Prepare)Append's
"QuotaExceededError" with app-directed Remove(). How often do you think this
might break existing apps that may not expect built-in GC to not be so
aggressive any longer?  For instance, media_time could very frequently become
time 0 in this case, though the app may have only re-appended some buffers much
later than time 0 most recently.

Have you run this change by YouTube, Shaka, dash.js, etc folks to see if there
already exist robust app-directed Remove() implementations in common players
when the API gives QuotaExceededError on Append{Buffer,Stream}?

Also, note that this change along with the Blink change, while compliant with
spec in only GC'ing prior to main AppendBuffer() [and not incrementally across
each asynch append], substantially increases the risk of the hardcoded stream
buffer limits being exceeded due to arbitrarily sized stream or buffer appends.
Ditto my question: have you run this expectation change by the common player
folks to make sure we're not breaking major players with this change, especially
on memory-limited devices? (Solution for this paragraph would be to communicate
clearly with major players that appends should be granular, especially on
memory-limited platforms.)

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2555:
EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
I suspect it'll be not-obvious to later maintainers why we GC after, not before,
the appends in some of these test cases. To me, it's clear, especially in the
context of *this* change. Do the tests need some refactoring?

[wolenetz, 2015-08-12 21:13:26]

Updated comment:

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:85: size_t newDataSize) {
On 2015/08/12 21:09:57, wolenetz wrote:
> newDataSize is unused. Please provide comment in code that this is expected
> (since current impl only collects *currently* buffered media until each stream
> independently has buffered amount less than a hard-coded cap, and the size of
> about-to-be-appended media is not yet taken into account during coded frame
> eviction).
> If there is not a crbug already tracking correct implementation of eviction
> including making headroom (across the streams for this SourceBuffer) for
> newDataSize, please file one and TODO it here.

Hmm. disregard the TODO/bug request. There is nothing in the spec mentioning
that the coded frame eviction needs to make some headroom for upcoming append of
some size; just that the result of eviction is under the implementation-specific
cap.

|newDataSize| is unused; what is it meant for?

[servolk, 2015-08-13 00:31:42]

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:85: size_t newDataSize) {
On 2015/08/12 21:13:26, wolenetz wrote:
> On 2015/08/12 21:09:57, wolenetz wrote:
> > newDataSize is unused. Please provide comment in code that this is expected
> > (since current impl only collects *currently* buffered media until each
stream
> > independently has buffered amount less than a hard-coded cap, and the size
of
> > about-to-be-appended media is not yet taken into account during coded frame
> > eviction).
> > If there is not a crbug already tracking correct implementation of eviction
> > including making headroom (across the streams for this SourceBuffer) for
> > newDataSize, please file one and TODO it here.
> 
> Hmm. disregard the TODO/bug request. There is nothing in the spec mentioning
> that the coded frame eviction needs to make some headroom for upcoming append
of
> some size; just that the result of eviction is under the
implementation-specific
> cap.
> 
> |newDataSize| is unused; what is it meant for?

I was actually hoping to use the newDataSize to determine how much data we need
to evict in the GC algorithm to stay under the memory limits after the new data
is appended. Then I realized that it's currently problematic, since newDataSize
is the combined size of muxed video+audio data chunk and we can't know precisely
how much audio data and how much video data is in there without parsing the new
data chunk (which, unfortunately, is going to happen later, after the
prepareAppend/GC phase). That's why I think now that we need to do GC
differently - simultaneously across all streams belonging to the same SB. Then
we'll be able to use it to decide how much data to evict.
But thinking about this some more, it could be useful for us even today. We
could look, at the current ratio of the buffered audio/video range sizes in the
range_for_next_append_, and assume that the ratio of audio/video data in the
newly appended chunk is similar. Then we can estimate the size of audio/video in
the new data chunk, if the current ratio R=(currently buffered video size) /
(currently buffered audio + video sizes), then we can estimate the size of new
video data as R*newDataSize. And we can use it during GC algorithm to decide
when we can stop removing data. WDYT? It's not super precise, but I think that
would be a good enough heuristic, which should be pretty easy to implement.

https://codereview.chromium.org/1008463002/diff/460001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:134: // Attempts to perform garbage collection in
SourceBuffers associated with
On 2015/08/12 21:09:57, wolenetz wrote:
> This comment seems ChunkDemuxer-focused. Move it to
> ChunkDemuxer::EvictCodedFrames in the .h?

Will do

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
media/filters/source_buffer_stream.cc:681: media_time =
(*range_for_next_append_)->GetStartTimestamp();
On 2015/08/12 21:09:57, wolenetz wrote:
> This may be too lax, though web apps can follow-up a failed (Prepare)Append's
> "QuotaExceededError" with app-directed Remove(). How often do you think this
> might break existing apps that may not expect built-in GC to not be so
> aggressive any longer?  For instance, media_time could very frequently become
> time 0 in this case, though the app may have only re-appended some buffers
much
> later than time 0 most recently.
> 
> Have you run this change by YouTube, Shaka, dash.js, etc folks to see if there
> already exist robust app-directed Remove() implementations in common players
> when the API gives QuotaExceededError on Append{Buffer,Stream}?
> 
> Also, note that this change along with the Blink change, while compliant with
> spec in only GC'ing prior to main AppendBuffer() [and not incrementally across
> each asynch append], substantially increases the risk of the hardcoded stream
> buffer limits being exceeded due to arbitrarily sized stream or buffer
appends.
> Ditto my question: have you run this expectation change by the common player
> folks to make sure we're not breaking major players with this change,
especially
> on memory-limited devices? (Solution for this paragraph would be to
communicate
> clearly with major players that appends should be granular, especially on
> memory-limited platforms.)

Wait, we will only adjust media time here if range_for_next_append_ is valid
(not ranges_.end()) and only when last_appended_buffer_timestamp_ is less than
the current media_time (see the condition of the outer if). So I think we are
going to hit this code path relatively infrequently and only for players that
start appending data at the new position before issuing a seek request (Netflix
app is one example I'm aware of). And even in those cases, we are not resetting
media_time to 0, we are resetting it to
(*range_for_next_append_)->GetStartTimestamp(), which should be the beginning of
the range where the most recent append happened.
This will indeed make the GC algorithm less aggressive, but that's the point of
this CL! Previously apps were breaking in this case anyway, because they didn't
expect the newly appended data to be removed (crbug.com/440173 happened exactly
because of this excessive aggressiveness). But now we'll throw a
QuotaExceededErr exception, instead of removing unconsumed data, to give the
apps a chance to recover (e.g. by issuing a seek to the new position, which is
now guaranteed to have some data buffered).

I have reached out to strobe@ who works on YouTube player, I'll add you to that
mail thread shortly. I haven't contacted anyone else yet. I certainly agree that
we should notify MSE API users about this, but I'm not sure what would be the
best way to reach them. Do you know who would be the right people to follow up
on this from Shaka team and dash.js?

Re your last point - I'm not sure I understand your concern. It doesn't matter
if we do GC incrementally for each async append or if we do it once before a big
append. What matters is the total size of data being appended. If the total size
is enough to bring memory usage over the MSE buffer limits, then it will trigger
GC sooner or later anyway and current GC will remove some data that shouldn't be
removed. The problem is that currently GC just removes this data and leaves the
SB in a bad state (with gaps or data missing where the JS app expects it to be
present), and then we are toasted anyway. After this change we'll notify the app
(by throwing the QuotaExceeded exception) that there's not enough space in SB.
And then it's up to the app to either remove some data or wait a little bit
until more data is consumed and then attempt to append again.

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2555:
EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
On 2015/08/12 21:09:57, wolenetz wrote:
> I suspect it'll be not-obvious to later maintainers why we GC after, not
before,
> the appends in some of these test cases. To me, it's clear, especially in the
> context of *this* change. Do the tests need some refactoring?

Yes, I think I'll add a comment that we are skipping the GarbageCollectIfNeeded
invocation before the append, because we know buffer is empty at that point and
GC will be a no-op anyway, and then we do the GC after append, since GC would
have to happen before the next append anyway. Is that good enough?
It's true those test could use some refactoring, but I'm not sure how to do it
best. Perhaps I should introduce GCIfNeeded invocation inside test methods like
NewSegmentAppend to simulate more closely how the actual SB::appendBuffer works.
Or perhaps these test should all be done on the blink level, where we could use
the same SB API as JS apps use?

[servolk, 2015-08-20 02:45:27]

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:85: size_t newDataSize) {
On 2015/08/13 00:31:42, servolk wrote:
> On 2015/08/12 21:13:26, wolenetz wrote:
> > On 2015/08/12 21:09:57, wolenetz wrote:
> > > newDataSize is unused. Please provide comment in code that this is
expected
> > > (since current impl only collects *currently* buffered media until each
> stream
> > > independently has buffered amount less than a hard-coded cap, and the size
> of
> > > about-to-be-appended media is not yet taken into account during coded
frame
> > > eviction).
> > > If there is not a crbug already tracking correct implementation of
eviction
> > > including making headroom (across the streams for this SourceBuffer) for
> > > newDataSize, please file one and TODO it here.
> > 
> > Hmm. disregard the TODO/bug request. There is nothing in the spec mentioning
> > that the coded frame eviction needs to make some headroom for upcoming
append
> of
> > some size; just that the result of eviction is under the
> implementation-specific
> > cap.
> > 
> > |newDataSize| is unused; what is it meant for?
> 
> I was actually hoping to use the newDataSize to determine how much data we
need
> to evict in the GC algorithm to stay under the memory limits after the new
data
> is appended. Then I realized that it's currently problematic, since
newDataSize
> is the combined size of muxed video+audio data chunk and we can't know
precisely
> how much audio data and how much video data is in there without parsing the
new
> data chunk (which, unfortunately, is going to happen later, after the
> prepareAppend/GC phase). That's why I think now that we need to do GC
> differently - simultaneously across all streams belonging to the same SB. Then
> we'll be able to use it to decide how much data to evict.
> But thinking about this some more, it could be useful for us even today. We
> could look, at the current ratio of the buffered audio/video range sizes in
the
> range_for_next_append_, and assume that the ratio of audio/video data in the
> newly appended chunk is similar. Then we can estimate the size of audio/video
in
> the new data chunk, if the current ratio R=(currently buffered video size) /
> (currently buffered audio + video sizes), then we can estimate the size of new
> video data as R*newDataSize. And we can use it during GC algorithm to decide
> when we can stop removing data. WDYT? It's not super precise, but I think that
> would be a good enough heuristic, which should be pretty easy to implement.

Update: I've implemented the logic described above in the latest patchset. PTAL.

https://codereview.chromium.org/1008463002/diff/460001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:134: // Attempts to perform garbage collection in
SourceBuffers associated with
On 2015/08/13 00:31:42, servolk wrote:
> On 2015/08/12 21:09:57, wolenetz wrote:
> > This comment seems ChunkDemuxer-focused. Move it to
> > ChunkDemuxer::EvictCodedFrames in the .h?
> 
> Will do

Done.

[wolenetz, 2015-08-20 23:23:37]

Mostly minor stuff remaining. Thanks for adding the heuristic, though there is
definitely room for improvement in future around selecting coherent removal
ranges across (muxed) streams in a SourceState.

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:85: size_t newDataSize) {
On 2015/08/12 21:13:26, wolenetz wrote:
> On 2015/08/12 21:09:57, wolenetz wrote:
> > newDataSize is unused. Please provide comment in code that this is expected
> > (since current impl only collects *currently* buffered media until each
stream
> > independently has buffered amount less than a hard-coded cap, and the size
of
> > about-to-be-appended media is not yet taken into account during coded frame
> > eviction).
> > If there is not a crbug already tracking correct implementation of eviction
> > including making headroom (across the streams for this SourceBuffer) for
> > newDataSize, please file one and TODO it here.
> 
> Hmm. disregard the TODO/bug request. There is nothing in the spec mentioning
> that the coded frame eviction needs to make some headroom for upcoming append
of
> some size; just that the result of eviction is under the
implementation-specific
> cap.
> 
> |newDataSize| is unused; what is it meant for?

Oops. There is actually spec line-item in the coded frame eviction algorithm
targeting making this headroom: "Let removal ranges equal a list of presentation
time ranges that can be evicted from the presentation to make room for the new
data."

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/filters/source_b...
media/filters/source_buffer_stream_unittest.cc:2555:
EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
On 2015/08/13 00:31:42, servolk wrote:
> On 2015/08/12 21:09:57, wolenetz wrote:
> > I suspect it'll be not-obvious to later maintainers why we GC after, not
> before,
> > the appends in some of these test cases. To me, it's clear, especially in
the
> > context of *this* change. Do the tests need some refactoring?
> 
> Yes, I think I'll add a comment that we are skipping the
GarbageCollectIfNeeded
> invocation before the append, because we know buffer is empty at that point
and
> GC will be a no-op anyway, and then we do the GC after append, since GC would
> have to happen before the next append anyway. Is that good enough?
> It's true those test could use some refactoring, but I'm not sure how to do it
> best. Perhaps I should introduce GCIfNeeded invocation inside test methods
like
> NewSegmentAppend to simulate more closely how the actual SB::appendBuffer
works.
> Or perhaps these test should all be done on the blink level, where we could
use
> the same SB API as JS apps use?

Best solution for now, ISTM, would be to add a comment in the private
AppendBuffers(...) that the expected GC-prior-to-append behavior is separated
out into the tests to more precisely test the append and GC operations
independently.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:378: bool
SourceState::EvictCodedFrames(DecodeTimestamp media_time,
Please extract the heuristic for determining newAudioSize and newVideoSize into
a method, and add unit tests for that method.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:385: if (audio_ && video_ && videoBufferedSize >
0) {
nit: please add a comment describing this heuristic, perhaps mentioning that
other heuristics that incrementally build audio/video/text removal ranges that
are aligned in time could be a better solution, since muxed contents are muxed
in interleaved DTS sequence across the streams.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:386: newVideoSize = newDataSize *
audio_->GetBufferedSize() / videoBufferedSize;
This seems incorrect.
[estimated headroom for new video] should be = [total new media size] *
[proportion of video of all buffered size]

example:
100MB total currently buffered, 80MB video + 20MB audio
total new media size = 10MB
estimated new video size = 10MB * (80MB / 100MB) = 8MB
estimated new audio size = 10MB * (20MB / 100MB) = 2MB

However, current patch set would instead calculate:
estimated new video size = 10MB * (20MB / 80MB) = 2.5MB
estimated new audio size = 10MB - 2.5MB = 7.5MB

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:393: 
Please add some DVLOGs describing newDataSize, newAudioSize, newVideoSize, and
before and after video_,audio_ ->GetBufferedSize()

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
media/filters/source_buffer_stream.cc:657: for (RangeList::iterator itr =
ranges_.begin(); itr != ranges_.end(); ++itr)
instead, use GetBufferedSize() ?

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
media/filters/source_buffer_stream.cc:661: if (ranges_size + newDataSize <=
memory_limit_)
this can overflow

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
media/filters/source_buffer_stream.cc:664: size_t bytes_to_free = ranges_size +
newDataSize - memory_limit_;

If newDataSize > memory_limit, trivially return false ? Or remove everything and
then return false ?

[servolk, 2015-08-21 01:32:09]

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1008463002/diff/460001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:85: size_t newDataSize) {
On 2015/08/20 23:23:37, wolenetz wrote:
> On 2015/08/12 21:13:26, wolenetz wrote:
> > On 2015/08/12 21:09:57, wolenetz wrote:
> > > newDataSize is unused. Please provide comment in code that this is
expected
> > > (since current impl only collects *currently* buffered media until each
> stream
> > > independently has buffered amount less than a hard-coded cap, and the size
> of
> > > about-to-be-appended media is not yet taken into account during coded
frame
> > > eviction).
> > > If there is not a crbug already tracking correct implementation of
eviction
> > > including making headroom (across the streams for this SourceBuffer) for
> > > newDataSize, please file one and TODO it here.
> > 
> > Hmm. disregard the TODO/bug request. There is nothing in the spec mentioning
> > that the coded frame eviction needs to make some headroom for upcoming
append
> of
> > some size; just that the result of eviction is under the
> implementation-specific
> > cap.
> > 
> > |newDataSize| is unused; what is it meant for?
> 
> Oops. There is actually spec line-item in the coded frame eviction algorithm
> targeting making this headroom: "Let removal ranges equal a list of
presentation
> time ranges that can be evicted from the presentation to make room for the new
> data."

Yes, step 3 of 3.5.14

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:378: bool
SourceState::EvictCodedFrames(DecodeTimestamp media_time,
On 2015/08/20 23:23:37, wolenetz wrote:
> Please extract the heuristic for determining newAudioSize and newVideoSize
into
> a method, and add unit tests for that method.

I've extracted the heuristic into a new method in SourceState class. But that's
an internal helper class for the ChunkDemuxer and currently there are no direct
tests for it. So for now I've added a new ChunkDemuxer test for
EvictCodedFrames, that uses muxed data and implicitly tests this function as
well.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:385: if (audio_ && video_ && videoBufferedSize >
0) {
On 2015/08/20 23:23:37, wolenetz wrote:
> nit: please add a comment describing this heuristic, perhaps mentioning that
> other heuristics that incrementally build audio/video/text removal ranges that
> are aligned in time could be a better solution, since muxed contents are muxed
> in interleaved DTS sequence across the streams.

Done.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:386: newVideoSize = newDataSize *
audio_->GetBufferedSize() / videoBufferedSize;
On 2015/08/20 23:23:37, wolenetz wrote:
> This seems incorrect.
> [estimated headroom for new video] should be = [total new media size] *
> [proportion of video of all buffered size]
> 
> example:
> 100MB total currently buffered, 80MB video + 20MB audio
> total new media size = 10MB
> estimated new video size = 10MB * (80MB / 100MB) = 8MB
> estimated new audio size = 10MB * (20MB / 100MB) = 2MB
> 
> However, current patch set would instead calculate:
> estimated new video size = 10MB * (20MB / 80MB) = 2.5MB
> estimated new audio size = 10MB - 2.5MB = 7.5MB

Oh, yes, you are right, not sure what I was thinking, I've changed the formula
to newDataSize * V / (A+V) now.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:393: 
On 2015/08/20 23:23:37, wolenetz wrote:
> Please add some DVLOGs describing newDataSize, newAudioSize, newVideoSize, and
> before and after video_,audio_ ->GetBufferedSize()

Done.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
media/filters/source_buffer_stream.cc:657: for (RangeList::iterator itr =
ranges_.begin(); itr != ranges_.end(); ++itr)
On 2015/08/20 23:23:37, wolenetz wrote:
> instead, use GetBufferedSize() ?

Done.

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
media/filters/source_buffer_stream.cc:661: if (ranges_size + newDataSize <=
memory_limit_)
On 2015/08/20 23:23:37, wolenetz wrote:
> this can overflow

Done, added a sanity and overflow check

https://codereview.chromium.org/1008463002/diff/500001/media/filters/source_b...
media/filters/source_buffer_stream.cc:664: size_t bytes_to_free = ranges_size +
newDataSize - memory_limit_;
On 2015/08/20 23:23:37, wolenetz wrote:
> 
> If newDataSize > memory_limit, trivially return false ? Or remove everything
and
> then return false ?

Done.

[servolk, 2015-08-21 15:59:55]

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

[commit-bot: I haz the power, 2015-08-21 16:00:36]

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

[commit-bot: I haz the power, 2015-08-21 16:47:28]

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

[commit-bot: I haz the power, 2015-08-21 16:47:29]

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

[servolk, 2015-08-21 17:44:42]

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

[commit-bot: I haz the power, 2015-08-21 17:45:26]

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

[wolenetz, 2015-08-21 18:15:57]

Almost there! :)
Marking not lgtm due to the current estimation logic is flawed (integer ratio
used in calculation).

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:405: DCHECK(bufferedVideoSize + bufferedAudioSize
> 0);
Hmm. size_t can't be negative. So this is only checking that there is something
buffered, right?

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:406: size_t estimatedVideoData =
muxed_data_chunk_size * bufferedVideoSize /
not lgtm:
Working with integers, this may not do what you want:

100MB existing, 51MB video, 49MB audio. 10MB new. I assume you'd want 4.9MB of
estimated audio headroom and 5.1MB of estimated video headroom (the latter being
the output of this helper method).
This method would instead yield:
estimated video data size == 10MB * (51MB / 100MB) == 10MB * (0) == 0

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:3805: TEST_F(ChunkDemuxerTest,
EvictCodedFramesTest) {
Please update this test to expose the problem with integer-based ratio
calculation from my comment in the .cc of this patch set.

[servolk, 2015-08-21 18:19:14]

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:405: DCHECK(bufferedVideoSize + bufferedAudioSize
> 0);
On 2015/08/21 18:15:57, wolenetz wrote:
> Hmm. size_t can't be negative. So this is only checking that there is
something
> buffered, right?

Right

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:406: size_t estimatedVideoData =
muxed_data_chunk_size * bufferedVideoSize /
On 2015/08/21 18:15:56, wolenetz wrote:
> not lgtm:
> Working with integers, this may not do what you want:
> 
> 100MB existing, 51MB video, 49MB audio. 10MB new. I assume you'd want 4.9MB of
> estimated audio headroom and 5.1MB of estimated video headroom (the latter
being
> the output of this helper method).
> This method would instead yield:
> estimated video data size == 10MB * (51MB / 100MB) == 10MB * (0) == 0

I believe it would be calculated as (10 * 51) / 100 = 5. I can either add extra
parentheses around multiplication or use floating-point calculations. Which one
do you like better?

[commit-bot: I haz the power, 2015-08-21 18:32:23]

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

[commit-bot: I haz the power, 2015-08-21 18:32:24]

Dry run: A disapproval has been posted by following reviewers:
wolenetz@chromium.org.
Please make sure to get positive review before another CQ attempt.

[servolk, 2015-08-21 19:06:45]

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:405: DCHECK(bufferedVideoSize + bufferedAudioSize
> 0);
On 2015/08/21 18:19:14, servolk wrote:
> On 2015/08/21 18:15:57, wolenetz wrote:
> > Hmm. size_t can't be negative. So this is only checking that there is
> something
> > buffered, right?
> 
> Right

(this is just a sanity check, since we are going to divide by that number in the
next line)

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:406: size_t estimatedVideoData =
muxed_data_chunk_size * bufferedVideoSize /
On 2015/08/21 18:19:14, servolk wrote:
> On 2015/08/21 18:15:56, wolenetz wrote:
> > not lgtm:
> > Working with integers, this may not do what you want:
> > 
> > 100MB existing, 51MB video, 49MB audio. 10MB new. I assume you'd want 4.9MB
of
> > estimated audio headroom and 5.1MB of estimated video headroom (the latter
> being
> > the output of this helper method).
> > This method would instead yield:
> > estimated video data size == 10MB * (51MB / 100MB) == 10MB * (0) == 0
> 
> I believe it would be calculated as (10 * 51) / 100 = 5. I can either add
extra
> parentheses around multiplication or use floating-point calculations. Which
one
> do you like better?

Since I had to upload a new patchset with the updated unit tests anyway, I've
added parens here to make this clearer.

[wolenetz, 2015-08-21 19:39:37]

lgtm % additional sanity nit.

Not using floating point for the ratio seems ok to me, though the example you
gave yields 5MB, not 5.1MB. I suspect that if rounding error introduces enough
error to cause unexpected evictions, then that's already an extreme edge case
already, and apps will observe simply too much evicted in the intersection of
the tracks' buffered ranges (which is all they can observe from
SourceBuffer.buffered).

Thanks for doing this!

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:405: DCHECK(bufferedVideoSize + bufferedAudioSize
> 0);
On 2015/08/21 19:06:45, servolk wrote:
> On 2015/08/21 18:19:14, servolk wrote:
> > On 2015/08/21 18:15:57, wolenetz wrote:
> > > Hmm. size_t can't be negative. So this is only checking that there is
> > something
> > > buffered, right?
> > 
> > Right
> 
> (this is just a sanity check, since we are going to divide by that number in
the
> next line)

Acknowledged.
Let's please strengthen sanity here by ensuring the addition doesn't overflow.
something like DCHECK(max_size_t - bufferedVideoSize > bufferedAudioSize)

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:406: size_t estimatedVideoData =
muxed_data_chunk_size * bufferedVideoSize /
On 2015/08/21 19:06:45, servolk wrote:
> On 2015/08/21 18:19:14, servolk wrote:
> > On 2015/08/21 18:15:56, wolenetz wrote:
> > > not lgtm:
> > > Working with integers, this may not do what you want:
> > > 
> > > 100MB existing, 51MB video, 49MB audio. 10MB new. I assume you'd want
4.9MB
> of
> > > estimated audio headroom and 5.1MB of estimated video headroom (the latter
> > being
> > > the output of this helper method).
> > > This method would instead yield:
> > > estimated video data size == 10MB * (51MB / 100MB) == 10MB * (0) == 0
> > 
> > I believe it would be calculated as (10 * 51) / 100 = 5. I can either add
> extra
> > parentheses around multiplication or use floating-point calculations. Which
> one
> > do you like better?
> 
> Since I had to upload a new patchset with the updated unit tests anyway, I've
> added parens here to make this clearer.

Acknowledged.

[servolk, 2015-08-21 20:16:54]

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:405: DCHECK(bufferedVideoSize + bufferedAudioSize
> 0);
On 2015/08/21 19:39:37, wolenetz wrote:
> On 2015/08/21 19:06:45, servolk wrote:
> > On 2015/08/21 18:19:14, servolk wrote:
> > > On 2015/08/21 18:15:57, wolenetz wrote:
> > > > Hmm. size_t can't be negative. So this is only checking that there is
> > > something
> > > > buffered, right?
> > > 
> > > Right
> > 
> > (this is just a sanity check, since we are going to divide by that number in
> the
> > next line)
> 
> Acknowledged.
> Let's please strengthen sanity here by ensuring the addition doesn't overflow.
> something like DCHECK(max_size_t - bufferedVideoSize > bufferedAudioSize)

Done.

[servolk, 2015-08-21 20:17:06]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-08-21 20:17:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1008463002/#ps600001
(title: "Added overflow check to sanity checks and use CHECK instead of DCHECK")

[wolenetz, 2015-08-21 20:17:58]

On 2015/08/21 20:16:54, servolk wrote:
>
https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/1008463002/diff/540001/media/filters/chunk_de...
> media/filters/chunk_demuxer.cc:405: DCHECK(bufferedVideoSize +
bufferedAudioSize
> > 0);
> On 2015/08/21 19:39:37, wolenetz wrote:
> > On 2015/08/21 19:06:45, servolk wrote:
> > > On 2015/08/21 18:19:14, servolk wrote:
> > > > On 2015/08/21 18:15:57, wolenetz wrote:
> > > > > Hmm. size_t can't be negative. So this is only checking that there is
> > > > something
> > > > > buffered, right?
> > > > 
> > > > Right
> > > 
> > > (this is just a sanity check, since we are going to divide by that number
in
> > the
> > > next line)
> > 
> > Acknowledged.
> > Let's please strengthen sanity here by ensuring the addition doesn't
overflow.
> > something like DCHECK(max_size_t - bufferedVideoSize > bufferedAudioSize)
> 
> Done.

LGTM! thanks for making them CHECK's. Those are better for this particular case.

[commit-bot: I haz the power, 2015-08-21 20:18:52]

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

[wolenetz, 2015-08-21 20:25:02]

From offline chat:

https://codereview.chromium.org/1008463002/diff/600001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1008463002/diff/600001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:407: CHECK(bufferedAudioSize <
tl;dr off by one, but still lgtm.
(bufferedAudioSize+bufferedVideoSize is limited to pass this CHECK only if they
sum to max() - 1, rather than max(). Regardless, size_t.max() is huge and we
shouldn't be buffering anywhere near that!

[commit-bot: I haz the power, 2015-08-21 23:39:17]

Committed patchset #31 (id:600001)

[commit-bot: I haz the power, 2015-08-21 23:40:09]

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