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

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 857bb9790cddedbca8ce0a9c88e31498b0e9746c..5bd6d92c7eb543ea56b6fc60713a2f9de643ea4e 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -401,8 +401,6 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
 
   SetSelectedRangeIfNeeded(next_buffer_timestamp);
 
-  GarbageCollectIfNeeded();
-
   DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
            << ": done. ranges_=" << RangesToString(ranges_);
   DCHECK(IsRangeListSorted(ranges_));
@@ -651,47 +649,82 @@ void SourceBufferStream::SetConfigIds(const BufferQueue& buffers) {
   }
 }
 
-void SourceBufferStream::GarbageCollectIfNeeded() {
+bool SourceBufferStream::GarbageCollectIfNeeded(DecodeTimestamp media_time,
+                                                size_t newDataSize) {
+  DCHECK(media_time != kNoDecodeTimestamp());
   // Compute size of |ranges_|.
   size_t ranges_size = 0;
   for (RangeList::iterator itr = ranges_.begin(); itr != ranges_.end(); ++itr)

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

instead, use GetBufferedSize() ?

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

Done.

     ranges_size += (*itr)->size_in_bytes();
 
   // Return if we're under or at the memory limit.
-  if (ranges_size <= memory_limit_)
-    return;
+  if (ranges_size + newDataSize <= memory_limit_)

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

this can overflow

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

Done, added a sanity and overflow check

+    return true;
 
-  size_t bytes_to_free = ranges_size - memory_limit_;
+  size_t bytes_to_free = ranges_size + newDataSize - memory_limit_;

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

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

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

Done.

 
   DVLOG(2) << __FUNCTION__ << " " << GetStreamTypeName() << ": Before GC"
-           << " ranges_size=" << ranges_size
+           << " media_time=" << media_time.InSecondsF()
            << " ranges_=" << RangesToString(ranges_)
-           << " memory_limit_=" << memory_limit_;
+           << " ranges_size=" << ranges_size
+           << " newDataSize=" << newDataSize
+           << " memory_limit_=" << memory_limit_
+           << " last_appended_buffer_timestamp_="
+           << last_appended_buffer_timestamp_.InSecondsF();
+
+  size_t bytes_freed = 0;
 
-  // Begin deleting after the last appended buffer.
-  size_t bytes_freed = FreeBuffersAfterLastAppended(bytes_to_free);
+  // If last appended buffer position was earlier than the current playback time
+  // then try deleting data between last append and current media_time.
+  if (last_appended_buffer_timestamp_ != kNoDecodeTimestamp() &&
+      last_appended_buffer_timestamp_ < media_time) {
+    size_t between = FreeBuffersAfterLastAppended(bytes_to_free, media_time);
+    DVLOG(3) << __FUNCTION__ << " FreeBuffersAfterLastAppended "
+             << " released " << between << " bytes"
+             << " 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)
+    if (range_for_next_append_ != ranges_.end()) {
+      DCHECK((*range_for_next_append_)->GetStartTimestamp() <= media_time);
+      media_time = (*range_for_next_append_)->GetStartTimestamp();
+    }
+  }
 
-  // Begin deleting from the front.
-  if (bytes_freed < bytes_to_free)
-    bytes_freed += FreeBuffers(bytes_to_free - bytes_freed, false);
+  // 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_);
+    bytes_freed += front;
+  }
 
-  // Begin deleting from the back.
-  if (bytes_freed < bytes_to_free)
-    bytes_freed += FreeBuffers(bytes_to_free - bytes_freed, true);
+  // Try removing data from the back of the SourceBuffer, until we reach the
+  // 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"
+             << " ranges_=" << RangesToString(ranges_);
+    bytes_freed += back;
+  }
 
   DVLOG(2) << __FUNCTION__ << " " << GetStreamTypeName() << ": After GC"
+           << " bytes_to_free=" << bytes_to_free
            << " bytes_freed=" << bytes_freed
            << " ranges_=" << RangesToString(ranges_);
