Update SourceBufferStream and its unit tests to always expect valid durations.

media/filters/source_buffer_stream_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 "media/filters/source_buffer_stream.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 13 matching lines @@
 typedef StreamParser::BufferQueue BufferQueue;
 
 static const int kDefaultFramesPerSecond = 30;
 static const int kDefaultKeyframesPerSecond = 6;
 static const uint8 kDataA = 0x11;
 static const uint8 kDataB = 0x33;
 static const int kDataSize = 1;
 
 class SourceBufferStreamTest : public testing::Test {
  protected:
-  SourceBufferStreamTest()
-      : accurate_durations_(false) {
+  SourceBufferStreamTest() {
     video_config_ = TestVideoConfig::Normal();
     SetStreamInfo(kDefaultFramesPerSecond, kDefaultKeyframesPerSecond);
     stream_.reset(new SourceBufferStream(video_config_, log_cb(), true));
   }
 
   void SetMemoryLimit(int buffers_of_data) {
     stream_->set_memory_limit_for_testing(buffers_of_data * kDataSize);
   }
 
   void SetStreamInfo(int frames_per_second, int keyframes_per_second) {
     frames_per_second_ = frames_per_second;
     keyframes_per_second_ = keyframes_per_second;
     frame_duration_ = ConvertToFrameDuration(frames_per_second);
   }
 
   void SetTextStream() {
     video_config_ = TestVideoConfig::Invalid();
     TextTrackConfig config(kTextSubtitles, "", "", "");
     stream_.reset(new SourceBufferStream(config, LogCB(), true));
     SetStreamInfo(2, 2);
   }
 
   void SetAudioStream() {
     video_config_ = TestVideoConfig::Invalid();
-    accurate_durations_ = true;
     audio_config_.Initialize(kCodecVorbis,
                              kSampleFormatPlanarF32,
                              CHANNEL_LAYOUT_STEREO,
                              1000,
                              NULL,
                              0,
                              false,
                              false,
                              base::TimeDelta(),
                              0);
@@ skipping 200 matching lines @@
             stream_->GetCurrentVideoDecoderConfig();
             break;
           case SourceBufferStream::kAudio:
             stream_->GetCurrentAudioDecoderConfig();
             break;
           case SourceBufferStream::kText:
             stream_->GetCurrentTextTrackConfig();
             break;
         }
 
         if (timestamps[i] == "C")

[wolenetz, 2014/07/09 22:42:18]

Something seems wrong here, too. We're checking that status is kConfigChange
inside a block that guarantees this.
Instead, ISTM like we should assert that timestamps[i] is "C" in this block, and
add an else clause for this block that asserts timestamps[i] isn't "C".

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

I just changed this to EXPECT_EQ("C", timestamps[i]) since I agree the condition
above guarantees the expectation ins always true.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

           EXPECT_EQ(SourceBufferStream::kConfigChange, status);
 
         ss << "C";
         continue;
       }
 
       EXPECT_EQ(SourceBufferStream::kSuccess, status);
       if (status != SourceBufferStream::kSuccess)
         break;
 
@@ skipping 57 matching lines @@
         << "Expected: " << config.AsHumanReadableString()
         << "\nActual: " << actual.AsHumanReadableString();
   }
 
   const LogCB log_cb() {
     return base::Bind(&SourceBufferStreamTest::DebugMediaLog,
                       base::Unretained(this));
   }
 
   base::TimeDelta frame_duration() const { return frame_duration_; }
+  void set_frame_duration(base::TimeDelta frame_duration) {
+    frame_duration_ = frame_duration;
+  }
 
   scoped_ptr<SourceBufferStream> stream_;
   VideoDecoderConfig video_config_;
   AudioDecoderConfig audio_config_;
 
  private:
   base::TimeDelta ConvertToFrameDuration(int frames_per_second) {
     return base::TimeDelta::FromMicroseconds(
         base::Time::kMicrosecondsPerSecond / frames_per_second);
   }
@@ skipping 31 matching lines @@
       if (is_keyframe) {
         presentation_timestamp = timestamp;
       } else if ((position - 1) % keyframe_interval == 0) {
         // This is the P-frame (first frame following the I-frame)
         presentation_timestamp =
             (timestamp + frame_duration_ * (keyframe_interval - 2));
       } else {
         presentation_timestamp = timestamp - frame_duration_;
       }
       buffer->set_timestamp(presentation_timestamp);
-      if (accurate_durations_)
-        buffer->set_duration(frame_duration_);
+      buffer->set_duration(frame_duration_);
 
       queue.push_back(buffer);
     }
     if (!queue.empty())
       EXPECT_EQ(expect_success, stream_->Append(queue));
   }
 
