Fix seeking back in the new MSE GC algorithm

media/filters/source_buffer_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/source_buffer_stream.h"
 
 #include <algorithm>
 #include <map>
 #include <sstream>
 
@@ skipping 640 matching lines @@
 
     // 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();
     }
   }
 

[wolenetz, 2015/09/15 23:19:22]

w.r.t. my comment on l.775: maybe do something like:
if (unsatisfied seek is pending prior to the last appended buffer/range), then
first collect from back (until reaching the GOP that was last appended to) and
then greedy collect from front; otherwise do the current code sequence
(non-greedy from front, normal from back).

   // 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;
   }
 
   // Try removing data from the back of the SourceBuffer, until we reach the
@@ skipping 80 matching lines @@
                "total bytes to free", total_bytes_to_free,
                "reverse direction", reverse_direction);
 
   DCHECK_GT(total_bytes_to_free, 0u);
   size_t bytes_freed = 0;
 
   // This range will save the last GOP appended to |range_for_next_append_|
   // if the buffers surrounding it get deleted during garbage collection.
   SourceBufferRange* new_range_for_append = NULL;
 
+  // If media_time is before the first buffered range, that means we are in the

[wolenetz, 2015/09/15 22:36:13]

nit:s/media_time/|media_time|/

[servolk, 2015/09/16 00:28:08]

Done.

+  // process of seeking back in the stream. HTMLMediaElement adjusts media_time

[wolenetz, 2015/09/15 22:36:13]

nit: s/HTMLMediaElement/caller/
nit: s/media_time/|media_time|/

[servolk, 2015/09/16 00:28:08]

Done.

+  // as soon as seeking is started. In those cases we can allow garbage
+  // collection to delete data from the front, since the player will append new
+  // data at the seek target position.
+  if (!ranges_.empty() &&
+      ranges_.front()->GetStartTimestamp() > media_time) {
+    DVLOG(5) << "media_time (" << media_time.InSecondsF() << ") is earlier than"
+             << " start of buffered data ("
+             << ranges_.front()->GetStartTimestamp().InSecondsF()
+             << "), using media_time="
+             << ranges_.front()->GetEndTimestamp().InSecondsF();
+    media_time = ranges_.front()->GetEndTimestamp();

[wolenetz, 2015/09/15 22:36:12]

nit: While this looks like it would work, could the change be more focused on a
different condition in l.795 to help maintainability? I tried briefly to
contrive such a condition, but it ended up being more complex. WDYT?

[servolk, 2015/09/16 00:28:08]

Acknowledged. I've reworked this as we discussed over hangouts.

+  }

[wolenetz, 2015/09/15 23:04:51]

As discussed further in chat, it seems we should allow collection (from front)
of as much as is needed, and not stop at the end of the first buffered range
(there could be many). Also, updating media_time here smells funny (I think to
both of us). Please change this logic to instead set some boolean like
"greedy_collect_from_front" true and use that to prevent stopping the collection
in l.795.

Yes, we could collect from the most recently appended range, including the most
recent append. Given that there might be a distinct collectable range after the
most recently appended-to range, in this case, we should collect from back first
(until last appended buffer), and then greedily collect from front? (I know the
caller determines the sequence of collect from front/back/etc, so the caller
might need some similar conditioning.)

[servolk, 2015/09/16 00:28:08]

Done in the lastest patchset.

+

[wolenetz, 2015/09/15 22:36:12]

Please add a unit test that verifies this works as intended.
Especially, if modifying l.795, make sure that the invariant that media_time
isn't changed after l.775 is held true. (Keeping to that invariant was what
complicated my attempt to construct a condition localized in just l.795).

[servolk, 2015/09/16 00:28:08]

I've added a ChunkDemuxer unit tests that verifies that GC succeeds when seeking
to a position that is earlier than any buffered ranges.

   while (!ranges_.empty() && bytes_freed < total_bytes_to_free) {
     SourceBufferRange* current_range = NULL;
     BufferQueue buffers;
     size_t bytes_deleted = 0;
 
     if (reverse_direction) {
       current_range = ranges_.back();
       DVLOG(5) << "current_range=" << RangeToString(*current_range);
       if (current_range->LastGOPContainsNextBufferPosition()) {
         DCHECK_EQ(current_range, selected_range_);
@@ skipping 959 matching lines @@
     return false;
 
   DCHECK_NE(have_splice_buffers, have_preroll_buffer);
   splice_buffers_index_ = 0;
   pending_buffer_.swap(*out_buffer);
   pending_buffers_complete_ = false;
   return true;
 }
 
 }  // namespace media