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

media/filters/source_buffer_stream_unittest.cc

Index: media/filters/source_buffer_stream_unittest.cc
diff --git a/media/filters/source_buffer_stream_unittest.cc b/media/filters/source_buffer_stream_unittest.cc
index 6282135634b70501c5ec9d765291003808954406..745d2f7546f5c727657f8bafcc51ab74c50dcdab 100644
--- a/media/filters/source_buffer_stream_unittest.cc
+++ b/media/filters/source_buffer_stream_unittest.cc
@@ -145,6 +145,11 @@ class SourceBufferStreamTest : public testing::Test {
     stream_->Seek(base::TimeDelta::FromMilliseconds(timestamp_ms));
   }
 
+  bool GarbageCollectWithPlaybackAtBuffer(int position) {
+    return stream_->GarbageCollectIfNeeded(
+        DecodeTimestamp::FromPresentationTime(position * frame_duration_));
+  }
+
   void RemoveInMs(int start, int end, int duration) {
     Remove(base::TimeDelta::FromMilliseconds(start),
            base::TimeDelta::FromMilliseconds(end),
@@ -2361,6 +2366,7 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteFront) {
 
   // None of the buffers should trigger garbage collection, so all data should
   // be there as expected.
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(0));
   CheckExpectedRanges("{ [0,19) }");
   Seek(0);
   CheckExpectedBuffers(0, 19, &kDataA);
@@ -2371,6 +2377,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteFront) {
   // Append 5 buffers to the end of the stream.
   AppendBuffers(20, 5, &kDataA);
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(10));
+
   // GC should have deleted the first 5 buffers.
   CheckExpectedRanges("{ [5,24) }");
   CheckExpectedBuffers(10, 24, &kDataA);
@@ -2391,6 +2399,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteFrontGOPsAtATime) {
   // Add one buffer to put the memory over the cap.
   AppendBuffers(20, 1, &kDataA);
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(10));
+
   // GC should have deleted the first 5 buffers so that the range still begins
   // with a keyframe.
   CheckExpectedRanges("{ [5,20) }");
@@ -2403,14 +2413,18 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteBack) {
   // Set memory limit to 5 buffers.
   SetMemoryLimit(5);
 
-  // Seek to position 0.
-  Seek(0);
-
-  // Append 20 buffers at positions 0 through 19.
-  NewSegmentAppend(0, 20, &kDataA);
+  // Append 5 buffers at positions 15 through 19.
+  NewSegmentAppend(15, 5, &kDataA);
 
-  // Should leave the first 5 buffers from 0 to 4 and the last GOP appended.
+  // Append 5 buffers at positions 0 through 4.
+  NewSegmentAppend(0, 5, &kDataA);
   CheckExpectedRanges("{ [0,4) [15,19) }");
+
+  // Seek to position 0.
+  Seek(0);
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(0));
+  // Should leave the first 5 buffers from 0 to 4.
+  CheckExpectedRanges("{ [0,4) }");
   CheckExpectedBuffers(0, 4, &kDataA);
 }
 
@@ -2423,10 +2437,16 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteFrontAndBack) {
 
   // Append 40 buffers at positions 0 through 39.
   NewSegmentAppend(0, 40, &kDataA);
+  // GC will try to keep data between current playback position and last append
+  // position. This will ensure that the last append position is 19 and will
+  // allow GC algorithm to collect data outside of the range [15,19)
+  NewSegmentAppend(15, 5, &kDataA);
+  CheckExpectedRanges("{ [0,39) }");
 
-  // Should leave the GOP containing the seek position and the last GOP
-  // appended.
-  CheckExpectedRanges("{ [15,19) [35,39) }");
+  // Should leave the GOP containing the current playback position 15 and the
+  // last append position 19. GC returns false, since we are still above limit.
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(15));
+  CheckExpectedRanges("{ [15,19) }");
   CheckExpectedBuffers(15, 19, &kDataA);
   CheckNoNextBuffer();
 }
