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

media/blink/websourcebuffer_impl.cc

Index: media/blink/websourcebuffer_impl.cc
diff --git a/media/blink/websourcebuffer_impl.cc b/media/blink/websourcebuffer_impl.cc
index 0e387785ae1db4d0bb3a64b54d7dac0d3b1100fc..256d4672df080bd783fcbfd056ad338fac26b156 100644
--- a/media/blink/websourcebuffer_impl.cc
+++ b/media/blink/websourcebuffer_impl.cc
@@ -81,6 +81,12 @@ blink::WebTimeRanges WebSourceBufferImpl::buffered() {
   return result;
 }
 
+bool WebSourceBufferImpl::evictCodedFrames(double currentPlaybackTime,
+                                           size_t newDataSize) {

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

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.

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

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]

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.

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

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

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

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."

[servolk, 2015/08/21 01:32:08]

Yes, step 3 of 3.5.14

+  return demuxer_->EvictCodedFrames(
+      base::TimeDelta::FromSecondsD(currentPlaybackTime));
+}
+
 void WebSourceBufferImpl::append(
     const unsigned char* data,
     unsigned length,