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..60d34df2b0753bf495e1027fea31384fec212ea6 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -759,6 +759,21 @@ size_t SourceBufferStream::FreeBuffers(size_t total_bytes_to_free,
   // 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;