@@ -2441,43 +2461,49 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteSeveralRanges) {
   // Append 5 buffers at positions 20 through 24.
   NewSegmentAppend(20, 5);
 
-  // Append 5 buffers at positions 30 through 34.
-  NewSegmentAppend(30, 5);
+  // Append 5 buffers at positions 40 through 44.
+  NewSegmentAppend(40, 5);
 
-  CheckExpectedRanges("{ [0,4) [10,14) [20,24) [30,34) }");
+  CheckExpectedRanges("{ [0,4) [10,14) [20,24) [40,44) }");
 
-  // Seek to position 21.
+  // Seek to position 20.
   Seek(20);
   CheckExpectedBuffers(20, 20);
 
   // Set memory limit to 1 buffer.
   SetMemoryLimit(1);
 
-  // Append 5 buffers at positions 40 through 44. This will trigger GC.
-  NewSegmentAppend(40, 5);
+  // Append 5 buffers at positions 30 through 34.
+  NewSegmentAppend(30, 5);
 
-  // Should delete everything except the GOP containing the current buffer and
-  // the last GOP appended.
-  CheckExpectedRanges("{ [20,24) [40,44) }");
+  // We will have more than 1 buffer left, GC will fail
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(20));
+
+  // Should have deleted all buffer ranges before the current buffer and after
+  // last GOP
+  CheckExpectedRanges("{ [20,24) [30,34) }");
   CheckExpectedBuffers(21, 24);
   CheckNoNextBuffer();
 
   // Continue appending into the last range to make sure it didn't break.
-  AppendBuffers(45, 10);
-  // Should only save last GOP appended.
-  CheckExpectedRanges("{ [20,24) [50,54) }");
+  AppendBuffers(35, 10);
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(20));
+  // Should save everything between read head and last appended
+  CheckExpectedRanges("{ [20,24) [30,44) }");
 
   // Make sure appending before and after the ranges didn't somehow break.
   SetMemoryLimit(100);
   NewSegmentAppend(0, 10);
-  CheckExpectedRanges("{ [0,9) [20,24) [50,54) }");
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(20));
+  CheckExpectedRanges("{ [0,9) [20,24) [30,44) }");
   Seek(0);
   CheckExpectedBuffers(0, 9);
 
   NewSegmentAppend(90, 10);
-  CheckExpectedRanges("{ [0,9) [20,24) [50,54) [90,99) }");
-  Seek(50);
-  CheckExpectedBuffers(50, 54);
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(0));
+  CheckExpectedRanges("{ [0,9) [20,24) [30,44) [90,99) }");
+  Seek(30);
+  CheckExpectedBuffers(30, 44);
   CheckNoNextBuffer();
   Seek(90);
   CheckExpectedBuffers(90, 99);
@@ -2494,6 +2520,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteAfterLastAppend) {
   // Append 2 GOPs starting at 490ms, 30ms apart.
   NewSegmentAppend("490K 520 550 580K 610 640");
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(0));
+
   CheckExpectedRangesByTimestamp("{ [310,400) [490,670) }");
 
   // Seek to the GOP at 580ms.
@@ -2503,6 +2531,9 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteAfterLastAppend) {
   // So the ranges before GC are "{ [100,280) [310,400) [490,670) }".
   NewSegmentAppend("100K 130 160 190K 220 250K");
 
+  EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(580)));
+
   // Should save the newly appended GOPs.
   CheckExpectedRangesByTimestamp("{ [100,280) [580,670) }");
 }
