Remove buffer duration calculation from WebMClusterParser and update SourceBufferStream accordingly

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 7ec3f9d16b9c93fd69acab9966495d72adb4dd98..79b89c54c4207b7b771492cd850196c4e1755fd4 100644
--- a/media/filters/source_buffer_stream_unittest.cc
+++ b/media/filters/source_buffer_stream_unittest.cc
@@ -62,7 +62,7 @@ class SourceBufferStreamTest : public testing::Test {
     ss << "{ ";
     for (size_t i = 0; i < r.size(); ++i) {
       int64 start = (r.start(i) / frame_duration_);
-      int64 end = (r.end(i) / frame_duration_) - 1;
+      int64 end = (r.end(i) / frame_duration_);
       ss << "[" << start << "," << end << ") ";
     }
     ss << "}";
@@ -139,8 +139,6 @@ class SourceBufferStreamTest : public testing::Test {
                      base::TimeDelta first_buffer_offset,
                      bool expect_success, const uint8* data, int size) {
     int keyframe_interval = frames_per_second_ / keyframes_per_second_;
-    base::TimeDelta media_segment_start_time =
-        starting_position * frame_duration_;
 
     SourceBufferStream::BufferQueue queue;
     for (int i = 0; i < number_of_buffers; i++) {
@@ -148,17 +146,22 @@ class SourceBufferStreamTest : public testing::Test {
       bool is_keyframe = position % keyframe_interval == 0;
       scoped_refptr<StreamParserBuffer> buffer =
           StreamParserBuffer::CopyFrom(data, size, is_keyframe);
-      base::TimeDelta duration = frame_duration_;
       base::TimeDelta timestamp = frame_duration_ * position;
-      if (i == 0) {
-        duration -= first_buffer_offset;
-        timestamp += first_buffer_offset;
+
+      if (is_keyframe) {

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

Why do we need OnNewMediaSegment() call per keyframe now? It seems like it would
be better to explicitly indicate the media segment boundaries in the test cases
especially since the diagrams show multiple keyframes in a segment.

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Done.

+        if (!queue.empty()) {
+          EXPECT_EQ(expect_success, stream_.Append(queue));
+          queue.clear();
+        }
+        stream_.OnNewMediaSegment(timestamp);
       }
-      buffer->SetDuration(duration);
+      if (i == 0)
+        timestamp += first_buffer_offset;
       buffer->SetTimestamp(timestamp);
       queue.push_back(buffer);
     }
-    EXPECT_EQ(stream_.Append(queue, media_segment_start_time), expect_success);
+    if (!queue.empty())
+      EXPECT_EQ(expect_success, stream_.Append(queue));
   }
 
   int frames_per_second_;
@@ -207,14 +210,14 @@ TEST_F(SourceBufferStreamTest, Append_DisjointRanges) {
 }
 
 TEST_F(SourceBufferStreamTest, Append_AdjacentRanges) {
-  // Append 12 buffers at positions 0 through 11.
-  AppendBuffers(0, 12);
+  // Append 10 buffers at positions 0 through 9.

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

Why did these need to change? Our ability to determine what is adjacent shoudn't
have changed.

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Technically this test testing something that the spec didn't allow: appending a
new media segment that doesn't begin with a keyframe. (The third AppendBuffers()
call is a new media segment, but it starts with 12 which is not a keyframe.)
This worked previously when the code didn't track new media segments....

Updated the SourceBufferStream class to return false during these sorts of
Append()s, and modified Append_DoesNotBeginWithKeyframe to also catch this case.

+  AppendBuffers(0, 10);
 
   // Append 11 buffers at positions 15 through 25.
   AppendBuffers(15, 11);
 
-  // Append 3 buffers at positions 12 through 14 to bridge the gap.
-  AppendBuffers(12, 3);
+  // Append 5 buffers at positions 10 through 14 to bridge the gap.
+  AppendBuffers(10, 5);
 
   // Check expected range.
   CheckExpectedRanges("{ [0,25) }");
@@ -224,8 +227,12 @@ TEST_F(SourceBufferStreamTest, Append_AdjacentRanges) {
 }
 
 TEST_F(SourceBufferStreamTest, Append_DoesNotBeginWithKeyframe) {
+  // NOTE: AppendBuffers() calls OnNewMediaSegment before each appending
+  // each keyframe. But because we're purposely doing it wrong in this test,
+  // we need to manually signal the beginning of a media segment.
+  stream_.OnNewMediaSegment(3 * frame_duration());
   // Append fails because the range doesn't begin with a keyframe.
-  AppendBuffers_ExpectFailure(3, 5);
+  AppendBuffers_ExpectFailure(3, 2);
 
   // Append 10 buffers at positions 5 through 14.
   AppendBuffers(5, 10);
@@ -237,7 +244,9 @@ TEST_F(SourceBufferStreamTest, Append_DoesNotBeginWithKeyframe) {
   CheckExpectedBuffers(5, 14);
 
   // Append fails because the range doesn't begin with a keyframe.
-  AppendBuffers_ExpectFailure(17, 10);
+  stream_.OnNewMediaSegment(17 * frame_duration());
+  AppendBuffers_ExpectFailure(17, 3);
+
   CheckExpectedRanges("{ [5,14) }");
   Seek(5);
   CheckExpectedBuffers(5, 14);
@@ -1242,4 +1251,7 @@ TEST_F(SourceBufferStreamTest, GetNextBuffer_NoSeek) {
 // 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