Fix SourceBufferStream alt-ref frame handling.

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 0b3b95d794264471d9076e37b9e076a70cadc9c8..2683026c32e89066e275afee693cca12b26f7d93 100644
--- a/media/filters/source_buffer_stream_unittest.cc
+++ b/media/filters/source_buffer_stream_unittest.cc
@@ -79,19 +79,27 @@ class SourceBufferStreamTest : public testing::Test {
   }
 
   void NewSegmentAppend(const std::string& buffers_to_append) {
-    AppendBuffers(buffers_to_append, true, false);
+    AppendBuffers(buffers_to_append, true, false, true);
   }
 
   void AppendBuffers(const std::string& buffers_to_append) {
-    AppendBuffers(buffers_to_append, false, false);
+    AppendBuffers(buffers_to_append, false, false, true);
   }
 
   void NewSegmentAppendOneByOne(const std::string& buffers_to_append) {
-    AppendBuffers(buffers_to_append, true, true);
+    AppendBuffers(buffers_to_append, true, true, true);
   }
 
   void AppendBuffersOneByOne(const std::string& buffers_to_append) {
-    AppendBuffers(buffers_to_append, false, true);
+    AppendBuffers(buffers_to_append, false, true, true);
+  }
+
+  void NewSegmentAppend_ExpectFailure(const std::string& buffers_to_append) {
+    AppendBuffers(buffers_to_append, true, false, false);
+  }
+
+  void AppendBuffers_ExpectFailure(const std::string& buffers_to_append) {
+    AppendBuffers(buffers_to_append, false, false, false);
   }
 
   void Seek(int position) {
@@ -188,20 +196,23 @@ class SourceBufferStreamTest : public testing::Test {
   void CheckExpectedBuffers(const std::string& expected) {
     std::vector<std::string> timestamps;
     base::SplitString(expected, ' ', &timestamps);
+    std::stringstream ss;
     for (size_t i = 0; i < timestamps.size(); i++) {
       scoped_refptr<StreamParserBuffer> buffer;
       SourceBufferStream::Status status = stream_->GetNextBuffer(&buffer);
 
+      if (i > 0)
+        ss << " ";
+
       EXPECT_EQ(SourceBufferStream::kSuccess, status);
       if (status != SourceBufferStream::kSuccess)
         break;
 
-      std::stringstream ss;
       ss << buffer->GetDecodeTimestamp().InMilliseconds();
       if (buffer->IsKeyframe())
         ss << "K";
-      EXPECT_EQ(timestamps[i], ss.str());
     }
+    EXPECT_EQ(expected, ss.str());

[acolwell GONE FROM CHROMIUM, 2013/03/11 18:35:15]

Moving the expectation check to here makes it easier to debug test failures.

[scherkus (not reviewing), 2013/03/11 21:23:40]

These days my preference is for helper methods to return values and use
EXPECT_XX() in test bodies, defining your own EXPECT macros if things get
unwieldy (e.g., CheckExpectedBuffers("0K 30 61 120K 150") can be
EXPECT_BUFFERS("0K 30 61 120K 150")).

Why? If a test calls multiple check methods it's not immediately obvious which
one failed since they all point to the same line of code. It also gets around
the problems using ASSERT_XX() inside helper methods.

I don't expect you to change this file -- just food for thought.

[acolwell GONE FROM CHROMIUM, 2013/03/11 22:31:38]

Thanks for the tip. I'll consider changing this in a later CL.

   }
 
   void CheckNoNextBuffer() {
@@ -273,7 +284,8 @@ class SourceBufferStreamTest : public testing::Test {
   }
 
   void AppendBuffers(const std::string& buffers_to_append,
-                     bool start_new_segment, bool one_by_one) {
+                     bool start_new_segment, bool one_by_one,
+                     bool expect_success) {
     std::vector<std::string> timestamps;
     base::SplitString(buffers_to_append, ' ', &timestamps);
 
@@ -304,7 +316,7 @@ class SourceBufferStreamTest : public testing::Test {
     }
 
     if (!one_by_one) {
-      EXPECT_TRUE(stream_->Append(buffers));
+      EXPECT_EQ(expect_success, stream_->Append(buffers));
       return;
     }
 
@@ -1835,7 +1847,7 @@ TEST_F(SourceBufferStreamTest, GetNextBuffer_ExhaustThenStartOverlap) {
   CheckExpectedBuffers(11, 14, &kDataB);
 
   // Replace the next buffer at position 15 with another start overlap.
-  AppendBuffers(15, 2, &kDataA);
+  NewSegmentAppend(15, 2, &kDataA);

[acolwell GONE FROM CHROMIUM, 2013/03/11 18:35:15]

This was a bug in the original test. Since this is supposed to be a start
overlap, it should be a new segment. The new alt-ref frame logic detected this
bug.

   CheckExpectedBuffers(15, 16, &kDataA);
 }
 
@@ -2705,6 +2717,64 @@ TEST_F(SourceBufferStreamTest, OverlapSplitAndMergeWhileWaitingForMoreData) {
   CheckExpectedBuffers("180K 210");
 }
 
+// Verify that non-keyframes with the same timestamp in the same
+// append are handled correctly.
+TEST_F(SourceBufferStreamTest, AltRefFrame_SingleAppend) {
+  Seek(0);
+  NewSegmentAppend("0K 30 30 60 90 120K 150");
+  CheckExpectedBuffers("0K 30 30 60 90 120K 150");
+}
+
+// Verify that non-keyframes with the same timestamp can occur
+// in different appends.
+TEST_F(SourceBufferStreamTest, AltRefFrame_TwoAppends) {
+  Seek(0);
+  NewSegmentAppend("0K 30");
+  AppendBuffers("30 60 90 120K 150");
+  CheckExpectedBuffers("0K 30 30 60 90 120K 150");
+}
+
+// Verify that a non-keyframe followed by a keyframe with the same timestamp
+// is not allowed.
+TEST_F(SourceBufferStreamTest, AltRefFrame_Invalid_1) {
+  Seek(0);
+  NewSegmentAppend("0K 30");
+  AppendBuffers_ExpectFailure("30K 60");
+}
+
+TEST_F(SourceBufferStreamTest, AltRefFrame_Invalid_2) {
+  Seek(0);
+  NewSegmentAppend_ExpectFailure("0K 30 30K 60");
+}
+
+// Verify that a keyframe followed by a non-keyframe with the same timestamp
+// is not allowed.
+TEST_F(SourceBufferStreamTest, AltRefFrame_Invalid_3) {
+  Seek(0);
+  NewSegmentAppend("0K 30K");
+  AppendBuffers_ExpectFailure("30 60");
+}
+
+TEST_F(SourceBufferStreamTest, AltRefFrame_Invalid_4) {
+  Seek(0);
+  NewSegmentAppend_ExpectFailure("0K 30K 30 60");
+}
+
+TEST_F(SourceBufferStreamTest, AltRefFrame_Overlap_1) {
+  Seek(0);
+  NewSegmentAppend("0K 30 60 60 90 120K 150");
+
+  NewSegmentAppend("60K 91 121K 151");
+  CheckExpectedBuffers("0K 30 60K 91 121K 151");
+}
+
+TEST_F(SourceBufferStreamTest, AltRefFrame_Overlap_2) {
+  Seek(0);
+  NewSegmentAppend("0K 30 60 60 90 120K 150");
+  NewSegmentAppend("0K 30 61");
+  CheckExpectedBuffers("0K 30 61 120K 150");
+}
+
 // TODO(vrk): Add unit tests where keyframes are unaligned between streams.
 // (crbug.com/133557)