@@ -2521,6 +2552,9 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_DeleteAfterLastAppendMerged) {
   // range.  So the range before GC is "{ [220,670) }".
   NewSegmentAppend("220K 250 280 310K 340 370");
 
+  EXPECT_TRUE(stream_->GarbageCollectIfNeeded(

[wolenetz, 2015/08/12 21:09:57]

I suspect it'll be not-obvious to later maintainers why we GC after, not before,
the appends in some of these test cases. To me, it's clear, especially in the
context of *this* change. Do the tests need some refactoring?

[servolk, 2015/08/13 00:31:42]

Yes, I think I'll add a comment that we are skipping the GarbageCollectIfNeeded
invocation before the append, because we know buffer is empty at that point and
GC will be a no-op anyway, and then we do the GC after append, since GC would
have to happen before the next append anyway. Is that good enough?
It's true those test could use some refactoring, but I'm not sure how to do it
best. Perhaps I should introduce GCIfNeeded invocation inside test methods like
NewSegmentAppend to simulate more closely how the actual SB::appendBuffer works.
Or perhaps these test should all be done on the blink level, where we could use
the same SB API as JS apps use?

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

Best solution for now, ISTM, would be to add a comment in the private
AppendBuffers(...) that the expected GC-prior-to-append behavior is separated
out into the tests to more precisely test the append and GC operations
independently.

+      DecodeTimestamp::FromMilliseconds(580)));
+
   // Should save the newly appended GOPs.
   CheckExpectedRangesByTimestamp("{ [220,400) [580,670) }");
 }
@@ -2532,7 +2566,13 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_NoSeek) {
   // Append 25 buffers at positions 0 through 24.
   NewSegmentAppend(0, 25, &kDataA);
 
-  // GC deletes the first 5 buffers to keep the memory limit within cap.
+  // If playback is still in the first GOP (starting at 0), GC should fail.
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(2));
+  CheckExpectedRanges("{ [0,24) }");
+
+  // As soon as playback position moves past the first GOP, it should be removed
+  // and after removing the first GOP we are under memory limit.
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(5));
   CheckExpectedRanges("{ [5,24) }");
   CheckNoNextBuffer();
   Seek(5);
@@ -2550,6 +2590,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_PendingSeek) {
   Seek(15);
   CheckNoNextBuffer();
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(0));
+
   CheckExpectedRanges("{ [0,9) [25,29) }");
 
   // Set memory limit to 5 buffers.
@@ -2558,6 +2600,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_PendingSeek) {
   // Append 5 buffers as positions 30 to 34 to trigger GC.
   AppendBuffers(30, 5, &kDataA);
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(30));
+
   // The current algorithm will delete from the beginning until the memory is
   // under cap.
   CheckExpectedRanges("{ [30,34) }");
@@ -2568,6 +2612,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_PendingSeek) {
   // Append data to fulfill seek.
   NewSegmentAppend(15, 5, &kDataA);
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(30));
+
   // Check to make sure all is well.
   CheckExpectedRanges("{ [15,19) [30,34) }");
   CheckExpectedBuffers(15, 19, &kDataA);
@@ -2584,22 +2630,27 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_NeedsMoreData) {
 
   // Advance next buffer position to 10.
   Seek(0);
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(0));
   CheckExpectedBuffers(0, 9, &kDataA);
   CheckNoNextBuffer();
 
   // Append 20 buffers at positions 15 through 34.
   NewSegmentAppend(15, 20, &kDataA);
+  CheckExpectedRanges("{ [0,9) [15,34) }");
 
-  // GC should have saved the keyframe before the current seek position and the
-  // data closest to the current seek position. It will also save the last GOP
-  // appended.
-  CheckExpectedRanges("{ [5,9) [15,19) [30,34) }");
+  // GC should save the keyframe before the next buffer position and the data
+  // closest to the next buffer position. It will also save all buffers from
+  // next buffer to the last GOP appended, which overflows limit and leads to
+  // failure.
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(5));
+  CheckExpectedRanges("{ [5,9) [15,34) }");
 
   // Now fulfill the seek at position 10. This will make GC delete the data
   // before position 10 to keep it within cap.
   NewSegmentAppend(10, 5, &kDataA);
