Fix buffered range updates in media::Pipeline

Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514
Committed: https://crrev.com/21e682ef9c2d34528aa24bf33f2e4b2734ac573b
Cr-Commit-Position: refs/heads/master@{#369585}

[servolk, 2015-12-17 01:06:08]

Description was changed from

==========
Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514
==========

to

==========
Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514
==========

[servolk, 2015-12-17 01:06:08]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, halliwell@chromium.org, hubbe@chromium.org,
jrummell@chromium.org

[halliwell, 2015-12-17 01:17:30]

On 2015/12/17 01:06:15, servolk wrote:

chromecast/ lgtm % compile error :)

[DaleCurtis, 2015-12-17 02:28:28]

dalecurtis@chromium.org changed reviewers:
+ wolenetz@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2015-12-17 02:28:54]

=>wolenetz

[servolk, 2015-12-18 22:29:13]

On 2015/12/17 02:28:54, DaleCurtis_OOO_Until_Jan_5 wrote:
> =>wolenetz

ping

[servolk, 2016-01-06 00:45:14]

On 2015/12/18 22:29:13, servolk wrote:
> On 2015/12/17 02:28:54, DaleCurtis_OOO_Until_Jan_5 wrote:
> > =>wolenetz
> 
> ping

ping

[wolenetz, 2016-01-06 00:46:56]

On 2016/01/06 00:45:14, servolk wrote:
> On 2015/12/18 22:29:13, servolk wrote:
> > On 2015/12/17 02:28:54, DaleCurtis_OOO_Until_Jan_5 wrote:
> > > =>wolenetz
> > 
> > ping
> 
> ping

I'll CR this ASAP tomorrow morning. Thanks for post-holiday catch-up patience :)

[wolenetz, 2016-01-06 19:28:41]

Great catch. Thanks for working on this, servolk@.
Explicit range removal, not just MSE coded frame eviction, should also trigger
updates to the exposed-to-web-app buffered time ranges, but it doesn't yet,
either.

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

https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:28: // becomes evicted, e.g. due to MSE GC algorithm).
nit: or by explicit removal of ranges directed by MSE web app.

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

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
media/base/pipeline.h:353: // DidLoadingProgress().
I'm not sure this has the same semantic meaning. If MSE web app explicitly
removes some previously buffered range(s), does that truly mean loading has
progressed?

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:661: ranges.Add(base::TimeDelta::FromSeconds(0),
kDuration / 8);
nit: just base::TimeDelta() is much more frequently used to represent time 0

https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1094: 
I think explicit SourceBuffer.Remove() operations should also result in updated
ranges in MediaSource.buffered(). It looks like we might not have previously
done that correctly, either! And we probably need to implement signalling of
ranges change (call the new demuxerhost method from chunkdemuxer, similarly to
how append is currently done). And test this, too.