+
+  return bytes_freed >= bytes_to_free;
 }
 
 size_t SourceBufferStream::FreeBuffersAfterLastAppended(
-    size_t total_bytes_to_free) {
-  DecodeTimestamp next_buffer_timestamp = GetNextBufferTimestamp();
-  if (last_appended_buffer_timestamp_ == kNoDecodeTimestamp() ||
-      next_buffer_timestamp == kNoDecodeTimestamp() ||
-      last_appended_buffer_timestamp_ >= next_buffer_timestamp) {
-    return 0;
-  }
+    size_t total_bytes_to_free, DecodeTimestamp media_time) {
+  DVLOG(4) << __FUNCTION__ << " last_appended_buffer_timestamp_="
+            << last_appended_buffer_timestamp_.InSecondsF()
+            << " media_time=" << media_time.InSecondsF();
 
   DecodeTimestamp remove_range_start = last_appended_buffer_timestamp_;
   if (last_appended_buffer_is_keyframe_)
@@ -701,18 +734,21 @@ size_t SourceBufferStream::FreeBuffersAfterLastAppended(
       remove_range_start);
   if (remove_range_start_keyframe != kNoDecodeTimestamp())
     remove_range_start = remove_range_start_keyframe;
-  if (remove_range_start >= next_buffer_timestamp)
+  if (remove_range_start >= media_time)
     return 0;
 
   DecodeTimestamp remove_range_end;
   size_t bytes_freed = GetRemovalRange(remove_range_start,
-                                       next_buffer_timestamp,
+                                       media_time,
                                        total_bytes_to_free,
                                        &remove_range_end);
   if (bytes_freed > 0) {
+    DVLOG(4) << __FUNCTION__ << " removing ["
+             << remove_range_start.ToPresentationTime().InSecondsF() << ";"
+             << remove_range_end.ToPresentationTime().InSecondsF() << "]";
     Remove(remove_range_start.ToPresentationTime(),
            remove_range_end.ToPresentationTime(),
-           next_buffer_timestamp.ToPresentationTime());
+           media_time.ToPresentationTime());
   }
 
   return bytes_freed;
@@ -745,6 +781,7 @@ size_t SourceBufferStream::GetRemovalRange(
 }
 
 size_t SourceBufferStream::FreeBuffers(size_t total_bytes_to_free,
+                                       DecodeTimestamp media_time,
                                        bool reverse_direction) {
   TRACE_EVENT2("media", "SourceBufferStream::FreeBuffers",
                "total bytes to free", total_bytes_to_free,
@@ -764,17 +801,24 @@ size_t SourceBufferStream::FreeBuffers(size_t total_bytes_to_free,
 
     if (reverse_direction) {
       current_range = ranges_.back();
+      DVLOG(5) << "current_range=" << RangeToString(*current_range);
       if (current_range->LastGOPContainsNextBufferPosition()) {
         DCHECK_EQ(current_range, selected_range_);
+        DVLOG(5) << "current_range contains next read position, stopping GC";
         break;
       }
+      DVLOG(5) << "Deleting GOP from back: " << RangeToString(*current_range);
       bytes_deleted = current_range->DeleteGOPFromBack(&buffers);
     } else {
       current_range = ranges_.front();
-      if (current_range->FirstGOPContainsNextBufferPosition()) {
-        DCHECK_EQ(current_range, selected_range_);
+      DVLOG(5) << "current_range=" << RangeToString(*current_range);
+      if (!current_range->FirstGOPEarlierThanMediaTime(media_time)) {
+        // We have removed all data up to the GOP that contains current playback
+        // position, we can't delete any further.
+        DVLOG(5) << "current_range contains playback position, stopping GC";
         break;
       }
+      DVLOG(4) << "Deleting GOP from front: " << RangeToString(*current_range);
       bytes_deleted = current_range->DeleteGOPFromFront(&buffers);
     }
 
@@ -783,6 +827,7 @@ size_t SourceBufferStream::FreeBuffers(size_t total_bytes_to_free,
     if (end_timestamp == last_appended_buffer_timestamp_) {
       DCHECK(last_appended_buffer_timestamp_ != kNoDecodeTimestamp());
       DCHECK(!new_range_for_append);
+
       // Create a new range containing these buffers.
       new_range_for_append = new SourceBufferRange(
           TypeToGapPolicy(GetType()),
@@ -801,6 +846,11 @@ size_t SourceBufferStream::FreeBuffers(size_t total_bytes_to_free,
       delete current_range;
       reverse_direction ? ranges_.pop_back() : ranges_.pop_front();
     }
+
+    if (reverse_direction && new_range_for_append) {
+      // We don't want to delete any further, or we'll be creating gaps
+      break;
+    }
   }
 
   // Insert |new_range_for_append| into |ranges_|, if applicable.
@@ -1260,8 +1310,10 @@ void SourceBufferStream::SeekAndSetSelectedRange(
 }
 
 void SourceBufferStream::SetSelectedRange(SourceBufferRange* range) {
-  DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
-           << ": " << selected_range_ << " -> " << range;
+  DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() << ": "
+           << selected_range_ << " "
+           << (selected_range_ ? RangeToString(*selected_range_) : "")
+           << " -> " << range << " " << (range ? RangeToString(*range) : "");
   if (selected_range_)
     selected_range_->ResetNextBufferPosition();
   DCHECK(!range || range->HasNextBufferPosition());
@@ -1285,6 +1337,13 @@ base::TimeDelta SourceBufferStream::GetBufferedDuration() const {
   return ranges_.back()->GetBufferedEndTimestamp().ToPresentationTime();
 }
 
+size_t SourceBufferStream::GetBufferedSize() const {
+  size_t ranges_size = 0;
+  for (const auto& range : ranges_)
+    ranges_size += range->size_in_bytes();
+  return ranges_size;
+}
+
 void SourceBufferStream::MarkEndOfStream() {
   DCHECK(!end_of_stream_);
   end_of_stream_ = true;