-  CheckExpectedRanges("{ [10,19) [30,34) }");
-  CheckExpectedBuffers(10, 19, &kDataA);
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(10));
+  CheckExpectedRanges("{ [10,24) }");
+  CheckExpectedBuffers(10, 24, &kDataA);
 }
 
 TEST_F(SourceBufferStreamTest, GarbageCollection_TrackBuffer) {
@@ -2612,15 +2663,21 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_TrackBuffer) {
   // Append 18 buffers at positions 0 through 17.
   NewSegmentAppend(0, 18, &kDataA);
 
+  EXPECT_TRUE(GarbageCollectWithPlaybackAtBuffer(15));
+
   // Should leave GOP containing seek position.
   CheckExpectedRanges("{ [15,17) }");
 
-  // Seek ahead to position 16.
+  // Move next buffer position to 16.
   CheckExpectedBuffers(15, 15, &kDataA);
 
   // Completely overlap the existing buffers.
   NewSegmentAppend(0, 20, &kDataB);
 
+  // Final GOP [15,19) contains 5 buffers, which is more than memory limit of
+  // 3 buffers set at the beginning of the test, so GC will fail.
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(15));
+
   // Because buffers 16 and 17 are not keyframes, they are moved to the track
   // buffer upon overlap. The source buffer (i.e. not the track buffer) is now
   // waiting for the next keyframe.
@@ -2631,6 +2688,9 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_TrackBuffer) {
   // Now add a keyframe at position 20.
   AppendBuffers(20, 5, &kDataB);
 
+  // 5 buffers in final GOP, GC will fail
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(20));
+
   // Should garbage collect such that there are 5 frames remaining, starting at
   // the keyframe.
   CheckExpectedRanges("{ [20,24) }");
@@ -2638,39 +2698,100 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_TrackBuffer) {
   CheckNoNextBuffer();
 }
 
+// Test GC preserves data starting at first GOP containing playback position.
+TEST_F(SourceBufferStreamTest, GarbageCollection_SaveDataAtPlaybackPosition) {
+  // Set memory limit to 30 buffers = 1 second of data.
+  SetMemoryLimit(30);
+  // And append 300 buffers = 10 seconds of data.
+  NewSegmentAppend(0, 300, &kDataA);
+  CheckExpectedRanges("{ [0,299) }");
+
+  // Playback position at 0, all data must be preserved.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(0)));
+  CheckExpectedRanges("{ [0,299) }");
+
+  // Playback position at 1 sec, the first second of data [0,29) should be
+  // collected, since we are way over memory limit.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(1000)));
+  CheckExpectedRanges("{ [30,299) }");
+
+  // Playback position at 1.1 sec, no new data can be collected, since the
+  // playback position is still in the first GOP of buffered data.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(1100)));
+  CheckExpectedRanges("{ [30,299) }");
+
+  // Playback position at 5.166 sec, just at the very end of GOP corresponding
+  // to buffer range 150-155, which should be preserved.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(5166)));
+  CheckExpectedRanges("{ [150,299) }");
+
+  // Playback position at 5.167 sec, just past the end of GOP corresponding to
+  // buffer range 150-155, it should be garbage collected now.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(5167)));
+  CheckExpectedRanges("{ [155,299) }");
+
+  // Playback at 9.0 sec, we can now successfully collect all data except the
+  // last second and we are back under memory limit of 30 buffers, so GCIfNeeded
+  // should return true.
+  EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(9000)));
+  CheckExpectedRanges("{ [270,299) }");
+
+  // Playback at 9.999 sec, GC succeeds, since we are under memory limit even
+  // without removing any data.
+  EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(9999)));
+  CheckExpectedRanges("{ [270,299) }");
+
+  // Playback at 15 sec, this should never happen during regular playback in
+  // browser, since this position has no data buffered, but it should still
+  // cause no problems to GC algorithm, so test it just in case.
+  EXPECT_TRUE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(15000)));
+  CheckExpectedRanges("{ [270,299) }");
+}
+
 // Test saving the last GOP appended when this GOP is the only GOP in its range.
 TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP) {
   // Set memory limit to 3 and make sure the 4-byte GOP is not garbage
   // collected.
   SetMemoryLimit(3);
   NewSegmentAppend("0K 30 60 90");
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(0));
   CheckExpectedRangesByTimestamp("{ [0,120) }");
 
   // Make sure you can continue appending data to this GOP; again, GC should not
   // wipe out anything.
   AppendBuffers("120D30");