+  void UpdateLastBufferDuration(base::TimeDelta current_dts,
+                                BufferQueue* buffers) {
+    if (buffers->empty() || buffers->back()->duration() > base::TimeDelta())
+      return;
+
+    base::TimeDelta last_dts = buffers->back()->GetDecodeTimestamp();
+    DCHECK(current_dts >= last_dts);
+    buffers->back()->set_duration(current_dts - last_dts);
+  }
+
   // StringToBufferQueue() allows for the generation of StreamParserBuffers from
   // coded strings of timestamps separated by spaces.  Supported syntax:
   //
   // ##:
   // Generates a StreamParserBuffer with decode timestamp ##.  E.g., "0 1 2 3".
   //
   // ##K:
   // Indicates the buffer with timestamp ## reflects a keyframe.  E.g., "0K 1".
   //
   // S(a# ... y# z#)
   // Indicates a splice frame buffer should be created with timestamp z#.  The
   // preceding timestamps a# ... y# will be treated as the fade out preroll for
   // the splice frame.  If a timestamp within the preroll ends with C the config
   // id to use for that and subsequent preroll appends is incremented by one.
   // The config id for non-splice frame appends will not be affected.

[wolenetz, 2014/07/09 22:42:17]

nit: Update comment for new ..D##.. syntax

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Done.

   BufferQueue StringToBufferQueue(const std::string& buffers_to_append) {
     std::vector<std::string> timestamps;
     base::SplitString(buffers_to_append, ' ', &timestamps);
 
     CHECK_GT(timestamps.size(), 0u);
 
     bool splice_frame = false;
     size_t splice_config_id = stream_->append_config_index_;
     BufferQueue pre_splice_buffers;
     BufferQueue buffers;
@@ skipping 28 matching lines @@
         timestamps[i] = timestamps[i].substr(0, timestamps[i].length() - 1);
       }
       // Handle preroll buffers.
       if (EndsWith(timestamps[i], "P", true)) {
         is_keyframe = true;
         has_preroll = true;
         // Remove the "P" off of the token.
         timestamps[i] = timestamps[i].substr(0, timestamps[i].length() - 1);
       }
 
-      int time_in_ms;
+      int time_in_ms = 0;
+      int duration_in_ms = 0;
+      size_t duration_pos = timestamps[i].find('D');
+      if (duration_pos != std::string::npos) {
+        CHECK(base::StringToInt(timestamps[i].substr(duration_pos + 1),
+                                &duration_in_ms));
+        timestamps[i] = timestamps[i].substr(0, duration_pos );

[wolenetz, 2014/07/09 22:42:16]

nit: s/ )/)/

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:28]

Done.

+      }
+
       CHECK(base::StringToInt(timestamps[i], &time_in_ms));
 
       // Create buffer. Buffer type and track ID are meaningless to these tests.
       scoped_refptr<StreamParserBuffer> buffer =
           StreamParserBuffer::CopyFrom(&kDataA, kDataSize, is_keyframe,
                                        DemuxerStream::AUDIO, 0);
       base::TimeDelta timestamp =
           base::TimeDelta::FromMilliseconds(time_in_ms);
       buffer->set_timestamp(timestamp);
-      if (accurate_durations_)
-        buffer->set_duration(frame_duration_);
+      if (duration_in_ms)
+        buffer->set_duration(base::TimeDelta::FromMilliseconds(duration_in_ms));
       buffer->SetDecodeTimestamp(timestamp);
 
       // Simulate preroll buffers by just generating another buffer and sticking
       // it as the preroll.
       if (has_preroll) {
         scoped_refptr<StreamParserBuffer> preroll_buffer =
             StreamParserBuffer::CopyFrom(
                 &kDataA, kDataSize, is_keyframe, DemuxerStream::AUDIO, 0);
         preroll_buffer->set_duration(frame_duration_);
         buffer->SetPrerollBuffer(preroll_buffer);
       }
 
       if (splice_frame) {
         if (!pre_splice_buffers.empty()) {
           // Enforce strictly monotonically increasing timestamps.
           CHECK_GT(
               timestamp.InMicroseconds(),
               pre_splice_buffers.back()->GetDecodeTimestamp().InMicroseconds());
         }
         buffer->SetConfigId(splice_config_id);
+        UpdateLastBufferDuration(buffer->GetDecodeTimestamp(),
+                                 &pre_splice_buffers);
         pre_splice_buffers.push_back(buffer);
         continue;
       }
 
       if (last_splice_frame) {
         // Require at least one additional buffer for a splice.
         CHECK(!pre_splice_buffers.empty());
         buffer->SetConfigId(splice_config_id);
         buffer->ConvertToSpliceBuffer(pre_splice_buffers);

[wolenetz, 2014/07/09 22:42:17]

Something seems wrong to me, or I've lost my clarity on this since previous CLs:
why do we test appending buffers that have splice frames already converted,
instead of testing that SBS append properly creates the splice buffers? ILTM
like SBS::GenerateSpliceFrame() is essentially simulated here (not tested here),
and that the purpose of this is to attempt to cover the GetNextBuffers and
buffered range calculations in the presence of splice frames. ChunkDemuxerTest
other pipeline / integration / layout tests might cover GenerateSpliceFrame, but
I'm wondering if there is a better way to unit test it here.

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

This sounds like a question for Dale and should be addressed in a separate CL.

[wolenetz, 2014/07/15 21:05:37]

I've pinged Dale in email. Thanks.

[DaleCurtis, 2014/07/15 21:06:41]

1. These tests were written before the splice frame implementation was complete.
 I wrote the unpacking logic first.
2. Splice frames aren't generated for Video which is what all tests except the
special Audio_XXX tests use.
3. IIRC, crafting some of the corner cases is ugly/non-trivial through the
standard interface.

Probably this path can be killed, but it may be non-trivial to do so. 
Definitely something for another CL.

         pre_splice_buffers.clear();
       }
 
