Fix seeking back in the new MSE GC algorithm

media/filters/chunk_demuxer_unittest.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 <algorithm>
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
@@ skipping 3349 matching lines @@
                             seek_time.InMilliseconds(), 5);
 
   // We should delete first append, and be exactly at buffer limit
   EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 0));
 
   // Verify that the old data, and nothing more, has been garbage collected.
   CheckExpectedRanges(DemuxerStream::AUDIO, "{ [1000,1230) }");
   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));
 
   demuxer_->SetMemoryLimits(DemuxerStream::AUDIO, 5 * kBlockSize);
 
   base::TimeDelta seek_time1 = base::TimeDelta::FromMilliseconds(1000);
   base::TimeDelta seek_time2 = base::TimeDelta::FromMilliseconds(500);
 
   // Initiate a seek to |seek_time1|.
   Seek(seek_time1);
@@ skipping 468 matching lines @@
   // audio size is 80 bytes, new data is 28 bytes, we need to remove just one 10
   // byte block to stay under 100 bytes memory limit after append
   // 80 - 10 + 28 = 98).
   // For video stream 150 + 52 = 202. Video limit is 150 bytes. We need to
   // remove at least 6 blocks to stay under limit.
   CheckExpectedBuffers(audio_stream, "40K 80K 120K 160K 200K 240K 280K");
   CheckExpectedBuffers(video_stream, "60K 70 80K 90 100K 110 120K 130 140K");
 }
 
 }  // namespace media