+  EXPECT_FALSE(GarbageCollectWithPlaybackAtBuffer(0));
   CheckExpectedRangesByTimestamp("{ [0,150) }");
 
-  // Set memory limit to 100 and append a 2nd range after this without
-  // triggering GC.
-  SetMemoryLimit(100);
+  // Append a 2nd range after this without triggering GC.
   NewSegmentAppend("200K 230 260 290K 320 350");
   CheckExpectedRangesByTimestamp("{ [0,150) [200,380) }");
 
   // Seek to 290ms.
   SeekToTimestampMs(290);
 
-  // Now set memory limit to 3 and append a GOP in a separate range after the
-  // selected range. Because it is after 290ms, this tests that the GOP is saved
-  // when deleting from the back.
-  SetMemoryLimit(3);
+  // Now append a GOP in a separate range after the selected range and trigger
+  // GC. Because it is after 290ms, this tests that the GOP is saved when
+  // deleting from the back.
   NewSegmentAppend("500K 530 560 590");
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(290)));
 
-  // Should save GOP with 290ms and last GOP appended.
+  // Should save GOPs between 290ms and the last GOP appended.
   CheckExpectedRangesByTimestamp("{ [290,380) [500,620) }");
 
   // Continue appending to this GOP after GC.
   AppendBuffers("620D30");
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(290)));
   CheckExpectedRangesByTimestamp("{ [290,380) [500,650) }");
 }
 
@@ -2686,34 +2807,39 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Middle) {
   SetMemoryLimit(1);
   NewSegmentAppend("80K 110 140");
 
-  // This whole GOP should be saved, and should be able to continue appending
-  // data to it.
+  // This whole GOP should be saved after GC, which will fail due to GOP being
+  // larger than 1 buffer
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(80)));
   CheckExpectedRangesByTimestamp("{ [80,170) }");
+  // We should still be able to continue appending data to GOP
   AppendBuffers("170D30");
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(80)));
   CheckExpectedRangesByTimestamp("{ [80,200) }");
 
-  // Set memory limit to 100 and append a 2nd range after this without
-  // triggering GC.
-  SetMemoryLimit(100);
+  // Append a 2nd range after this range, without triggering GC.
   NewSegmentAppend("400K 430 460 490K 520 550 580K 610 640");
   CheckExpectedRangesByTimestamp("{ [80,200) [400,670) }");
 
   // Seek to 80ms to make the first range the selected range.
   SeekToTimestampMs(80);
 
-  // Now set memory limit to 3 and append a GOP in the middle of the second
-  // range. Because it is after the selected range, this tests that the GOP is
-  // saved when deleting from the back.
-  SetMemoryLimit(3);
+  // Now append a GOP in the middle of the second range and trigger GC. Because
+  // it is after the selected range, this tests that the GOP is saved when
+  // deleting from the back.
   NewSegmentAppend("500K 530 560 590");
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(80)));
 
-  // Should save the GOP containing the seek point and GOP that was last
-  // appended.
-  CheckExpectedRangesByTimestamp("{ [80,200) [500,620) }");
+  // Should save the GOPs between the seek point and GOP that was last appended
+  CheckExpectedRangesByTimestamp("{ [80,200) [400,620) }");
 
   // Continue appending to this GOP after GC.
   AppendBuffers("620D30");