+      UpdateLastBufferDuration(buffer->GetDecodeTimestamp(), &buffers);
       buffers.push_back(buffer);
     }
+
+    // If the last buffer doesn't have a duration, assume it is the
+    // same as the second to last buffer.
+    if (buffers.size() >= 2 &&
+        buffers.back()->duration() <= base::TimeDelta()) {
+      buffers.back()->set_duration(
+          buffers[buffers.size() - 2]->duration());
+    }
+
     return buffers;
   }
 
   void AppendBuffers(const std::string& buffers_to_append,
                      bool start_new_segment,
                      base::TimeDelta segment_start_timestamp,
                      bool one_by_one,
                      bool expect_success) {
     BufferQueue buffers = StringToBufferQueue(buffers_to_append);
 
@@ skipping 20 matching lines @@
     }
   }
 
   void DebugMediaLog(const std::string& log) {
     DVLOG(1) << log;
   }
 
   int frames_per_second_;
   int keyframes_per_second_;
   base::TimeDelta frame_duration_;
-  // TODO(dalecurtis): It's silly to have this, all tests should use accurate
-  // durations instead.  However, currently all tests are written with an
-  // expectation of 0 duration, so it's an involved change.
-  bool accurate_durations_;
   DISALLOW_COPY_AND_ASSIGN(SourceBufferStreamTest);
 };
 
 TEST_F(SourceBufferStreamTest, Append_SingleRange) {
   // Append 15 buffers at positions 0 through 14.
   NewSegmentAppend(0, 15);
 
   // Check expected range.
   CheckExpectedRanges("{ [0,14) }");
   // Check buffers in range.
@@ skipping 102 matching lines @@
   // Append a segment with a start timestamp of 0, but the first
   // buffer starts at 30ms. This can happen in muxed content where the
   // audio starts before the first frame.
   NewSegmentAppend(base::TimeDelta::FromMilliseconds(0), "30K 60K 90K 120K");
 
   CheckExpectedRangesByTimestamp("{ [0,150) }");
 
   // Completely overlap the old buffers, with a segment that starts
   // after the old segment start timestamp, but before the timestamp
   // of the first buffer in the segment.
-  NewSegmentAppend("20K 50K 80K 110K");
+  NewSegmentAppend("20K 50K 80K 110D10K");
 
   // Verify that the buffered ranges are updated properly and we don't crash.
   CheckExpectedRangesByTimestamp("{ [20,150) }");
 
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(20));
   CheckExpectedBuffers("20K 50K 80K 110K 120K");
 }
 
 TEST_F(SourceBufferStreamTest, Complete_Overlap_EdgeCase) {
   // Make each frame a keyframe so that it's okay to overlap frames at any point
@@ skipping 61 matching lines @@
   CheckExpectedBuffers(15, 19);
 }
 
 // Test an end overlap edge case where a single buffer overlaps the
 // beginning of a range.
 // old  : *0K*   30   60   90   120K  150
 // new  : *0K*
 // after: *0K*                 *120K* 150K
 // track:
 TEST_F(SourceBufferStreamTest, End_Overlap_SingleBuffer) {
+  set_frame_duration(base::TimeDelta::FromMilliseconds(30));
+
   // Seek to start of stream.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(0));
 
   NewSegmentAppend("0K 30 60 90 120K 150");
   CheckExpectedRangesByTimestamp("{ [0,180) }");
 
-  NewSegmentAppend("0K");
+  NewSegmentAppend("0D30K");

[wolenetz, 2014/07/09 22:42:18]

curious: without D30 and frame_duration_ set to 30ms, what would the resulting
ranges be on the next line?

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Removed the set_frame_duration() call since it was left over from some old
changes.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

   CheckExpectedRangesByTimestamp("{ [0,30) [120,180) }");
 
   CheckExpectedBuffers("0K");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, Complete_Overlap_Several) {
   // Append 2 buffers at positions 5 through 6.
   NewSegmentAppend(5, 2);
 
@@ skipping 748 matching lines @@
   CheckExpectedBuffers(0, 9, &kDataB);
 }
 
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_DeleteGroup) {
   NewSegmentAppendOneByOne("10K 40 70 100 130K");
   CheckExpectedRangesByTimestamp("{ [10,160) }");
 
   // Seek to 130ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(130));
 
   // Overlap with a new segment from 0 to 120ms.

[wolenetz, 2014/07/09 22:42:17]

nit: s/120/130?

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Done.

-  NewSegmentAppendOneByOne("0K 120");
+  NewSegmentAppendOneByOne("0K 120D10");
 
   // Next buffer should still be 130ms.
   CheckExpectedBuffers("130K");
 
   // Check the final buffers is correct.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(0));
   CheckExpectedBuffers("0K 120 130K");
 }
 
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_BetweenMediaSegments) {
@@ skipping 10 matching lines @@
   // Check the final buffers is correct; the keyframe at 110ms should be
   // deleted.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(0));
   CheckExpectedBuffers("0K 30 60 90 120K 150 180 210");
 }
 
 // old  :   10K  40  *70*  100K  125  130K
 // new  : 0K   30   60   90   120K
 // after: 0K   30   60   90  *120K*   130K
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_TrackBuffer) {
-  NewSegmentAppendOneByOne("10K 40 70 100K 125 130K");
+  NewSegmentAppendOneByOne("10K 40 70 100K 125 130D30K");
   CheckExpectedRangesByTimestamp("{ [10,160) }");
 
   // Seek to 70ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms.

[wolenetz, 2014/07/09 22:42:16]

nit: s/120/130? (or 125, see next comment)

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Done.

-  NewSegmentAppendOneByOne("0K 30 60 90 120K");
+  NewSegmentAppendOneByOne("0K 30 60 90 120D10K");

[wolenetz, 2014/07/09 22:42:17]

s/120D10K/120D5K/ to better test that old buffer at 125ms is deleted since its
prerequisite keyframe was deleted? This might match more closely the intention
in the next source code comment, below.

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

I'm just trying to explicitly document the original test behavior. I'm not
trying to refine the tests here.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

   CheckExpectedRangesByTimestamp("{ [0,160) }");
 
-  // Should return frames 70ms and 100ms from the track buffer, then switch
+  // Should return frame 70ms from the track buffer, then switch
   // to the new data at 120K, then switch back to the old data at 130K. The
   // frame at 125ms that depended on keyframe 100ms should have been deleted.
-  CheckExpectedBuffers("70 100K 120K 130K");

[acolwell GONE FROM CHROMIUM, 2014/07/09 02:07:02]

I haven't 100% convinced myself that this particular change in behavior is
right. The old behavior does appear to rely on the fact that the old durations
were super tiny instead of 30ms like they were actually intended to be when the
test was written. I'm pretty sure when 90 gets appended it now causes 100 to get
deleted, where that wouldn't have happened before because 90 would have only
spanned 90-90.000001.

[wolenetz, 2014/07/09 22:42:17]

I think your analysis is correct, and the new behavior is also spec compliant
(CFP step 15.2, specifically, should remove original 100K (and step 16 should
then remove original 125)). I'm less clear why the previous code behaved as it
did (how does SBS determine frame_end_timestamp for the purposes of step 15?)
reference spec:
https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source....)

