Parsing of encoded duration for unencrypted opus streams.

media/formats/webm/webm_cluster_parser_unittest.cc

Index: media/formats/webm/webm_cluster_parser_unittest.cc
diff --git a/media/formats/webm/webm_cluster_parser_unittest.cc b/media/formats/webm/webm_cluster_parser_unittest.cc
index bf19bee1c14db41fde44ef47d6c47b4a7329fb75..d459082f2f8f71a3e08a0ce70c473e3ac5a933ad 100644
--- a/media/formats/webm/webm_cluster_parser_unittest.cc
+++ b/media/formats/webm/webm_cluster_parser_unittest.cc
@@ -4,11 +4,14 @@
 
 #include <algorithm>
 #include <cstdlib>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "media/base/audio_decoder_config.h"
 #include "media/base/decrypt_config.h"
 #include "media/formats/webm/cluster_builder.h"
+#include "media/formats/webm/opus_packet_builder.h"
 #include "media/formats/webm/webm_cluster_parser.h"
 #include "media/formats/webm/webm_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -49,18 +52,24 @@ struct BlockInfo {
   // this BlockGroup. The absolute value is used for parser verification.
   // For simple blocks, this value must be non-negative, and is used only for
   // parser verification.
-  int duration;
+  float duration;
+
   bool use_simple_block;
+
+  // Default data will be used if no data given.
+  const uint8* data;
+  int data_length;
 };
 
 static const BlockInfo kDefaultBlockInfo[] = {
-  { kAudioTrackNum, 0, 23, true },
-  { kAudioTrackNum, 23, 23, true },
-  { kVideoTrackNum, 33, 34, true },  // Assumes not using DefaultDuration
-  { kAudioTrackNum, 46, 23, true },
-  { kVideoTrackNum, 67, 33, false },
-  { kAudioTrackNum, 69, 23, false },
-  { kVideoTrackNum, 100, 33, false },
+    {kAudioTrackNum, 0, 23, true, NULL, 0},
+    {kAudioTrackNum, 23, 23, true, NULL, 0},
+    // Assumes not using DefaultDuration
+    {kVideoTrackNum, 33, 34, true, NULL, 0},
+    {kAudioTrackNum, 46, 23, true, NULL, 0},
+    {kVideoTrackNum, 67, 33, false, NULL, 0},
+    {kAudioTrackNum, 69, 23, false, NULL, 0},
+    {kVideoTrackNum, 100, 33, false, NULL, 0},
 };
 
 static const uint8 kEncryptedFrame[] = {
@@ -74,27 +83,35 @@ static scoped_ptr<Cluster> CreateCluster(int timecode,
   ClusterBuilder cb;
   cb.SetClusterTimecode(0);
 
+  uint8 kDefaultBlockData[] = {0x00};

[wolenetz, 2015/02/03 22:47:02]

nit: space after { and before }

[chcunningham, 2015/02/05 02:48:22]

Done.

+
   for (int i = 0; i < block_count; i++) {
-    uint8 data[] = { 0x00 };
+    const uint8* data;
+    int data_length;
+    if (block_info[i].data != NULL) {
+      data = block_info[i].data;
+      data_length = block_info[i].data_length;
+    } else {
+      data = kDefaultBlockData;
+      data_length = sizeof(kDefaultBlockData);
+    }
+
     if (block_info[i].use_simple_block) {
       CHECK_GE(block_info[i].duration, 0);
-      cb.AddSimpleBlock(block_info[i].track_num,
-                        block_info[i].timestamp,
-                        0, data, sizeof(data));
+      cb.AddSimpleBlock(block_info[i].track_num, block_info[i].timestamp, 0,
+                        data, data_length);
       continue;
     }
 
     if (block_info[i].duration < 0) {
       cb.AddBlockGroupWithoutBlockDuration(block_info[i].track_num,
-                                           block_info[i].timestamp,
-                                           0, data, sizeof(data));
+                                           block_info[i].timestamp, 0, data,
+                                           data_length);
       continue;
     }
 
-    cb.AddBlockGroup(block_info[i].track_num,
-                     block_info[i].timestamp,
-                     block_info[i].duration,
-                     0, data, sizeof(data));
+    cb.AddBlockGroup(block_info[i].track_num, block_info[i].timestamp,
+                     block_info[i].duration, 0, data, data_length);
   }
 
   return cb.Finish();
@@ -161,7 +178,7 @@ static bool VerifyBuffers(const WebMClusterParser::BufferQueue& audio_buffers,
 
     EXPECT_EQ(block_info[i].timestamp, buffer->timestamp().InMilliseconds());
     EXPECT_EQ(std::abs(block_info[i].duration),
-              buffer->duration().InMilliseconds());
+              buffer->duration().InMillisecondsF());
     EXPECT_EQ(expected_type, buffer->type());
     EXPECT_EQ(block_info[i].track_num, buffer->track_id());
   }
