Implement evictFrames() to support MSE's coded frame eviction algorithm.

Implement evictFrames() to support MSE's coded frame eviction algorithm.

s/WebKit/Blink/ Implemented WebSourceBufferImpl::evictFrames() to call SourceBuffer::GarbageCollectIfNeeded()

Also use playback time in the MSE garbage collection:

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.

[kjoswiak, 2014-12-11 23:25:31]

kjoswiak@chromium.org changed reviewers:
+ damienv@chromium.org, philipj@opera.com, wolenetz@google.com

[kjoswiak, 2014-12-11 23:25:31]

Combination of prior CLs https://codereview.chromium.org/791723003/ and
https://codereview.chromium.org/789983003/. A more comprehensive rework of the
garbage collection pathway, removing call to GC during append loop and
delegating it solely to the new call to EvictFrames from Webkit. Removes need
for SourceBufferStream to track media time.

Currently I still need to throw together some tests, and to investigate one
potential issue wherein without GC during append loop, really we're only freeing
enough space to get buffer bellow capacity, and not to ensure we can fit the new
append.

Also see: https://codereview.chromium.org/785343002/

[kjoswiak, 2014-12-11 23:26:04]

kjoswiak@chromium.org changed reviewers:
+ wolenetz@chromium.org
- wolenetz@google.com

[kjoswiak, 2014-12-11 23:30:24]

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

[servolk, 2014-12-12 01:01:53]

We'll need to look into adding more unit tests for GC in SBS, covering this
situation, and possibly some others (e.g. possibly a situation when GC from the
back of the buffer is triggered when seeking to a position before the current
playback time)

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (left):

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:612: bytes_freed +=
FreeBuffers(bytes_to_free - bytes_freed, true);
So now we are never gonna to try freeing buffers from the back? Is it safe? What
if we are playing at the end of a very long video and then seek back to
beginning. I think in the past GC would remove the buffers at the back of the
SBS - how is it gonna be handled now?

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:612: return (bytes_to_free - bytes_freed
<= 0);
Why not just "return bytes_freed >= bytes_to_free" ? I think that would be more
readable.

[kjoswiak, 2014-12-12 01:22:05]

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (left):

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:612: bytes_freed +=
FreeBuffers(bytes_to_free - bytes_freed, true);
On 2014/12/12 01:01:53, servolk wrote:
> So now we are never gonna to try freeing buffers from the back? Is it safe?
What
> if we are playing at the end of a very long video and then seek back to
> beginning. I think in the past GC would remove the buffers at the back of the
> SBS - how is it gonna be handled now?

The call to FreeBuffersAfterLastAppended on line 604 should take care of extra
buffers at end of our ranges.

This call removes buffers between current playhead and last append, which is
going to create a gap in the buffer. This is the source of gaps that have been
causing the playback stall bugs, since most clients are not going to expect
they're going to have to reappend data in this range.

I can put this call back in if I'm misunderstanding the situation, but I think
this is the source of GC over aggression, and would prefer we fail rather than
try this call.

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:612: return (bytes_to_free - bytes_freed
<= 0);
On 2014/12/12 01:01:53, servolk wrote:
> Why not just "return bytes_freed >= bytes_to_free" ? I think that would be
more
> readable.

Done.

[servolk, 2015-03-12 18:23:38]

On 2014/12/12 01:22:05, kjoswiak wrote:
>
https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
> File media/filters/source_buffer_stream.cc (left):
> 
>
https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
> media/filters/source_buffer_stream.cc:612: bytes_freed +=
> FreeBuffers(bytes_to_free - bytes_freed, true);
> On 2014/12/12 01:01:53, servolk wrote:
> > So now we are never gonna to try freeing buffers from the back? Is it safe?
> What
> > if we are playing at the end of a very long video and then seek back to
> > beginning. I think in the past GC would remove the buffers at the back of
the
> > SBS - how is it gonna be handled now?
> 
> The call to FreeBuffersAfterLastAppended on line 604 should take care of extra
> buffers at end of our ranges.
> 
> This call removes buffers between current playhead and last append, which is
> going to create a gap in the buffer. This is the source of gaps that have been
> causing the playback stall bugs, since most clients are not going to expect
> they're going to have to reappend data in this range.
> 
> I can put this call back in if I'm misunderstanding the situation, but I think
> this is the source of GC over aggression, and would prefer we fail rather than
> try this call.
> 
>
https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
> File media/filters/source_buffer_stream.cc (right):
> 
>
https://codereview.chromium.org/794343003/diff/1/media/filters/source_buffer_...
> media/filters/source_buffer_stream.cc:612: return (bytes_to_free - bytes_freed
> <= 0);
> On 2014/12/12 01:01:53, servolk wrote:
> > Why not just "return bytes_freed >= bytes_to_free" ? I think that would be
> more
> > readable.
> 
> Done.

Kyle has left Google, continuing this work in a different CL, we still want this
to be fixed in Chromium media pipeline:
https://codereview.chromium.org/1008463002/