Fix seeking back in the new MSE GC algorithm

media/filters/chunk_demuxer_unittest.cc

Index: media/filters/chunk_demuxer_unittest.cc
diff --git a/media/filters/chunk_demuxer_unittest.cc b/media/filters/chunk_demuxer_unittest.cc
index 99299ac305dd0f346d3862b44188b8740a6cbb26..c2e4ff70dc44a1aaccbeb73b67cd882977f67ed2 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -3367,6 +3367,100 @@ TEST_F(ChunkDemuxerTest, SetMemoryLimitType) {
   CheckExpectedRanges(DemuxerStream::VIDEO, "{ [1000,1165) }");
 }
 
+TEST_F(ChunkDemuxerTest, GCDuringSeek_SingleRange_SeekForward) {
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO));
+  demuxer_->SetMemoryLimits(DemuxerStream::AUDIO, 10 * kBlockSize);
+  // Append some data at position 1000ms
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 1000, 10);
+  CheckExpectedRanges(kSourceId, "{ [1000,1230) }");
+
+  // GC should be able to evict frames in the currently buffered range, since
+  // those frames are earlier than the seek target position.
+  base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(2000);
+  Seek(seek_time);
+  EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 5 * kBlockSize));
+
+  // Append data to complete seek operation
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 2000, 5);
+  CheckExpectedRanges(kSourceId, "{ [1115,1230) [2000,2115) }");
+}
+
+TEST_F(ChunkDemuxerTest, GCDuringSeek_SingleRange_SeekBack) {
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO));
+  demuxer_->SetMemoryLimits(DemuxerStream::AUDIO, 10 * kBlockSize);
+  // Append some data at position 1000ms
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 1000, 10);
+  CheckExpectedRanges(kSourceId, "{ [1000,1230) }");
+
+  // GC should be able to evict frames in the currently buffered range, since
+  // seek target position has no data and so we should allow some frames to be
+  // evicted to make space for the upcoming append at seek target position.
+  base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(0);
+  Seek(seek_time);
+  EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 5 * kBlockSize));
+
+  // Append data to complete seek operation
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 0, 5);
+  CheckExpectedRanges(kSourceId, "{ [0,115) [1115,1230) }");
+}
+
+TEST_F(ChunkDemuxerTest, GCDuringSeek_MultipleRanges_SeekForward) {
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO));
+  demuxer_->SetMemoryLimits(DemuxerStream::AUDIO, 10 * kBlockSize);
+  // Append some data at position 1000ms then at 2000ms
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 1000, 5);
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 2000, 5);
+  CheckExpectedRanges(kSourceId, "{ [1000,1115) [2000,2115) }");
+
+  // GC should be able to evict frames in the currently buffered range, since

[wolenetz, 2015/09/17 18:57:54]

nit:s/range/ranges/

[servolk, 2015/09/18 01:29:27]

Done.

+  // those frames are earlier than the seek target position.
+  base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(3000);
+  Seek(seek_time);
+  EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 8 * kBlockSize));
+
+  // Append data to complete seek operation
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 3000, 5);
+  CheckExpectedRanges(kSourceId, "{ [2069,2115) [3000,3115) }");
+}
+
+TEST_F(ChunkDemuxerTest, GCDuringSeek_MultipleRanges_SeekInbetween) {
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO));
+  demuxer_->SetMemoryLimits(DemuxerStream::AUDIO, 10 * kBlockSize);
+  // Append some data at position 1000ms then at 2000ms
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 1000, 5);
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 2000, 5);
+  CheckExpectedRanges(kSourceId, "{ [1000,1115) [2000,2115) }");
+
+  // GC should be able to evict frames from the front, since

[wolenetz, 2015/09/17 18:57:54]

nit: it's collecting from front conservatively, then greedily. last append was
2115, so greedy was needed to get enough room. comment is unclear about this.

Regarding test coverage, just one more I think would be good:
GCDuringSeek_MultipleRanges_SeekInbetween2, where last append was the first of
the two ranges in timeline. This would need to exercise and verify
collect_from_back_until_last_appended_GOP.

[servolk, 2015/09/18 01:29:28]

Good point, I've expanded this comment to explain better what's going on this
this case.
I've also added a new test case where the appended data in the first range was
appended more recently than in the second range and that makes GC algorithm
choose to remove the second buffered range (since the first one was appended
more recently than the second).

+  // those frames are earlier than the seek target position.
+  base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(1500);
+  Seek(seek_time);
+  EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 8 * kBlockSize));
+
+  // Append data to complete seek operation
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 1500, 5);
+  CheckExpectedRanges(kSourceId, "{ [1500,1615) [2069,2115) }");
+}
+
+TEST_F(ChunkDemuxerTest, GCDuringSeek_MultipleRanges_SeekBack) {
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO));
+  demuxer_->SetMemoryLimits(DemuxerStream::AUDIO, 10 * kBlockSize);
+  // Append some data at position 1000ms then at 2000ms
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 1000, 5);
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 2000, 5);
+  CheckExpectedRanges(kSourceId, "{ [1000,1115) [2000,2115) }");
+
+  // GC should be able to evict frames in the currently buffered range, since
+  // those frames are earlier than the seek target position.
+  base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(0);

[wolenetz, 2015/09/17 18:57:54]

weak nit: here and elsewhere, just base::TimeDelta() for a time of 0. Existing
unit test code here is inconsistent, hence the weakness of this nit...

[servolk, 2015/09/18 01:29:28]

It makes sense in retrospect, but I just didn't realize that TimeDelta has a
default constructor that inits it to 0. I've made changes to use it for now,
although perhaps in the future we could consider introducing operator
TimeDelta(int ms) just in this file locally, it could make test code a bit
cleaner by avoiding all those repeated FromMilliseconds calls.

+  Seek(seek_time);
+  EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 8 * kBlockSize));
+
+  // Append data to complete seek operation
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum, 0, 5);
+  CheckExpectedRanges(kSourceId, "{ [0,115) [2069,2115) }");
+}
+
 TEST_F(ChunkDemuxerTest, GCDuringSeek) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO));