Fix seeking back in the new MSE GC algorithm

media/filters/source_buffer_stream.cc

Index: media/filters/source_buffer_stream.cc
diff --git a/media/filters/source_buffer_stream.cc b/media/filters/source_buffer_stream.cc
index e17a4f1c91952b64cf280a9777404e4adeec71d2..30439522c5669a5477c42b87a91dc5254a501f6b 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -34,6 +34,9 @@ const int kMaxSpliceGenerationSuccessLogs = 20;
 // Limit the number of MEDIA_LOG() logs for track buffer time gaps.
 const int kMaxTrackBufferGapWarningLogs = 20;
 
+// Limit the number of MEDIA_LOG() logs for MSE GC algorithm warnings.
+const int kMaxGarbageCollectAlgorithWarningLogs = 50;

[wolenetz, 2015/09/21 20:14:39]

nit: 50 is probably too high. In general, GC failure should be allowed for, so
we don't want to allow repeated such failures to exhaust the media internals
event cache.

so, s/50/20/ please.

[servolk, 2015/09/21 21:09:57]

Done.

+
 // Helper method that returns true if |ranges| is sorted in increasing order,
 // false otherwise.
 bool IsRangeListSorted(const std::list<media::SourceBufferRange*>& ranges) {
@@ -619,8 +622,13 @@ bool SourceBufferStream::GarbageCollectIfNeeded(DecodeTimestamp media_time,
 
   // Sanity and overflow checks
   if ((newDataSize > memory_limit_) ||
-      (ranges_size + newDataSize < ranges_size))
+      (ranges_size + newDataSize < ranges_size)) {
+    LIMITED_MEDIA_LOG(DEBUG, media_log_, num_garbage_collect_algorithm_logs_,
+                      kMaxGarbageCollectAlgorithWarningLogs)
+        << "GarbageCollectIfNeeded failed: memory_limit_=" << memory_limit_

[wolenetz, 2015/09/21 20:14:39]

nit: since this is going to chrome://media-internals, let's help developers out
by letting them know at least the type of the stream (audio, video, ...) in the
log message, as well as a little more plainly state the problem. Something like

"New [audio,video,text] append of size [x] bytes exceeds buffering capacity of
[y] bytes. Previously buffered size was [z] bytes." 

[servolk, 2015/09/21 21:09:57]

Done.

+        << " ranges_size=" << ranges_size << " newDataSize=" << newDataSize;
     return false;
+  }
 
   // Return if we're under or at the memory limit.
   if (ranges_size + newDataSize <= memory_limit_)
@@ -628,9 +636,14 @@ bool SourceBufferStream::GarbageCollectIfNeeded(DecodeTimestamp media_time,
 
   size_t bytes_to_free = ranges_size + newDataSize - memory_limit_;
 
+  // If there is a pending seek, then we should allow GC to remove data more
+  // aggressively for the upcoming append to allow seeking to proceed.
+  bool seek_pending = IsSeekPending();

[wolenetz, 2015/09/21 20:14:39]

IIUC, IsSeekPending() isn't really what we want: it allows for edge cases
related to EOS to filter the |seek_pending_| state. Also, "seek_pending" looks
too much like "seek_pending_" but has different meaning.
Finally, IIUC, Blink always unmarks EOS prior to reaching this GC code path, so
|seek_pending_| should be exactly what we want to use here.
So, please use |seek_pending_| instead of a local, and also add a
DCHECK(!end_of_stream_); here (perhaps with a clarifying comment why...) 

[servolk, 2015/09/21 21:09:57]

Done.

+
   DVLOG(2) << __FUNCTION__ << " " << GetStreamTypeName() << ": Before GC"
            << " media_time=" << media_time.InSecondsF()
            << " ranges_=" << RangesToString(ranges_)
+           << " seek_pending=" << seek_pending
            << " ranges_size=" << ranges_size
            << " newDataSize=" << newDataSize
            << " memory_limit_=" << memory_limit_
@@ -649,22 +662,59 @@ bool SourceBufferStream::GarbageCollectIfNeeded(DecodeTimestamp media_time,
              << " ranges_=" << RangesToString(ranges_);
     bytes_freed += between;
 
-    // If the last append happened before the current playback position
-    // |media_time|, then JS player is probably preparing to seek back and we
-    // should try to preserve all most recently appended data (which is in
-    // range_for_next_append_) from being removed by GC (see crbug.com/440173)
+    // Some players start appending data at the new seek target position before
+    // actually initiating the seek operation (i.e. they try to improve seek
+    // performance by prebuffering some data at the seek target position and
+    // initiating seek once enough data is pre-buffered. In those cases we'll
+    // see that data is being appended at some new position, but there is no
+    // pending seek reported yet. In this situation we need to try preserving
+    // the most recently appended data, i.e. data belonging to the same buffered
+    // range as the most recent append.
     if (range_for_next_append_ != ranges_.end()) {
       DCHECK((*range_for_next_append_)->GetStartTimestamp() <= media_time);
       media_time = (*range_for_next_append_)->GetStartTimestamp();
     }
   }
 
+  // If there is an unsatisfied pending seek, we can safely remove all data that
+  // is earlier than seek target, then remove from the back until we reach the
+  // most recently appended GOP and then remove from the front if we still don't
+  // have enough space for the upcoming append.
+  if (bytes_freed < bytes_to_free && seek_pending) {
+    DCHECK(!ranges_.empty());
+    // All data earlier than the seek target |media_time| can be removed safely
+    size_t front = FreeBuffers(bytes_to_free - bytes_freed, media_time, false);
+    DVLOG(3) << __FUNCTION__ << " Removed " << front << " bytes from the"
+             << " front. ranges_=" << RangesToString(ranges_);
+    bytes_freed += front;
+
+    // If removing data earlier than |media_time| didn't free up enough space,
+    // then try deleting from the back until we reach most recently appended GOP
+    if (bytes_freed < bytes_to_free) {
+      size_t back = FreeBuffers(bytes_to_free - bytes_freed, media_time, true);
+      DVLOG(3) << __FUNCTION__ << " Removed " << back << " bytes from the back"
+               << " ranges_=" << RangesToString(ranges_);
+      bytes_freed += back;
+    }
+
+    // If even that wasn't enough, then try greedily deleting from the front,
+    // that should allow us to remove as much data as necessary to succeed.
+    if (bytes_freed < bytes_to_free) {
+      size_t front2 = FreeBuffers(bytes_to_free - bytes_freed,
+                                  ranges_.back()->GetEndTimestamp(), false);
+      DVLOG(3) << __FUNCTION__ << " Removed " << front << " bytes from the"
+               << " front. ranges_=" << RangesToString(ranges_);
+      bytes_freed += front2;
+    }
+    DCHECK(bytes_freed >= bytes_to_free);
+  }
+
   // Try removing data from the front of the SourceBuffer up to |media_time|
   // position.
   if (bytes_freed < bytes_to_free) {
     size_t front = FreeBuffers(bytes_to_free - bytes_freed, media_time, false);
-    DVLOG(3) << __FUNCTION__ << " Removed " << front << " bytes from the front"
-             << " ranges_=" << RangesToString(ranges_);
+    DVLOG(3) << __FUNCTION__ << " Removed " << front << " bytes from the"
+             << " front. ranges_=" << RangesToString(ranges_);
     bytes_freed += front;
   }
 
@@ -672,7 +722,7 @@ bool SourceBufferStream::GarbageCollectIfNeeded(DecodeTimestamp media_time,
   // most recent append position.
   if (bytes_freed < bytes_to_free) {
     size_t back = FreeBuffers(bytes_to_free - bytes_freed, media_time, true);
-    DVLOG(3) << __FUNCTION__ << " Removed " << back << " bytes from the back"
+    DVLOG(3) << __FUNCTION__ << " Removed " << back << " bytes from the back."
              << " ranges_=" << RangesToString(ranges_);
     bytes_freed += back;
   }