+  CheckExpectedBuffers("70 120K 130K");
 
   // Check the final result: should not include data from the track buffer.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(0));
   CheckExpectedBuffers("0K 30 60 90 120K 130K");
 }
 
 // Overlap the next keyframe after the end of the track buffer with a new
 // keyframe.
 // old  :   10K  40  *70*  100K  125  130K
 // new  : 0K   30   60   90   120K
 // after: 0K   30   60   90  *120K*   130K
-// track:             70   100K
+// track:             70
 // new  :                     110K    130
 // after: 0K   30   60   90  *110K*   130
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_TrackBuffer2) {
-  NewSegmentAppendOneByOne("10K 40 70 100K 125 130K");
+  NewSegmentAppendOneByOne("10K 40 70 100K 125 130D30K");
   CheckExpectedRangesByTimestamp("{ [10,160) }");
 
   // Seek to 70ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms; 70ms and 100ms go in track
   // buffer.
-  NewSegmentAppendOneByOne("0K 30 60 90 120K");
+  NewSegmentAppendOneByOne("0K 30 60 90 120D10K");
   CheckExpectedRangesByTimestamp("{ [0,160) }");
 
   // Now overlap the keyframe at 120ms.
   NewSegmentAppendOneByOne("110K 130");
 
   // Should expect buffers 70ms and 100ms from the track buffer. Then it should

[wolenetz, 2014/07/09 22:42:17]

nit: s/expect buffers 70ms and 100ms/return frame 70ms/

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:26]

Done.

   // return the keyframe after the track buffer, which is at 110ms.
-  CheckExpectedBuffers("70 100K 110K 130");
+  CheckExpectedBuffers("70 110K 130");
 }
 
 // Overlap the next keyframe after the end of the track buffer without a
 // new keyframe.
 // old  :   10K  40  *70*  100K  125  130K
 // new  : 0K   30   60   90   120K
 // after: 0K   30   60   90  *120K*   130K
-// track:             70   100K
+// track:             70
 // new  :        50K   80   110          140
 // after: 0K   30   50K   80   110   140 * (waiting for keyframe)