https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1110: do {
nit: before do, EXPECT_EQ(1u, buffered_ranges.size());

[wolenetz, 2016-01-06 19:38:30]

On 2016/01/06 19:28:41, wolenetz wrote:
> Great catch. Thanks for working on this, servolk@.
> Explicit range removal, not just MSE coded frame eviction, should also trigger
> updates to the exposed-to-web-app buffered time ranges, but it doesn't yet,
> either.
> 
> https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h
> File media/base/demuxer.h (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h#ne...
> media/base/demuxer.h:28: // becomes evicted, e.g. due to MSE GC algorithm).
> nit: or by explicit removal of ranges directed by MSE web app.
> 
> https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h
> File media/base/pipeline.h (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
> media/base/pipeline.h:353: // DidLoadingProgress().
> I'm not sure this has the same semantic meaning. If MSE web app explicitly
> removes some previously buffered range(s), does that truly mean loading has
> progressed?
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> File media/base/pipeline_unittest.cc (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> media/base/pipeline_unittest.cc:661:
ranges.Add(base::TimeDelta::FromSeconds(0),
> kDuration / 8);
> nit: just base::TimeDelta() is much more frequently used to represent time 0
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> media/test/pipeline_integration_test.cc:1094: 
> I think explicit SourceBuffer.Remove() operations should also result in
updated
> ranges in MediaSource.buffered(). It looks like we might not have previously
> done that correctly, either! And we probably need to implement signalling of
> ranges change (call the new demuxerhost method from chunkdemuxer, similarly to
> how append is currently done). And test this, too.
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> media/test/pipeline_integration_test.cc:1110: do {
> nit: before do, EXPECT_EQ(1u, buffered_ranges.size());

Also, hubbe@: Does WMPI's ::buffered() implementation that takes the current
pipeline buffered ranges and adds them all to the buffered data source make
sense still now that removals can occur, not just additions? Likewise, does
buffered data source's didloadingprogress() implementation make sense when such
range removals might trigger "progress" with current logic?

[hubbe, 2016-01-06 19:43:27]

On 2016/01/06 19:38:30, wolenetz wrote:
> On 2016/01/06 19:28:41, wolenetz wrote:
> > Great catch. Thanks for working on this, servolk@.
> > Explicit range removal, not just MSE coded frame eviction, should also
trigger
> > updates to the exposed-to-web-app buffered time ranges, but it doesn't yet,
> > either.
> > 
> > https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h
> > File media/base/demuxer.h (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h#ne...
> > media/base/demuxer.h:28: // becomes evicted, e.g. due to MSE GC algorithm).
> > nit: or by explicit removal of ranges directed by MSE web app.
> > 
> > https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h
> > File media/base/pipeline.h (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
> > media/base/pipeline.h:353: // DidLoadingProgress().
> > I'm not sure this has the same semantic meaning. If MSE web app explicitly
> > removes some previously buffered range(s), does that truly mean loading has
> > progressed?
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > File media/base/pipeline_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > media/base/pipeline_unittest.cc:661:
> ranges.Add(base::TimeDelta::FromSeconds(0),
> > kDuration / 8);
> > nit: just base::TimeDelta() is much more frequently used to represent time 0
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > File media/test/pipeline_integration_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > media/test/pipeline_integration_test.cc:1094: 
> > I think explicit SourceBuffer.Remove() operations should also result in
> updated
> > ranges in MediaSource.buffered(). It looks like we might not have previously
> > done that correctly, either! And we probably need to implement signalling of
> > ranges change (call the new demuxerhost method from chunkdemuxer, similarly
to
> > how append is currently done). And test this, too.
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > media/test/pipeline_integration_test.cc:1110: do {
> > nit: before do, EXPECT_EQ(1u, buffered_ranges.size());
> 
> Also, hubbe@: Does WMPI's ::buffered() implementation that takes the current
> pipeline buffered ranges and adds them all to the buffered data source make
> sense still now that removals can occur, not just additions? Likewise, does
> buffered data source's didloadingprogress() implementation make sense when
such
> range removals might trigger "progress" with current logic?

Not entirely sure. WMPI assumes that data ends up cached in the disk cache, so
it shows all data
it's ever loaded as buffered.

[wolenetz, 2016-01-06 21:02:06]

On 2016/01/06 19:43:27, hubbe wrote:
> On 2016/01/06 19:38:30, wolenetz wrote:
> > On 2016/01/06 19:28:41, wolenetz wrote:
> > > Great catch. Thanks for working on this, servolk@.
> > > Explicit range removal, not just MSE coded frame eviction, should also
> trigger
> > > updates to the exposed-to-web-app buffered time ranges, but it doesn't
yet,
> > > either.
> > > 
> > > https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h
> > > File media/base/demuxer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h#ne...
> > > media/base/demuxer.h:28: // becomes evicted, e.g. due to MSE GC
algorithm).
> > > nit: or by explicit removal of ranges directed by MSE web app.
> > > 
> > >
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h
> > > File media/base/pipeline.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
> > > media/base/pipeline.h:353: // DidLoadingProgress().
> > > I'm not sure this has the same semantic meaning. If MSE web app explicitly
> > > removes some previously buffered range(s), does that truly mean loading
has
> > > progressed?
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > > File media/base/pipeline_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > > media/base/pipeline_unittest.cc:661:
> > ranges.Add(base::TimeDelta::FromSeconds(0),
> > > kDuration / 8);
> > > nit: just base::TimeDelta() is much more frequently used to represent time
0
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > > File media/test/pipeline_integration_test.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > > media/test/pipeline_integration_test.cc:1094: 
> > > I think explicit SourceBuffer.Remove() operations should also result in
> > updated
> > > ranges in MediaSource.buffered(). It looks like we might not have
previously
> > > done that correctly, either! And we probably need to implement signalling
of
> > > ranges change (call the new demuxerhost method from chunkdemuxer,
similarly
> to
> > > how append is currently done). And test this, too.
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > > media/test/pipeline_integration_test.cc:1110: do {
> > > nit: before do, EXPECT_EQ(1u, buffered_ranges.size());
> > 
> > Also, hubbe@: Does WMPI's ::buffered() implementation that takes the current
> > pipeline buffered ranges and adds them all to the buffered data source make
> > sense still now that removals can occur, not just additions? Likewise, does
> > buffered data source's didloadingprogress() implementation make sense when
> such
> > range removals might trigger "progress" with current logic?
> 
> Not entirely sure. WMPI assumes that data ends up cached in the disk cache, so
> it shows all data
> it's ever loaded as buffered.

Is WMPI's buffered_data_source_ really only useful in the presence of non-MSE
pipeline? The MSE version of WMPI+pipeline doesn't report buffered ranges in
bytes, but in times, and already has what looks to me like the same/similar
logic as WMPI's buffered_data_source_ internally in pipeline.cc.

I wonder if we really need to use WMPI's buffered_data_source if WMPI is in MSE
mode. I also wonder if we really need two very-similar buffered
range/didloadingprogress management mechanisms (in demuxerhost, and in
buffereddatasourcehost). The former could be fixed in this CL. Refactoring the
latter seems out of scope a little to me for this CL.

[servolk, 2016-01-06 21:05:05]

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

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
media/base/pipeline.h:353: // DidLoadingProgress().
On 2016/01/06 19:28:41, wolenetz wrote:
> I'm not sure this has the same semantic meaning. If MSE web app explicitly
> removes some previously buffered range(s), does that truly mean loading has
> progressed?

I assumed that the HTMLMediaElement 'progress' event was a notification for the
apps that HTMLMediaElement::buffered ranges have changed. But now I checked the
spec
(http://www.w3.org/TR/2011/WD-html5-20110405/video.html#event-media-progress)
and it says "[Progress event is dispatched when...] The user agent is fetching
media data."
But if we don't fire the 'progress' event here, is there an alternative way to
notify apps that buffered ranges have changed?

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:661: ranges.Add(base::TimeDelta::FromSeconds(0),
kDuration / 8);
On 2016/01/06 19:28:41, wolenetz wrote:
> nit: just base::TimeDelta() is much more frequently used to represent time 0

Done.

https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1094: 
On 2016/01/06 19:28:41, wolenetz wrote:
> I think explicit SourceBuffer.Remove() operations should also result in
updated
> ranges in MediaSource.buffered(). It looks like we might not have previously
> done that correctly, either! And we probably need to implement signalling of
> ranges change (call the new demuxerhost method from chunkdemuxer, similarly to
> how append is currently done). And test this, too.

Good catch, done: added signalling from ChunkDemuxer::Remove and added a unit
test to verify that buffered ranges change after MSE SB.remove.

https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1110: do {
On 2016/01/06 19:28:41, wolenetz wrote:
> nit: before do, EXPECT_EQ(1u, buffered_ranges.size());

Done.

[servolk, 2016-01-13 21:30:18]

On 2016/01/06 21:05:05, servolk wrote:
> https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h
> File media/base/pipeline.h (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
> media/base/pipeline.h:353: // DidLoadingProgress().
> On 2016/01/06 19:28:41, wolenetz wrote:
> > I'm not sure this has the same semantic meaning. If MSE web app explicitly
> > removes some previously buffered range(s), does that truly mean loading has
> > progressed?
> 
> I assumed that the HTMLMediaElement 'progress' event was a notification for
the
> apps that HTMLMediaElement::buffered ranges have changed. But now I checked
the
> spec
> (http://www.w3.org/TR/2011/WD-html5-20110405/video.html#event-media-progress)
> and it says "[Progress event is dispatched when...] The user agent is fetching
> media data."
> But if we don't fire the 'progress' event here, is there an alternative way to
> notify apps that buffered ranges have changed?
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> File media/base/pipeline_unittest.cc (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> media/base/pipeline_unittest.cc:661:
ranges.Add(base::TimeDelta::FromSeconds(0),
> kDuration / 8);
> On 2016/01/06 19:28:41, wolenetz wrote:
> > nit: just base::TimeDelta() is much more frequently used to represent time 0
> 
> Done.
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> media/test/pipeline_integration_test.cc:1094: 
> On 2016/01/06 19:28:41, wolenetz wrote:
> > I think explicit SourceBuffer.Remove() operations should also result in
> updated
> > ranges in MediaSource.buffered(). It looks like we might not have previously
> > done that correctly, either! And we probably need to implement signalling of
> > ranges change (call the new demuxerhost method from chunkdemuxer, similarly
to
> > how append is currently done). And test this, too.
> 
> Good catch, done: added signalling from ChunkDemuxer::Remove and added a unit
> test to verify that buffered ranges change after MSE SB.remove.
> 
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> media/test/pipeline_integration_test.cc:1110: do {
> On 2016/01/06 19:28:41, wolenetz wrote:
> > nit: before do, EXPECT_EQ(1u, buffered_ranges.size());
> 
> Done.

ping

[wolenetz, 2016-01-13 22:23:03]

On 2016/01/13 21:30:18, servolk wrote:
> On 2016/01/06 21:05:05, servolk wrote:
> > https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h
> > File media/base/pipeline.h (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
> > media/base/pipeline.h:353: // DidLoadingProgress().
> > On 2016/01/06 19:28:41, wolenetz wrote:
> > > I'm not sure this has the same semantic meaning. If MSE web app explicitly
> > > removes some previously buffered range(s), does that truly mean loading
has
> > > progressed?
> > 
> > I assumed that the HTMLMediaElement 'progress' event was a notification for
> the
> > apps that HTMLMediaElement::buffered ranges have changed. But now I checked
> the
> > spec
> >
(http://www.w3.org/TR/2011/WD-html5-20110405/video.html#event-media-progress)
> > and it says "[Progress event is dispatched when...] The user agent is
fetching
> > media data."
> > But if we don't fire the 'progress' event here, is there an alternative way
to
> > notify apps that buffered ranges have changed?
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > File media/base/pipeline_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > media/base/pipeline_unittest.cc:661:
> ranges.Add(base::TimeDelta::FromSeconds(0),
> > kDuration / 8);
> > On 2016/01/06 19:28:41, wolenetz wrote:
> > > nit: just base::TimeDelta() is much more frequently used to represent time
0
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > File media/test/pipeline_integration_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > media/test/pipeline_integration_test.cc:1094: 
> > On 2016/01/06 19:28:41, wolenetz wrote:
> > > I think explicit SourceBuffer.Remove() operations should also result in
> > updated
> > > ranges in MediaSource.buffered(). It looks like we might not have
previously
> > > done that correctly, either! And we probably need to implement signalling
of
> > > ranges change (call the new demuxerhost method from chunkdemuxer,
similarly
> to
> > > how append is currently done). And test this, too.
> > 
> > Good catch, done: added signalling from ChunkDemuxer::Remove and added a
unit
> > test to verify that buffered ranges change after MSE SB.remove.
> > 
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > media/test/pipeline_integration_test.cc:1110: do {
> > On 2016/01/06 19:28:41, wolenetz wrote:
> > > nit: before do, EXPECT_EQ(1u, buffered_ranges.size());
> > 
> > Done.
> 
> ping

hubbe@:
Is WMPI's buffered_data_source_ really only useful in the presence of non-MSE
pipeline? The MSE version of WMPI+pipeline doesn't report buffered ranges in
bytes, but in times, and already has what looks to me like the same/similar
logic as WMPI's buffered_data_source_ internally in pipeline.cc.

I wonder if we really need to use WMPI's buffered_data_source if WMPI is in MSE
mode. I also wonder if we really need two very-similar buffered
range/didloadingprogress management mechanisms (in demuxerhost, and in
buffereddatasourcehost). The former could be fixed in this CL. Refactoring the
latter seems out of scope a little to me for this CL.

servolk@: I'll take another look at your most recent update later today. I was
pending answers hopefully from hubbe@, too.

[wolenetz, 2016-01-14 01:50:18]

On 2016/01/13 22:23:03, wolenetz wrote:
> On 2016/01/13 21:30:18, servolk wrote:
> > On 2016/01/06 21:05:05, servolk wrote:
> > >
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h
> > > File media/base/pipeline.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
> > > media/base/pipeline.h:353: // DidLoadingProgress().
> > > On 2016/01/06 19:28:41, wolenetz wrote:
> > > > I'm not sure this has the same semantic meaning. If MSE web app
explicitly
> > > > removes some previously buffered range(s), does that truly mean loading
> has
> > > > progressed?
> > > 
> > > I assumed that the HTMLMediaElement 'progress' event was a notification
for
> > the
> > > apps that HTMLMediaElement::buffered ranges have changed. But now I
checked
> > the
> > > spec
> > >
> (http://www.w3.org/TR/2011/WD-html5-20110405/video.html#event-media-progress)
> > > and it says "[Progress event is dispatched when...] The user agent is
> fetching
> > > media data."
> > > But if we don't fire the 'progress' event here, is there an alternative
way
> to
> > > notify apps that buffered ranges have changed?
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > > File media/base/pipeline_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
> > > media/base/pipeline_unittest.cc:661:
> > ranges.Add(base::TimeDelta::FromSeconds(0),
> > > kDuration / 8);
> > > On 2016/01/06 19:28:41, wolenetz wrote:
> > > > nit: just base::TimeDelta() is much more frequently used to represent
time
> 0
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > > File media/test/pipeline_integration_test.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > > media/test/pipeline_integration_test.cc:1094: 
> > > On 2016/01/06 19:28:41, wolenetz wrote:
> > > > I think explicit SourceBuffer.Remove() operations should also result in
> > > updated
> > > > ranges in MediaSource.buffered(). It looks like we might not have
> previously
> > > > done that correctly, either! And we probably need to implement
signalling
> of
> > > > ranges change (call the new demuxerhost method from chunkdemuxer,
> similarly
> > to
> > > > how append is currently done). And test this, too.
> > > 
> > > Good catch, done: added signalling from ChunkDemuxer::Remove and added a
> unit
> > > test to verify that buffered ranges change after MSE SB.remove.
> > > 
> > >
> >
>
https://codereview.chromium.org/1526303004/diff/40001/media/test/pipeline_int...
> > > media/test/pipeline_integration_test.cc:1110: do {
> > > On 2016/01/06 19:28:41, wolenetz wrote:
> > > > nit: before do, EXPECT_EQ(1u, buffered_ranges.size());
> > > 
> > > Done.
> > 
> > ping
> 
> hubbe@:
> Is WMPI's buffered_data_source_ really only useful in the presence of non-MSE
> pipeline? The MSE version of WMPI+pipeline doesn't report buffered ranges in
> bytes, but in times, and already has what looks to me like the same/similar
> logic as WMPI's buffered_data_source_ internally in pipeline.cc.
> 
> I wonder if we really need to use WMPI's buffered_data_source if WMPI is in
MSE
> mode. I also wonder if we really need two very-similar buffered
> range/didloadingprogress management mechanisms (in demuxerhost, and in
> buffereddatasourcehost). The former could be fixed in this CL. Refactoring the
> latter seems out of scope a little to me for this CL.
> 
> servolk@: I'll take another look at your most recent update later today. I was
> pending answers hopefully from hubbe@, too.

I'll need to consider further the question around 'progress' event w.r.t. MSE.
I'd also like to sync w/hubbe@ around my questions. I'll setup a sync soon.

[wolenetz, 2016-01-14 19:25:53]

Changes look good so far % a couple persistent nits and % discussion with
hubbe@.

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

https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:28: // becomes evicted, e.g. due to MSE GC algorithm).
On 2016/01/06 19:28:40, wolenetz wrote:
> nit: or by explicit removal of ranges directed by MSE web app.

ping on this nit.

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

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline.h#n...
media/base/pipeline.h:353: // DidLoadingProgress().
On 2016/01/06 21:05:05, servolk wrote:
> On 2016/01/06 19:28:41, wolenetz wrote:
> > I'm not sure this has the same semantic meaning. If MSE web app explicitly
> > removes some previously buffered range(s), does that truly mean loading has
> > progressed?
> 
> I assumed that the HTMLMediaElement 'progress' event was a notification for
the
> apps that HTMLMediaElement::buffered ranges have changed. But now I checked
the
> spec
> (http://www.w3.org/TR/2011/WD-html5-20110405/video.html#event-media-progress)
> and it says "[Progress event is dispatched when...] The user agent is fetching
> media data."
> But if we don't fire the 'progress' event here, is there an alternative way to
> notify apps that buffered ranges have changed?

Hmm. The spec also is more clear about when 'progress' is queued to be fired:
[In the resource fetch algorithm, which "Every 350ms (±200ms) or for every byte
received, whichever is least frequent, queue a task to fire a simple event named
progress at the element."
[Also,] Once the entire media resource has been fetched (but potentially before
any of it has been decoded)
Queue a task to fire a simple event named progress at the media element.

I suspect that range removal (either by MSE GC on Append or by MSE explicit
removal by app) is not ever in scope of normal HTML5 src=, hence the 'progress'
event really means just that fetching is in progress or has completed. Explicit
range removal or GC are both app-directed (the latter by append), and
'updateend' is the result of both, so the app knows if [potential] removal is
done and buffered ranges may have changed.

For Chrome MSE, any successful range removal occurs in a state where the
MediaSource is 'open' (not ended, for example), whereby the normal HTML5 src=
'progress' event is queued to fire every 350ms+/-200ms, just like what might
happen on the result of an MSE append or a src= continuing fetch. So, I think
exposing an MSE range removal (or addition; e.g. any buffered range change) as a
'didLoadingProgress == true' seems fine, since 'progress' is going to be fired
every 350+/200ms regardless until after the MediaSource has transitioned to
'ended'.

tl;dr: I've answered my original query in this thread, and I don't think any
further change here is needed at this point.

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:661: ranges.Add(base::TimeDelta::FromSeconds(0),
kDuration / 8);
On 2016/01/06 21:05:05, servolk wrote:
> On 2016/01/06 19:28:41, wolenetz wrote:
> > nit: just base::TimeDelta() is much more frequently used to represent time 0
> 
> Done.

Friendly nit ping (it's still FromSeconds(0) in patch set 5.)

[servolk, 2016-01-14 19:28:54]

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

https://codereview.chromium.org/1526303004/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:28: // becomes evicted, e.g. due to MSE GC algorithm).
On 2016/01/14 19:25:53, wolenetz wrote:
> On 2016/01/06 19:28:40, wolenetz wrote:
> > nit: or by explicit removal of ranges directed by MSE web app.
> 
> ping on this nit.

Done.

[wolenetz, 2016-01-14 19:53:06]

LGTM % one final persistent nit :)

hubbe@, servolk@ and I chatted w.r.t. the BufferedDataSource/progress questions
I had, and the net is this CL doesn't regress that behavior. Though the MSE code
path for "didLoadingProgress" remains smelly (ChunkDemuxer knows buffered
ranges, so does pipeline, and so does WMPI via pipeline, and WMPI also attempts
to use a non-MSE BufferedDataSourceHost), at least Pipeline and thereby WMPI
have access to a correct MSE buffered ranges now (w.r.t. range removals).

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/1526303004/diff/40001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:661: ranges.Add(base::TimeDelta::FromSeconds(0),
kDuration / 8);
On 2016/01/14 19:25:53, wolenetz wrote:
> On 2016/01/06 21:05:05, servolk wrote:
> > On 2016/01/06 19:28:41, wolenetz wrote:
> > > nit: just base::TimeDelta() is much more frequently used to represent time
0
> > 
> > Done.
> 
> Friendly nit ping (it's still FromSeconds(0) in patch set 5.)

Still a nit :)

[servolk, 2016-01-14 19:55:39]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-01-14 19:55:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1526303004/#ps120001
(title: "Fixed TimeDelta nit")

[commit-bot: I haz the power, 2016-01-14 19:56:29]

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

[commit-bot: I haz the power, 2016-01-14 20:17:36]

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

[commit-bot: I haz the power, 2016-01-14 20:17:37]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[servolk, 2016-01-14 20:57:59]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-01-14 20:58:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1526303004/#ps140001
(title: "Android buildfix")

[commit-bot: I haz the power, 2016-01-14 20:59:19]

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

[commit-bot: I haz the power, 2016-01-14 22:38:30]

Description was changed from

==========
Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514
==========

to

==========
Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514
==========

[commit-bot: I haz the power, 2016-01-14 22:38:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-14 22:39:58]

Description was changed from

==========
Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514
==========

to

==========
Fix buffered range updates in media::Pipeline

Currently the buffered ranges reported by
media::Pipeline::GetBufferedTimeRanges may be out of sync with the
actually buffered ranges in the demuxer, since there's no way for a
demuxer to indicate that some time range got evicted (e.g. in MSE case).
This CL replaces media::Pipeline::AddBufferedTimeRange with
OnBufferedTimeRangesChanged method that allows demuxer to report
arbitrary changes in buffered ranges.

BUG=570514

Committed: https://crrev.com/21e682ef9c2d34528aa24bf33f2e4b2734ac573b
Cr-Commit-Position: refs/heads/master@{#369585}
==========

[commit-bot: I haz the power, 2016-01-14 22:39:59]

Patchset 8 (id:??) landed as
https://crrev.com/21e682ef9c2d34528aa24bf33f2e4b2734ac573b
Cr-Commit-Position: refs/heads/master@{#369585}