-  CheckExpectedRangesByTimestamp("{ [80,200) [500,650) }");
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(80)));
+  CheckExpectedRangesByTimestamp("{ [80,200) [400,650) }");
 }
 
 // Test saving the last GOP appended when the GOP containing the next buffer is
@@ -2730,7 +2856,10 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Selected1) {
   SetMemoryLimit(1);
   NewSegmentAppend("0K 30 60");
 
-  // Should save the GOP at 0ms and 90ms.
+  // GC should save the GOP at 0ms and 90ms, and will fail since GOP larger
+  // than 1 buffer
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(90)));
   CheckExpectedRangesByTimestamp("{ [0,180) }");
 
   // Seek to 0 and check all buffers.
@@ -2743,6 +2872,8 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Selected1) {
   NewSegmentAppend("180K 210 240");
 
   // Should save the GOP at 90ms and the GOP at 180ms.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(90)));
   CheckExpectedRangesByTimestamp("{ [90,270) }");
   CheckExpectedBuffers("90K 120 150 180K 210 240");
   CheckNoNextBuffer();
@@ -2763,22 +2894,24 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Selected2) {
   SetMemoryLimit(1);
   NewSegmentAppend("90K 120 150");
 
-  // Should save the GOP at 90ms and the GOP at 270ms.
-  CheckExpectedRangesByTimestamp("{ [90,180) [270,360) }");
+  // GC will save data in the range where the most recent append has happened
+  // [0; 180) and the range where the next read position is [270;360)
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(270)));
+  CheckExpectedRangesByTimestamp("{ [0,180) [270,360) }");
 
-  // Set memory limit to 100 and add 3 GOPs to the end of the selected range
-  // at 360ms, 450ms, and 540ms.
-  SetMemoryLimit(100);
+  // Add 3 GOPs to the end of the selected range at 360ms, 450ms, and 540ms.
   NewSegmentAppend("360K 390 420 450K 480 510 540K 570 600");
-  CheckExpectedRangesByTimestamp("{ [90,180) [270,630) }");
+  CheckExpectedRangesByTimestamp("{ [0,180) [270,630) }");
 
-  // Constrain the memory limit again and overlap the GOP at 450ms to test
-  // deleting from the back.
-  SetMemoryLimit(1);
+  // Overlap the GOP at 450ms and garbage collect to test deleting from the
+  // back.
   NewSegmentAppend("450K 480 510");
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(270)));
 
-  // Should save GOP at 270ms and the GOP at 450ms.
-  CheckExpectedRangesByTimestamp("{ [270,360) [450,540) }");
+  // Should save GOPs from GOP at 270ms to GOP at 450ms.
+  CheckExpectedRangesByTimestamp("{ [270,540) }");
 }
 
 // Test saving the last GOP appended when it is the same as the GOP containing
@@ -2796,8 +2929,10 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Selected3) {
   SetMemoryLimit(1);
   NewSegmentAppend("0K 30");
 
-  // Should save the newly appended GOP, which is also the next GOP that will be
-  // returned from the seek request.
+  // GC should save the newly appended GOP, which is also the next GOP that
+  // will be returned from the seek request.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(0)));
   CheckExpectedRangesByTimestamp("{ [0,60) }");
 
   // Check the buffers in the range.
@@ -2807,8 +2942,10 @@ TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Selected3) {
   // Continue appending to this buffer.
   AppendBuffers("60 90");
 
-  // Should still save the rest of this GOP and should be able to fulfill the
-  // read.
+  // GC should still save the rest of this GOP and should be able to fulfill
+  // the read.
+  EXPECT_FALSE(stream_->GarbageCollectIfNeeded(
+      DecodeTimestamp::FromMilliseconds(0)));
   CheckExpectedRangesByTimestamp("{ [0,120) }");
   CheckExpectedBuffers("60 90");
   CheckNoNextBuffer();