@@ -212,7 +229,7 @@ static bool VerifyTextBuffers(
     const scoped_refptr<StreamParserBuffer> buffer = *buffer_iter++;
     EXPECT_EQ(block_info.timestamp, buffer->timestamp().InMilliseconds());
     EXPECT_EQ(std::abs(block_info.duration),
-              buffer->duration().InMilliseconds());
+              buffer->duration().InMillisecondsF());
     EXPECT_EQ(DemuxerStream::TEXT, buffer->type());
     EXPECT_EQ(text_track_num, buffer->track_id());
   }
@@ -248,6 +265,7 @@ class WebMClusterParserTest : public testing::Test {
                                       std::set<int64>(),
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB())) {}
 
  protected:
@@ -270,6 +288,7 @@ class WebMClusterParserTest : public testing::Test {
                                         std::set<int64>(),
                                         std::string(),
                                         std::string(),
+                                        kUnknownAudioCodec,
                                         LogCB()));
   }
 
@@ -305,18 +324,19 @@ TEST_F(WebMClusterParserTest, HeldBackBufferHoldsBackAllTracks) {
                                       std::set<int64>(),
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB()));
 
   const BlockInfo kBlockInfo[] = {
-    { kVideoTrackNum, 0, 33, true },
-    { kAudioTrackNum, 0, 23, false },
-    { kTextTrackNum, 10, 42, false },
-    { kAudioTrackNum, 23, kTestAudioFrameDefaultDurationInMs, true },
-    { kVideoTrackNum, 33, 33, true },
-    { kAudioTrackNum, 36, kTestAudioFrameDefaultDurationInMs, true },
-    { kVideoTrackNum, 66, 33, true },
-    { kAudioTrackNum, 70, kTestAudioFrameDefaultDurationInMs, true },
-    { kAudioTrackNum, 83, kTestAudioFrameDefaultDurationInMs, true },
+      {kVideoTrackNum, 0, 33, true, NULL, 0},
+      {kAudioTrackNum, 0, 23, false, NULL, 0},
+      {kTextTrackNum, 10, 42, false, NULL, 0},
+      {kAudioTrackNum, 23, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kVideoTrackNum, 33, 33, true, NULL, 0},
+      {kAudioTrackNum, 36, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kVideoTrackNum, 66, 33, true, NULL, 0},
+      {kAudioTrackNum, 70, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kAudioTrackNum, 83, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
   };
 
   const int kExpectedBuffersOnPartialCluster[] = {
@@ -444,8 +464,8 @@ TEST_F(WebMClusterParserTest, ParseClusterWithMultipleCalls) {
 // one of these scenarios.
 TEST_F(WebMClusterParserTest, ParseBlockGroup) {
   const BlockInfo kBlockInfo[] = {
-    { kAudioTrackNum, 0, 23, false },
-    { kVideoTrackNum, 33, 34, false },
+      {kAudioTrackNum, 0, 23, false, NULL, 0},
+      {kVideoTrackNum, 33, 34, false, NULL, 0},
   };
   int block_count = arraysize(kBlockInfo);
 
@@ -470,11 +490,11 @@ TEST_F(WebMClusterParserTest, ParseBlockGroup) {
 
 TEST_F(WebMClusterParserTest, ParseSimpleBlockAndBlockGroupMixture) {
   const BlockInfo kBlockInfo[] = {
-    { kAudioTrackNum, 0, 23, true },
-    { kAudioTrackNum, 23, 23, false },
-    { kVideoTrackNum, 33, 34, true },
-    { kAudioTrackNum, 46, 23, false },
-    { kVideoTrackNum, 67, 33, false },
+      {kAudioTrackNum, 0, 23, true, NULL, 0},
+      {kAudioTrackNum, 23, 23, false, NULL, 0},
+      {kVideoTrackNum, 33, 34, true, NULL, 0},
+      {kAudioTrackNum, 46, 23, false, NULL, 0},
+      {kVideoTrackNum, 67, 33, false, NULL, 0},
   };
   int block_count = arraysize(kBlockInfo);
   scoped_ptr<Cluster> cluster(CreateCluster(0, kBlockInfo, block_count));
@@ -497,24 +517,25 @@ TEST_F(WebMClusterParserTest, IgnoredTracks) {
                                       ignored_tracks,
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB()));
 
   const BlockInfo kInputBlockInfo[] = {
-    { kAudioTrackNum, 0,  23, true },
-    { kAudioTrackNum, 23, 23, true },
-    { kVideoTrackNum, 33, 34, true },
-    { kTextTrackNum,  33, 99, true },
-    { kAudioTrackNum, 46, 23, true },
-    { kVideoTrackNum, 67, 34, true },
+      {kAudioTrackNum, 0, 23, true, NULL, 0},
+      {kAudioTrackNum, 23, 23, true, NULL, 0},
+      {kVideoTrackNum, 33, 34, true, NULL, 0},
+      {kTextTrackNum, 33, 99, true, NULL, 0},
+      {kAudioTrackNum, 46, 23, true, NULL, 0},
+      {kVideoTrackNum, 67, 34, true, NULL, 0},
   };
   int input_block_count = arraysize(kInputBlockInfo);
 
   const BlockInfo kOutputBlockInfo[] = {
-    { kAudioTrackNum, 0,  23, true },
-    { kAudioTrackNum, 23, 23, true },
-    { kVideoTrackNum, 33, 34, true },
-    { kAudioTrackNum, 46, 23, true },
-    { kVideoTrackNum, 67, 34, true },
+      {kAudioTrackNum, 0, 23, true, NULL, 0},
+      {kAudioTrackNum, 23, 23, true, NULL, 0},
+      {kVideoTrackNum, 33, 34, true, NULL, 0},
+      {kAudioTrackNum, 46, 23, true, NULL, 0},
+      {kVideoTrackNum, 67, 34, true, NULL, 0},
   };
   int output_block_count = arraysize(kOutputBlockInfo);
 
@@ -542,16 +563,17 @@ TEST_F(WebMClusterParserTest, ParseTextTracks) {
                                       std::set<int64>(),
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB()));
 
   const BlockInfo kInputBlockInfo[] = {
-    { kAudioTrackNum, 0,  23, true },
-    { kAudioTrackNum, 23, 23, true },
-    { kVideoTrackNum, 33, 34, true },
-    { kTextTrackNum,  33, 42, false },
-    { kAudioTrackNum, 46, 23, true },
-    { kTextTrackNum, 55, 44, false },
-    { kVideoTrackNum, 67, 34, true },
+      {kAudioTrackNum, 0, 23, true, NULL, 0},
+      {kAudioTrackNum, 23, 23, true, NULL, 0},
+      {kVideoTrackNum, 33, 34, true, NULL, 0},
+      {kTextTrackNum, 33, 42, false, NULL, 0},
+      {kAudioTrackNum, 46, 23, true, NULL, 0},
+      {kTextTrackNum, 55, 44, false, NULL, 0},
+      {kVideoTrackNum, 67, 34, true, NULL, 0},
   };
   int input_block_count = arraysize(kInputBlockInfo);
 
@@ -579,6 +601,7 @@ TEST_F(WebMClusterParserTest, TextTracksSimpleBlock) {
                                       std::set<int64>(),
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB()));
 
   const BlockInfo kInputBlockInfo[] = {
@@ -616,17 +639,18 @@ TEST_F(WebMClusterParserTest, ParseMultipleTextTracks) {
                                       std::set<int64>(),
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB()));
 
   const BlockInfo kInputBlockInfo[] = {
-    { kAudioTrackNum, 0,  23, true },
-    { kAudioTrackNum, 23, 23, true },
-    { kVideoTrackNum, 33, 34, true },
-    { kSubtitleTextTrackNum,  33, 42, false },
-    { kAudioTrackNum, 46, 23, true },
-    { kCaptionTextTrackNum, 55, 44, false },
-    { kVideoTrackNum, 67, 34, true },
-    { kSubtitleTextTrackNum,  67, 33, false },
+      {kAudioTrackNum, 0, 23, true, NULL, 0},
+      {kAudioTrackNum, 23, 23, true, NULL, 0},
+      {kVideoTrackNum, 33, 34, true, NULL, 0},
+      {kSubtitleTextTrackNum, 33, 42, false, NULL, 0},
+      {kAudioTrackNum, 46, 23, true, NULL, 0},
+      {kCaptionTextTrackNum, 55, 44, false, NULL, 0},
+      {kVideoTrackNum, 67, 34, true, NULL, 0},
+      {kSubtitleTextTrackNum, 67, 33, false, NULL, 0},
   };
   int input_block_count = arraysize(kInputBlockInfo);
 
@@ -662,6 +686,7 @@ TEST_F(WebMClusterParserTest, ParseEncryptedBlock) {
                                       std::set<int64>(),
                                       std::string(),
                                       "video_key_id",
+                                      kUnknownAudioCodec,
                                       LogCB()));
   int result = parser_->Parse(cluster->data(), cluster->size());
   EXPECT_EQ(cluster->size(), result);
@@ -683,6 +708,7 @@ TEST_F(WebMClusterParserTest, ParseBadEncryptedBlock) {
                                       std::set<int64>(),
                                       std::string(),
                                       "video_key_id",
+                                      kUnknownAudioCodec,
                                       LogCB()));
   int result = parser_->Parse(cluster->data(), cluster->size());
   EXPECT_EQ(-1, result);
@@ -722,6 +748,7 @@ TEST_F(WebMClusterParserTest, ParseInvalidTextBlockGroupWithoutDuration) {
                                       std::set<int64>(),
                                       std::string(),
                                       std::string(),
+                                      kUnknownAudioCodec,
                                       LogCB()));
 
   const BlockInfo kBlockInfo[] = {
@@ -741,13 +768,13 @@ TEST_F(WebMClusterParserTest, ParseWithDefaultDurationsSimpleBlocks) {
   EXPECT_LT(kTestVideoFrameDefaultDurationInMs, 33);
 
   const BlockInfo kBlockInfo[] = {
-    { kAudioTrackNum, 0, kTestAudioFrameDefaultDurationInMs, true },
-    { kAudioTrackNum, 23, kTestAudioFrameDefaultDurationInMs, true },
-    { kVideoTrackNum, 33, kTestVideoFrameDefaultDurationInMs, true },
-    { kAudioTrackNum, 46, kTestAudioFrameDefaultDurationInMs, true },
-    { kVideoTrackNum, 67, kTestVideoFrameDefaultDurationInMs, true },
-    { kAudioTrackNum, 69, kTestAudioFrameDefaultDurationInMs, true },
-    { kVideoTrackNum, 100, kTestVideoFrameDefaultDurationInMs, true },
+      {kAudioTrackNum, 0, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kAudioTrackNum, 23, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kVideoTrackNum, 33, kTestVideoFrameDefaultDurationInMs, true, NULL, 0},
+      {kAudioTrackNum, 46, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kVideoTrackNum, 67, kTestVideoFrameDefaultDurationInMs, true, NULL, 0},
+      {kAudioTrackNum, 69, kTestAudioFrameDefaultDurationInMs, true, NULL, 0},
+      {kVideoTrackNum, 100, kTestVideoFrameDefaultDurationInMs, true, NULL, 0},
   };
 
   int block_count = arraysize(kBlockInfo);
@@ -778,13 +805,15 @@ TEST_F(WebMClusterParserTest, ParseWithoutAnyDurationsSimpleBlocks) {
   // as the lowest non-zero duration seen so far if the last buffer in the track
   // in the cluster (independently for each track in the cluster).
   const BlockInfo kBlockInfo1[] = {
-    { kAudioTrackNum, 0, 23, true },
-    { kAudioTrackNum, 23, 22, true },
-    { kVideoTrackNum, 33, 33, true },
-    { kAudioTrackNum, 45, 23, true },
-    { kVideoTrackNum, 66, 34, true },
-    { kAudioTrackNum, 68, 22, true },  // Estimated from minimum audio dur
-    { kVideoTrackNum, 100, 33, true },  // Estimated from minimum video dur
+      {kAudioTrackNum, 0, 23, true, NULL, 0},
+      {kAudioTrackNum, 23, 22, true, NULL, 0},
+      {kVideoTrackNum, 33, 33, true, NULL, 0},
+      {kAudioTrackNum, 45, 23, true, NULL, 0},
+      {kVideoTrackNum, 66, 34, true, NULL, 0},
+      // Estimated from minimum audio dur
+      {kAudioTrackNum, 68, 22, true, NULL, 0},
+      // Estimated from minimum video dur
+      {kVideoTrackNum, 100, 33, true, NULL, 0},
   };
 
   int block_count1 = arraysize(kBlockInfo1);
@@ -812,8 +841,10 @@ TEST_F(WebMClusterParserTest, ParseWithoutAnyDurationsSimpleBlocks) {
   // Verify that the estimated frame duration is tracked across clusters for
   // each track.
   const BlockInfo kBlockInfo2[] = {
-    { kAudioTrackNum, 200, 22, true },  // Estimate carries over across clusters
-    { kVideoTrackNum, 201, 33, true },  // Estimate carries over across clusters
+      // Estimate carries over across clusters
+      {kAudioTrackNum, 200, 22, true, NULL, 0},
+      // Estimate carries over across clusters
+      {kVideoTrackNum, 201, 33, true, NULL, 0},
   };
 
   int block_count2 = arraysize(kBlockInfo2);
@@ -831,14 +862,16 @@ TEST_F(WebMClusterParserTest, ParseWithoutAnyDurationsBlockGroups) {
   // cluster, and are estimated as the lowest non-zero duration seen so far if
   // the last buffer in the track in the cluster (independently for each track
   // in the cluster).
-    const BlockInfo kBlockInfo1[] = {
-    { kAudioTrackNum, 0, -23, false },
-    { kAudioTrackNum, 23, -22, false },
-    { kVideoTrackNum, 33, -33, false },
-    { kAudioTrackNum, 45, -23, false },
-    { kVideoTrackNum, 66, -34, false },
-    { kAudioTrackNum, 68, -22, false },  // Estimated from minimum audio dur
-    { kVideoTrackNum, 100, -33, false },  // Estimated from minimum video dur
+  const BlockInfo kBlockInfo1[] = {
+      {kAudioTrackNum, 0, -23, false, NULL, 0},
+      {kAudioTrackNum, 23, -22, false, NULL, 0},
+      {kVideoTrackNum, 33, -33, false, NULL, 0},
+      {kAudioTrackNum, 45, -23, false, NULL, 0},
+      {kVideoTrackNum, 66, -34, false, NULL, 0},
+      // Estimated from minimum audio dur
+      {kAudioTrackNum, 68, -22, false, NULL, 0},
+      // Estimated from minimum video dur
+      {kVideoTrackNum, 100, -33, false, NULL, 0},
   };
 
   int block_count1 = arraysize(kBlockInfo1);
@@ -866,8 +899,8 @@ TEST_F(WebMClusterParserTest, ParseWithoutAnyDurationsBlockGroups) {
   // Verify that the estimated frame duration is tracked across clusters for
   // each track.
   const BlockInfo kBlockInfo2[] = {
-    { kAudioTrackNum, 200, -22, false },
-    { kVideoTrackNum, 201, -33, false },
+      {kAudioTrackNum, 200, -22, false, NULL, 0},
+      {kVideoTrackNum, 201, -33, false, NULL, 0},
   };
 
   int block_count2 = arraysize(kBlockInfo2);
@@ -887,13 +920,18 @@ TEST_F(WebMClusterParserTest,
   EXPECT_LT(kTestVideoFrameDefaultDurationInMs, 33);
 
   const BlockInfo kBlockInfo[] = {
-    { kAudioTrackNum, 0, -kTestAudioFrameDefaultDurationInMs, false },
-    { kAudioTrackNum, 23, -kTestAudioFrameDefaultDurationInMs, false },
-    { kVideoTrackNum, 33, -kTestVideoFrameDefaultDurationInMs, false },
-    { kAudioTrackNum, 46, -kTestAudioFrameDefaultDurationInMs, false },
-    { kVideoTrackNum, 67, -kTestVideoFrameDefaultDurationInMs, false },
-    { kAudioTrackNum, 69, -kTestAudioFrameDefaultDurationInMs, false },
-    { kVideoTrackNum, 100, -kTestVideoFrameDefaultDurationInMs, false },
+      {kAudioTrackNum, 0, -kTestAudioFrameDefaultDurationInMs, false, NULL, 0},
+      {kAudioTrackNum, 23, -kTestAudioFrameDefaultDurationInMs, false, NULL, 0},
+      {kVideoTrackNum, 33, -kTestVideoFrameDefaultDurationInMs, false, NULL, 0},
+      {kAudioTrackNum, 46, -kTestAudioFrameDefaultDurationInMs, false, NULL, 0},
+      {kVideoTrackNum, 67, -kTestVideoFrameDefaultDurationInMs, false, NULL, 0},
+      {kAudioTrackNum, 69, -kTestAudioFrameDefaultDurationInMs, false, NULL, 0},
+      {kVideoTrackNum,
+       100,
+       -kTestVideoFrameDefaultDurationInMs,
+       false,
+       NULL,
+       0},
   };
 
   int block_count = arraysize(kBlockInfo);
@@ -938,20 +976,57 @@ TEST_F(WebMClusterParserTest,
   ASSERT_TRUE(VerifyBuffers(parser_, kBlockInfo, block_count));
 }
 
-TEST_F(WebMClusterParserTest,
-       ParseDegenerateClusterWithDefaultDurationsYieldsDefaultDurations) {

[wolenetz, 2015/02/03 22:47:02]

Hmm. Do we no longer have DefaultDuration tested? Perhaps keep this by testing
with a codec that indicates the data is encrypted (and therefore skips the
attempt at extracting the encoded duration from the data, and falls back to
TrackEntry->DefaultDuration parsing)?

[chcunningham, 2015/02/05 02:48:22]

Done with tweaks. As you know, the preference of duration sources is:

1. encoded
2. BlockDuration
3. track DefaultDuration
4. estimation...

I didn't mean to remove this test. I think it covers well the fallback from #2
to #3, so I've re-added it as-is.

For the encryption proposal, I've added a new test with encrypted opus data that
should fallback to BlockDuration. I think this is good to cover the fallback
from #1 to #2 and is more direct than checking for #1 -> #3. 

I think with this, we're pretty well covered.

-  ResetParserToHaveDefaultDurations();
-
-  const BlockInfo kBlockInfo[] = {
-    { kAudioTrackNum, 0, kTestAudioFrameDefaultDurationInMs, true },
-    { kVideoTrackNum, 0, kTestVideoFrameDefaultDurationInMs, true },
-  };
+TEST_F(WebMClusterParserTest, ReadOpusDurationsSimpleBlockAtEndOfCluster) {
+  // Reset parser to expect Opus codec audio.
+  parser_.reset(new WebMClusterParser(
+      kTimecodeScale, kAudioTrackNum, kNoTimestamp(), kVideoTrackNum,
+      kNoTimestamp(), TextTracks(), std::set<int64>(), std::string(),
+      std::string(), kCodecOpus, LogCB()));
+
+  for (const auto& packet_ptr : BuildAllOpusPackets()) {
+    const BlockInfo kBlockInfo[] = {{kAudioTrackNum,
+                                     0,
+                                     packet_ptr->duration_ms,
+                                     true,  // Make it a SimpleBlock.
+                                     &(packet_ptr->data[0]),
+                                     packet_ptr->data.size()}};
+
+    int block_count = arraysize(kBlockInfo);

[wolenetz, 2015/02/03 22:47:02]

isn't this 1 always?

[chcunningham, 2015/02/05 02:48:22]

Yeah, the whole file is silly this way. Would you support changing the whole
file? I'm flexible, I just want to be consistent. Changing to literals seems
aligned with the wisdom of this TotT, but its really minor...

http://googletesting.blogspot.com/2014/07/testing-on-toilet-dont-put-logic-in...

[wolenetz, 2015/02/05 23:04:59]

tott aside, let's keep consistent. Maybe a helper would work better. I'm fine
with this as-is.

+    scoped_ptr<Cluster> cluster(CreateCluster(0, kBlockInfo, block_count));
+    int result = parser_->Parse(cluster->data(), cluster->size());
+    EXPECT_EQ(cluster->size(), result);
+    ASSERT_TRUE(VerifyBuffers(parser_, kBlockInfo, block_count));
+  }
+}
 
-  int block_count = arraysize(kBlockInfo);
-  scoped_ptr<Cluster> cluster(CreateCluster(0, kBlockInfo, block_count));
-  int result = parser_->Parse(cluster->data(), cluster->size());
-  EXPECT_EQ(cluster->size(), result);
-  ASSERT_TRUE(VerifyBuffers(parser_, kBlockInfo, block_count));
+TEST_F(WebMClusterParserTest, PreferOpusDurationsOverBlockDurations) {
+  // Reset parser to expect Opus codec audio.
+  parser_.reset(new WebMClusterParser(
+      kTimecodeScale, kAudioTrackNum, kNoTimestamp(), kVideoTrackNum,
+      kNoTimestamp(), TextTracks(), std::set<int64>(), std::string(),
+      std::string(), kCodecOpus, LogCB()));
+
+  for (const auto& packet_ptr : BuildAllOpusPackets()) {
+    // Setting BlockDuration != Opus duration to see which one the parser uses.
+    int block_duration_ms = packet_ptr->duration_ms + 10;
+
+    BlockInfo block_infos[] = {{kAudioTrackNum,
+                                0,
+                                block_duration_ms,
+                                false,  // Not a SimpleBlock.
+                                &(packet_ptr->data[0]),
+                                packet_ptr->data.size()}};
+
+    int block_count = arraysize(block_infos);

[wolenetz, 2015/02/03 22:47:02]

isn't this 1 always?

[chcunningham, 2015/02/05 02:48:22]

See other reply

[wolenetz, 2015/02/05 23:04:59]

Acknowledged.

+    scoped_ptr<Cluster> cluster(CreateCluster(0, block_infos, block_count));
+    int result = parser_->Parse(cluster->data(), cluster->size());
+    EXPECT_EQ(cluster->size(), result);
+
+    // BlockInfo duration will be used to verify buffer duration, so changing
+    // duration to be that of the Opus packet to verify it was preferred.
+    block_infos[0].duration = packet_ptr->duration_ms;

[wolenetz, 2015/02/03 22:47:02]

nit: I assume that the next line would fail without this line. Can you please
add a line that does ASSERT_FALSE(VerifyBuffers(....)) before this line to
confirm my assumption :)
Note: Such change will likely require fixing VerifyBuffers to not have
"EXPECT_EQ" on duration, but rather return false if duration mismatches.

[chcunningham, 2015/02/05 02:48:22]

I think this test is quite strong. I can't think of way to "fake" passing
outside of making BlockDuraiton = EncodedDuration, which we are certain don't
match because we set them to be different right there in the test...

int block_duration_ms = packet_ptr->duration_ms() + 10;

IMHO Adding the proposed line might help readability, but changing the return of
the VerifyBuffers to specialize for duration smells funny - the return signal is
too narrow given all the things the function verifies.

I considered calling out the buffer duration explicitly like so... 

    const scoped_refptr<media::StreamParserBuffer> audio_buffer 
        = parser_->GetAudioBuffers()[0];
    EXPECT_NE(block_duration_ms, audio_buffer->duration().InMillisecondsF());
    EXPECT_EQ(packet_ptr->duration_ms(),
audio_buffer->duration().InMillisecondsF());

But to me it might actually be more confusing as long as I'm still changing the
duration in BlockInfo and calling VerifyBuffers after that (which is worth doing
given all the other things it verifies).

[wolenetz, 2015/02/05 23:04:59]

The test is strong. I think changing the EXPECT_EQ to return DVLOG + return
false in VerifyBuffers() would be good regardless, since ALL calls to
VerifyBuffers() previously asserted the retval was true. This enables us to do a
negative test to enhance readability here.

Please also add some verification that at least some nonzero number of
iterations of this inner loop actually occur.

+    ASSERT_TRUE(VerifyBuffers(parser_, block_infos, block_count));
+  }
 }
 
 }  // namespace media