MSE - Fix crash caused by incorrect GC of GOP with next buffer position

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 619ff47ab7d4c54a1dcc070384c3ab0bfcfe5c70..8187fa90ecfb297b7599699460594f6e7eb820a9 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -730,7 +730,7 @@ bool SourceBufferStream::GarbageCollectIfNeeded(DecodeTimestamp media_time,
     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"
+      DVLOG(3) << __FUNCTION__ << " Removed " << front2 << " bytes from the"
                << " front. ranges_=" << RangesToString(ranges_);
       bytes_freed += front2;
     }
@@ -855,13 +855,17 @@ size_t SourceBufferStream::FreeBuffers(size_t total_bytes_to_free,
     } else {
       current_range = ranges_.front();
       DVLOG(5) << "current_range=" << RangeToString(*current_range);
-      if (!current_range->FirstGOPEarlierThanMediaTime(media_time)) {
+      if (!current_range->FirstGOPEarlierThanMediaTime(media_time) ||
+          current_range->FirstGOPContainsNextBufferPosition()) {

[wolenetz, 2016/02/13 01:35:07]

This is the core of the fix...

[servolk, 2016/02/16 19:28:38]

Have you been able to figure out exactly how this happens? TBH I don't
understand how that's possible. I think media_time (the current playback
position on the timeline) should be always <= next_buffer_position (the demuxer
read position). So having this additional check here shouldn't change anything,
imho. Perhaps we should also add a dcheck here that media_time <=
next_buffer_position?

[wolenetz, 2016/02/16 20:12:47]

Yes. I've confirmed at least one way this happens via log inspection and a unit
test (see the new unit test in this CL).
tl;dr: have some later buffered range than the current range. Seek to a time
within the duration of the very last frame in the current range, but after the
DTS of that frame. Garbage collect that range from the beginning. BOOM.
This is also why I added the "alternatively..." CR comment in this CL's earlier
patch set 1 at SBR::FirstGOPEarlierThanMediaTime().

[servolk, 2016/02/16 23:11:24]

ok, but this requires seeking to a very precise position (within a duration of
the video frame!) to cause any problems, so I think it's unlikely to be the root
cause of the crashes being investigated, although I agree we can harden things
here as well.

[chcunningham, 2016/02/16 23:36:25]

After chatting f2f we settled on doing the "alternative" fix as well, as we
agree FirstGOPEarlierThanMediaTime is not quite correct if it fails to
considered the duration of the last frame. 

[wolenetz, 2016/02/16 23:37:55]

Acknowledged. Unfortunately, I have been hitting this and only this in my
attempts to repro the crashes using the steps from the internal b/. I've changed
some of SourceBufferRange's DCHECKs to CHECKs temporarily already
(https://codereview.chromium.org/1673293003/), and may do more of the same if we
continue to see crashes in SourceBufferRange after this fix has landed.

I suspect that the repro involves or is exacerbated by apps clamping seeks to
end of buffered ranges (either by interception of seeks or by doing some
explicit remove starting at the seek target time prior to appending data to
satisfy the seek there and invoking the implicit GC). Note that buffered end
time of a range is what is visible to apps, and that is very likely beyond the
last DTS of the last coded frame in the range.

         // 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);
+      DVLOG(4) << "Deleting GOP from front: " << RangeToString(*current_range)
+               << ", media_time: " << media_time.InMicroseconds()
+               << ", current_range->HasNextBufferPosition(): "
+               << current_range->HasNextBufferPosition();
       bytes_deleted = current_range->DeleteGOPFromFront(&buffers);
     }