-// track:               70   100K   120K   130K
+// track:             70
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_TrackBuffer3) {
-  NewSegmentAppendOneByOne("10K 40 70 100K 125 130K");
+  NewSegmentAppendOneByOne("10K 40 70 100K 125 130D30K");
   CheckExpectedRangesByTimestamp("{ [10,160) }");
 
   // Seek to 70ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
-  // Overlap with a new segment from 0 to 120ms; 70ms and 100ms go in track
-  // buffer.
-  NewSegmentAppendOneByOne("0K 30 60 90 120K");
+  // Overlap with a new segment from 0 to 120ms; 70ms goes in track buffer.
+  NewSegmentAppendOneByOne("0K 30 60 90 120D10K");
   CheckExpectedRangesByTimestamp("{ [0,160) }");
 
   // Now overlap the keyframe at 120ms. There's no keyframe after 70ms, so 120ms

[wolenetz, 2014/07/09 22:42:17]

nit: s/*/Now overlap the keyframes at 120ms and 130ms./ ?

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:26]

Done.

   // and 130ms go into the track buffer.
   NewSegmentAppendOneByOne("50K 80 110 140");
+  CheckExpectedRangesByTimestamp("{ [0,170) }");
 
   // Should have all the buffers from the track buffer, then stall.
-  CheckExpectedBuffers("70 100K 120K 130K");
+  CheckExpectedBuffers("70");
   CheckNoNextBuffer();
 
   // Appending a keyframe should fulfill the read.
-  AppendBuffersOneByOne("150K");
+  AppendBuffersOneByOne("150D30K");
   CheckExpectedBuffers("150K");
   CheckNoNextBuffer();
 }
 
 // Overlap the next keyframe after the end of the track buffer with a keyframe
 // that comes before the end of the track buffer.
 // old  :   10K  40  *70*  100K  125  130K
 // new  : 0K   30   60   90   120K
 // after: 0K   30   60   90  *120K*   130K
 // track:             70   100K

[wolenetz, 2014/07/09 22:42:17]

nit: s/100K//

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:26]

Done.

 // new  :              80K  110          140
 // after: 0K   30   60   *80K*  110   140
 // track:               70
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_TrackBuffer4) {
-  NewSegmentAppendOneByOne("10K 40 70 100K 125 130K");
+  NewSegmentAppendOneByOne("10K 40 70 100K 125 130D30K");
   CheckExpectedRangesByTimestamp("{ [10,160) }");
 
   // Seek to 70ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms; 70ms and 100ms go in track
   // buffer.
-  NewSegmentAppendOneByOne("0K 30 60 90 120K");
+  NewSegmentAppendOneByOne("0K 30 60 90 120D10K");
   CheckExpectedRangesByTimestamp("{ [0,160) }");
 
   // Now append a keyframe at 80ms.
   NewSegmentAppendOneByOne("80K 110 140");
 
   CheckExpectedBuffers("70 80K 110 140");
   CheckNoNextBuffer();
 }
 
 // Overlap the next keyframe after the end of the track buffer with a keyframe
 // that comes before the end of the track buffer, when the selected stream was
 // waiting for the next keyframe.
 // old  :   10K  40  *70*  100K
 // new  : 0K   30   60   90   120
 // after: 0K   30   60   90   120 * (waiting for keyframe)
 // track:             70   100K

[wolenetz, 2014/07/09 22:42:17]

nit: s/100K//

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Done.

 // new  :              80K  110          140
 // after: 0K   30   60   *80K*  110   140
 // track:               70
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_TrackBuffer5) {
   NewSegmentAppendOneByOne("10K 40 70 100K");

[wolenetz, 2014/07/09 22:42:17]

nit: is s/100K/100D30K/ necessary to match my other nits in this test?

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

The D30 is not needed because the duration is implied by the previous frames.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

   CheckExpectedRangesByTimestamp("{ [10,130) }");
 
   // Seek to 70ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms; 70ms and 100ms go in track

[wolenetz, 2014/07/09 22:42:17]

nit: s/and 100ms go/goes/

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:26]

Done.

   // buffer.
   NewSegmentAppendOneByOne("0K 30 60 90 120");
   CheckExpectedRangesByTimestamp("{ [0,150) }");
 
   // Now append a keyframe at 80ms. The buffer at 100ms should be deleted from

[wolenetz, 2014/07/09 22:42:16]

nit s/The...//

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Done.

   // the track buffer.
   NewSegmentAppendOneByOne("80K 110 140");
 
   CheckExpectedBuffers("70 80K 110 140");
   CheckNoNextBuffer();
 }
 
 // Test that appending to a different range while there is data in
 // the track buffer doesn't affect the selected range or track buffer state.
 // old  :   10K  40  *70*  100K  125  130K ... 200K 230
 // new  : 0K   30   60   90   120K
 // after: 0K   30   60   90  *120K*   130K ... 200K 230
-// track:             70   100K
+// track:             70
 // old  : 0K   30   60   90  *120K*   130K ... 200K 230
 // new  :                                               260K 290
 // after: 0K   30   60   90  *120K*   130K ... 200K 230 260K 290
-// track:             70   100K
+// track:             70
 TEST_F(SourceBufferStreamTest, Overlap_OneByOne_TrackBuffer6) {
-  NewSegmentAppendOneByOne("10K 40 70 100K 125 130K");
+  NewSegmentAppendOneByOne("10K 40 70 100K 125 130D30K");
   NewSegmentAppendOneByOne("200K 230");
   CheckExpectedRangesByTimestamp("{ [10,160) [200,260) }");
 
   // Seek to 70ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms.
-  NewSegmentAppendOneByOne("0K 30 60 90 120K");
+  NewSegmentAppendOneByOne("0K 30 60 90 120D10K");
   CheckExpectedRangesByTimestamp("{ [0,160) [200,260) }");
 
   // Verify that 70 gets read out of the track buffer.
   CheckExpectedBuffers("70");
 
   // Append more data to the unselected range.
   NewSegmentAppendOneByOne("260K 290");
   CheckExpectedRangesByTimestamp("{ [0,160) [200,320) }");
 
-  CheckExpectedBuffers("100K 120K 130K");
+  CheckExpectedBuffers("120K 130K");
   CheckNoNextBuffer();
 
   // Check the final result: should not include data from the track buffer.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(0));
   CheckExpectedBuffers("0K 30 60 90 120K 130K");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, Seek_Keyframe) {
   // Append 6 buffers at positions 0 through 5.
@@ skipping 345 matching lines @@
 // GetNextBuffer() skips to second GOP in the newly appended data instead
 // of returning two buffers with the same timestamp.
 TEST_F(SourceBufferStreamTest, GetNextBuffer_ExhaustThenStartOverlap2) {
   NewSegmentAppend("0K 30 60 90 120");
 
   Seek(0);
   CheckExpectedBuffers("0K 30 60 90 120");
   CheckNoNextBuffer();
 
   // Append a keyframe with the same timestamp as the last buffer output.
-  NewSegmentAppend("120K");
+  NewSegmentAppend("120D30K");
   CheckNoNextBuffer();
 
   // Append the rest of the segment and make sure that buffers are returned
   // from the first GOP after 120.
   AppendBuffers("150 180 210K 240");
   CheckExpectedBuffers("210K 240");
 
   // Seek to the beginning and verify the contents of the source buffer.
   Seek(0);
   CheckExpectedBuffers("0K 30 60 90 120K 150 180 210K 240");
@@ skipping 407 matching lines @@
 // 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");
   CheckExpectedRangesByTimestamp("{ [0,120) }");
 
   // Make sure you can continue appending data to this GOP; again, GC should not
   // wipe out anything.
-  AppendBuffers("120");
+  AppendBuffers("120D30");
   CheckExpectedRangesByTimestamp("{ [0,150) }");
 
   // Set memory limit to 100 and append a 2nd range after this without
   // triggering GC.
   SetMemoryLimit(100);
   NewSegmentAppend("200K 230 260 290K 320 350");
   CheckExpectedRangesByTimestamp("{ [0,150) [200,380) }");
 
   // Seek to 290ms.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(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);
   NewSegmentAppend("500K 530 560 590");
 
   // Should save GOP with 290ms and last GOP appended.
   CheckExpectedRangesByTimestamp("{ [290,380) [500,620) }");
 
   // Continue appending to this GOP after GC.
-  AppendBuffers("620");
+  AppendBuffers("620D30");
   CheckExpectedRangesByTimestamp("{ [290,380) [500,650) }");
 }
 
 // Test saving the last GOP appended when this GOP is in the middle of a
 // non-selected range.
 TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Middle) {
   // Append 3 GOPs starting at 0ms, 30ms apart.
   NewSegmentAppend("0K 30 60 90K 120 150 180K 210 240");
   CheckExpectedRangesByTimestamp("{ [0,270) }");
 
   // Now set the memory limit to 1 and overlap the middle of the range with a
   // new GOP.
   SetMemoryLimit(1);
   NewSegmentAppend("80K 110 140");
 
   // This whole GOP should be saved, and should be able to continue appending
   // data to it.
   CheckExpectedRangesByTimestamp("{ [80,170) }");
-  AppendBuffers("170");
+  AppendBuffers("170D30");
   CheckExpectedRangesByTimestamp("{ [80,200) }");
 
   // Set memory limit to 100 and append a 2nd range after this without
   // triggering GC.
   SetMemoryLimit(100);
   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.
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(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);
   NewSegmentAppend("500K 530 560 590");
 
   // Should save the GOP containing the seek point and GOP that was last
   // appended.
   CheckExpectedRangesByTimestamp("{ [80,200) [500,620) }");
 
   // Continue appending to this GOP after GC.
-  AppendBuffers("620");
+  AppendBuffers("620D30");
   CheckExpectedRangesByTimestamp("{ [80,200) [500,650) }");
 }
 
 // Test saving the last GOP appended when the GOP containing the next buffer is
 // adjacent to the last GOP appended.
 TEST_F(SourceBufferStreamTest, GarbageCollection_SaveAppendGOP_Selected1) {
   // Append 3 GOPs at 0ms, 90ms, and 180ms.
   NewSegmentAppend("0K 30 60 90K 120 150 180K 210 240");
   CheckExpectedRangesByTimestamp("{ [0,270) }");
 
@@ skipping 994 matching lines @@
   Seek(0);
   CheckExpectedBuffers("0K 500K 1000K 3000K 3500K 4000K");
 }
 
 TEST_F(SourceBufferStreamTest, Text_CompleteOverlap) {
   SetTextStream();
   NewSegmentAppend("3000K 3500K 4000K");
   CheckExpectedRangesByTimestamp("{ [3000,4500) }");
   NewSegmentAppend("0K 501K 1001K 1501K 2001K 2501K "
                    "3001K 3501K 4001K 4501K 5001K");
-  CheckExpectedRangesByTimestamp("{ [0,5502) }");
+  CheckExpectedRangesByTimestamp("{ [0,5501) }");
 
   Seek(0);
   CheckExpectedBuffers("0K 501K 1001K 1501K 2001K 2501K "
                        "3001K 3501K 4001K 4501K 5001K");
 }
 
 TEST_F(SourceBufferStreamTest, Text_OverlapAfter) {
   SetTextStream();
   NewSegmentAppend("0K 500K 1000K 1500K 2000K");
   CheckExpectedRangesByTimestamp("{ [0,2500) }");
   NewSegmentAppend("1499K 2001K 2501K 3001K");
-  CheckExpectedRangesByTimestamp("{ [0,3503) }");
+  CheckExpectedRangesByTimestamp("{ [0,3501) }");
 
   Seek(0);
   CheckExpectedBuffers("0K 500K 1000K 1499K 2001K 2501K 3001K");
 }
 
 TEST_F(SourceBufferStreamTest, Text_OverlapBefore) {
   SetTextStream();
   NewSegmentAppend("1500K 2000K 2500K 3000K 3500K");
   CheckExpectedRangesByTimestamp("{ [1500,4000) }");
   NewSegmentAppend("0K 501K 1001K 1501K 2001K");
-  CheckExpectedRangesByTimestamp("{ [0,4001) }");
+  CheckExpectedRangesByTimestamp("{ [0,4000) }");
 
   Seek(0);
-  CheckExpectedBuffers("0K 501K 1001K 1501K 2001K 2500K 3000K 3500K");
+  CheckExpectedBuffers("0K 501K 1001K 1501K 2001K 3000K 3500K");
 }
 
 TEST_F(SourceBufferStreamTest, SpliceFrame_Basic) {
   Seek(0);
-  NewSegmentAppend("0K S(3K 6 9 10) 15 20 S(25K 30 35) 40");
+  NewSegmentAppend("0K S(3K 6 9D3 10D5) 15 20 S(25K 30D5 35D5) 40");
   CheckExpectedBuffers("0K 3K 6 9 C 10 15 20 25K 30 C 35 40");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, SpliceFrame_SeekClearsSplice) {
   Seek(0);
-  NewSegmentAppend("0K S(3K 6 9 10) 15K 20");
+  NewSegmentAppend("0K S(3K 6 9D3 10D5) 15K 20");
   CheckExpectedBuffers("0K 3K 6");
 
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(15));
   CheckExpectedBuffers("15K 20");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, SpliceFrame_SeekClearsSpliceFromTrackBuffer) {
   Seek(0);
-  NewSegmentAppend("0K 2K S(3K 6 9 10) 15K 20");
+  NewSegmentAppend("0K 2K S(3K 6 9D3 10D5) 15K 20");
   CheckExpectedBuffers("0K 2K");
 
   // Overlap the existing segment.
   NewSegmentAppend("5K 15K 20");
   CheckExpectedBuffers("3K 6");
 
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(15));
   CheckExpectedBuffers("15K 20");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, SpliceFrame_ConfigChangeWithinSplice) {
   VideoDecoderConfig new_config = TestVideoConfig::Large();
   ASSERT_FALSE(new_config.Matches(video_config_));
 
   // Add a new video config, then reset the config index back to the original.
   stream_->UpdateVideoConfig(new_config);
   stream_->UpdateVideoConfig(video_config_);
 
   Seek(0);
   CheckVideoConfig(video_config_);
-  NewSegmentAppend("0K S(3K 6C 9 10) 15");
+  NewSegmentAppend("0K S(3K 6C 9D3 10D5) 15");
 
   CheckExpectedBuffers("0K 3K C");
   CheckVideoConfig(new_config);
   CheckExpectedBuffers("6 9 C");
   CheckExpectedBuffers("10 C");
   CheckVideoConfig(video_config_);
   CheckExpectedBuffers("15");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, SpliceFrame_BasicFromTrackBuffer) {
   Seek(0);
-  NewSegmentAppend("0K 5K S(8K 9 10) 20");
+  NewSegmentAppend("0K 5K S(8K 9D1 10D10) 20");
   CheckExpectedBuffers("0K 5K");
 
   // Overlap the existing segment.
   NewSegmentAppend("5K 20");
   CheckExpectedBuffers("8K 9 C 10 20");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest,
        SpliceFrame_ConfigChangeWithinSpliceFromTrackBuffer) {
   VideoDecoderConfig new_config = TestVideoConfig::Large();
   ASSERT_FALSE(new_config.Matches(video_config_));
 
   // Add a new video config, then reset the config index back to the original.
   stream_->UpdateVideoConfig(new_config);
   stream_->UpdateVideoConfig(video_config_);
 
   Seek(0);
   CheckVideoConfig(video_config_);
-  NewSegmentAppend("0K 5K S(7K 8C 9 10) 20");
+  NewSegmentAppend("0K 5K S(7K 8C 9D1 10D10) 20");
   CheckExpectedBuffers("0K 5K");
 
   // Overlap the existing segment.
   NewSegmentAppend("5K 20");
   CheckExpectedBuffers("7K C");
   CheckVideoConfig(new_config);
   CheckExpectedBuffers("8 9 C");
   CheckExpectedBuffers("10 C");
   CheckVideoConfig(video_config_);
   CheckExpectedBuffers("20");
@@ skipping 24 matching lines @@
   Seek(0);
   NewSegmentAppend("0K 2K 4K 6K 8K 10K 12K");
   NewSegmentAppend("11K 13K 15K 17K");
 
   // Verify the splice was created.
   CheckExpectedBuffers("0K 2K 4K 6K 8K 10K 12K C 11K 13K 15K 17K");
   CheckNoNextBuffer();
   Seek(0);
 
   // Create a splice before the first splice which would include it.
-  NewSegmentAppend("9K");
+  NewSegmentAppend("9D2K");

[wolenetz, 2014/07/09 22:42:17]

Is D2 necessary since SetAudioStream()->SetStreamInfo() sets |frame_duration_|?

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

Yes. frame_duration_ is not used by string based appends. I wanted to force
durations to be specified explicitly in the 1 buffer case.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

 
   // A splice on top of a splice should result in a discard of the original
   // splice and no new splice frame being generated.
   CheckExpectedBuffers("0K 2K 4K 6K 8K 9K 13K 15K 17K");
   CheckNoNextBuffer();
 }
 
 // Test that a splice is not created if an end timestamp and start timestamp
 // overlap.
 TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_NoSplice) {
   SetAudioStream();
   Seek(0);
   NewSegmentAppend("0K 2K 4K 6K 8K 10K");
   NewSegmentAppend("12K 14K 16K 18K");
   CheckExpectedBuffers("0K 2K 4K 6K 8K 10K 12K 14K 16K 18K");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_CorrectMediaSegmentStartTime) {
   SetAudioStream();
   Seek(0);
   NewSegmentAppend("0K 2K 4K");
   CheckExpectedRangesByTimestamp("{ [0,6) }");
   NewSegmentAppend("6K 8K 10K");
   CheckExpectedRangesByTimestamp("{ [0,12) }");
-  NewSegmentAppend("1K 4K");
+  NewSegmentAppend("1K 4D2K");

[wolenetz, 2014/07/09 22:42:16]

ditto

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:26]

The duration is only specified here because the normal rule of using the
duration between the current and last frame is not what the original test was
expecting. This is just preserving the old test behavior.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

   CheckExpectedRangesByTimestamp("{ [0,12) }");
   CheckExpectedBuffers("0K 2K 4K C 1K 4K 6K 8K 10K");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_ConfigChange) {
   SetAudioStream();
 
   AudioDecoderConfig new_config(kCodecVorbis,
                                 kSampleFormatPlanarF32,
@@ skipping 16 matching lines @@
 
 // Ensure splices are not created if there are not enough frames to crossfade.
 TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_NoTinySplices) {
   SetAudioStream();
   Seek(0);
 
   // Overlap the range [0, 2) with [1, 3).  Since each frame has a duration of
   // 2ms this results in an overlap of 1ms between the ranges.  A splice frame
   // should not be generated since it requires at least 2 frames, or 2ms in this
   // case, of data to crossfade.
-  NewSegmentAppend("0K");
+  NewSegmentAppend("0D2K");

[wolenetz, 2014/07/09 22:42:17]

ditto

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:27]

ditto re: making 1 buffer case explicit.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

   CheckExpectedRangesByTimestamp("{ [0,2) }");
-  NewSegmentAppend("1K");
+  NewSegmentAppend("1D2K");

[wolenetz, 2014/07/09 22:42:17]

ditto

[acolwell GONE FROM CHROMIUM, 2014/07/15 18:49:26]

ditto re: making 1 buffer case explicit.

[wolenetz, 2014/07/15 21:05:37]

Acknowledged.

   CheckExpectedRangesByTimestamp("{ [0,3) }");
   CheckExpectedBuffers("0K 1K");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_Preroll) {
   SetAudioStream();
   Seek(0);
   NewSegmentAppend("0K 2K 4K 6K 8K 10K 12K");
   NewSegmentAppend("11P 13K 15K 17K");
   CheckExpectedBuffers("0K 2K 4K 6K 8K 10K 12K C 11P 13K 15K 17K");
   CheckNoNextBuffer();
 }
 
 TEST_F(SourceBufferStreamTest, Audio_PrerollFrame) {
   Seek(0);
   NewSegmentAppend("0K 3P 6K");
   CheckExpectedBuffers("0K 3P 6K");
   CheckNoNextBuffer();
 }
 
 // TODO(vrk): Add unit tests where keyframes are unaligned between streams.
 // (crbug.com/133557)
 
 // TODO(vrk): Add unit tests with end of stream being called at interesting
 // times.
 
 }  // namespace media