Update ChunkDemuxer unit tests to use real muxed clusters.

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 379 matching lines @@
         block_duration = kTextBlockDuration;
         break;
     }
     ASSERT_NE(block_duration, 0);
     int end_timecode = timecode + block_count * block_duration;
     AppendCluster(source_id,
                   GenerateSingleStreamCluster(
                       timecode, end_timecode, track_number, block_duration));
   }
 
-  // |cluster_description| - A space delimited string of buffer info that
-  //  is used to construct a cluster. Each buffer info is a timestamp in
-  //  milliseconds and optionally followed by a 'K' to indicate that a buffer
-  //  should be marked as a keyframe. For example "0K 30 60" should constuct
-  //  a cluster with 3 blocks: a keyframe with timestamp 0 and 2 non-keyframes
-  //  at 30ms and 60ms.
-  void AppendSingleStreamCluster(const std::string& source_id, int track_number,
-                                 const std::string& cluster_description) {
+  struct BlockInfo {
+    int track_number;
+    int timestamp_in_ms;
+    int flags;
+    int duration;
+
+    bool operator< (const BlockInfo& rhs) const {
+      // We want pop() to return blocks in timestamp increasing order

[wolenetz, 2014/07/08 19:33:53]

nit: This could become confusing later if BlockInfo used for other purposes.
Could we just use < and populate the blocks vector in AppendMuxedCluster() in
reverse order as we top/pop from the pri queue?

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:38]

Done.

+      // so timestamps with lower values have higher priority.
+      return timestamp_in_ms > rhs.timestamp_in_ms;
+    }
+  };
+
+  // |track_number| - The track number to place in
+  // |blocks_description| - A space delimited string of block info that
+  //  is used to populate |blocks|. Each block info has a timestamp in
+  //  milliseconds and optionally followed by a 'K' to indicate that a block
+  //  should be marked as a keyframe. For example "0K 30 60" should populate
+  //  |blocks| with 3 BlockInfo objects: a keyframe with timestamp 0 |and 2

[wolenetz, 2014/07/08 19:33:53]

nit: s/|and/and

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:38]

Done.

+  //  non-keyframes at 30ms and 60ms.
+  void ParseBlockDescription(int track_number,
+                             const std::string blocks_description,
+                             std::vector<BlockInfo>* blocks) {
     std::vector<std::string> timestamps;
-    base::SplitString(cluster_description, ' ', &timestamps);
+    base::SplitString(blocks_description, ' ', &timestamps);
 
-    ClusterBuilder cb;
-    std::vector<uint8> data(10);
     for (size_t i = 0; i < timestamps.size(); ++i) {
       std::string timestamp_str = timestamps[i];
-      int block_flags = 0;
+      BlockInfo block_info;
+
+      block_info.track_number = track_number;
+      block_info.flags = 0;
+      block_info.duration = 0;
+
       if (EndsWith(timestamp_str, "K", true)) {
-        block_flags = kWebMFlagKeyframe;
+        block_info.flags = kWebMFlagKeyframe;
         // Remove the "K" off of the token.
         timestamp_str = timestamp_str.substr(0, timestamps[i].length() - 1);
       }
-      int timestamp_in_ms;
-      CHECK(base::StringToInt(timestamp_str, &timestamp_in_ms));
-
-      if (i == 0)
-        cb.SetClusterTimecode(timestamp_in_ms);
+      CHECK(base::StringToInt(timestamp_str, &block_info.timestamp_in_ms));
 
       if (track_number == kTextTrackNum ||
           track_number == kAlternateTextTrackNum) {
-        cb.AddBlockGroup(track_number, timestamp_in_ms, kTextBlockDuration,
-                         block_flags, &data[0], data.size());
+        block_info.duration = kTextBlockDuration;
+        ASSERT_EQ(block_info.flags, kWebMFlagKeyframe)

[wolenetz, 2014/07/08 19:33:53]

nit: ASSERT_EQ(expected, actual) per
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
Though this is not really adhered to much in this file.

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:37]

Done.
/me shakes fist at junit.

+            << "Text block with timestamp " << block_info.timestamp_in_ms
+            << " was not marked as a keyframe."
+            << " All text blocks must be keyframes";
+      }
+
+      blocks->push_back(block_info);
+    }
+  }
+
+  scoped_ptr<Cluster> GenerateCluster(const std::vector<BlockInfo>& blocks) {
+    ClusterBuilder cb;

[wolenetz, 2014/07/08 19:33:53]

nit: Add ASSERT_LT(0, blocks.size()); since we won't have a cluster timecode if
blocks is empty; caller could instead explicitly use GenerateEmptyCluster()?

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:38]

Done.

+
+    std::vector<uint8> data(10);
+    for (size_t i = 0; i < blocks.size(); ++i) {
+      if (i == 0)
+        cb.SetClusterTimecode(blocks[i].timestamp_in_ms);
+
+      if (blocks[i].duration) {
+        cb.AddBlockGroup(blocks[i].track_number, blocks[i].timestamp_in_ms,
+                         blocks[i].duration, blocks[i].flags,
+                         &data[0], data.size());
       } else {
-        cb.AddSimpleBlock(track_number, timestamp_in_ms, block_flags,
+        cb.AddSimpleBlock(blocks[i].track_number, blocks[i].timestamp_in_ms,
+                          blocks[i].flags,
                           &data[0], data.size());
       }
     }
-    AppendCluster(source_id, cb.Finish());
+
+    return cb.Finish();
+  }
+
+  // |block_description| - The block description used to construct the cluster.
+  // See the documentation for ParseBlockDescription() for details on the string
+  // format.
+  void AppendSingleStreamCluster(const std::string& source_id, int track_number,
+                                 const std::string& block_description) {
+    std::vector<BlockInfo> blocks;
+    ParseBlockDescription(track_number, block_description, &blocks);
+    AppendCluster(source_id, GenerateCluster(blocks));
   }
 
   struct MuxedStreamInfo {
     MuxedStreamInfo()
         : track_number(0),
-          cluster_description("")
+          block_description("")
     {}
 
-    MuxedStreamInfo(int track_num, const char* cluster_desc)
+    MuxedStreamInfo(int track_num, const char* block_desc)
         : track_number(track_num),
-          cluster_description(cluster_desc) {
+          block_description(block_desc) {
     }
 
     int track_number;
-    // The cluster description passed to AppendSingleStreamCluster().
+    // The block description passed to ParseBlockDescription().
     // See the documentation for that method for details on the string format.
-    const char* cluster_description;
+    const char* block_description;

[wolenetz, 2014/07/08 19:33:53]

nit: s/block_description/block_descriptions or blocks_description  or
block_description. Name similarly the ParseBlockDescription parameter to
whichever you choose here.

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:38]

Done.

   };
 
   void AppendMuxedCluster(const MuxedStreamInfo& msi_1,
                           const MuxedStreamInfo& msi_2) {
     std::vector<MuxedStreamInfo> msi(2);
     msi[0] = msi_1;
     msi[1] = msi_2;
     AppendMuxedCluster(msi);
   }
 
   void AppendMuxedCluster(const MuxedStreamInfo& msi_1,
                           const MuxedStreamInfo& msi_2,
                           const MuxedStreamInfo& msi_3) {
     std::vector<MuxedStreamInfo> msi(3);
     msi[0] = msi_1;
     msi[1] = msi_2;
     msi[2] = msi_3;
     AppendMuxedCluster(msi);
   }
 
   void AppendMuxedCluster(const std::vector<MuxedStreamInfo> msi) {
+    std::priority_queue<BlockInfo> block_queue;
     for (size_t i = 0; i < msi.size(); ++i) {
-      AppendSingleStreamCluster(kSourceId,
-                                msi[i].track_number,
-                                msi[i].cluster_description);
+      std::vector<BlockInfo> track_blocks;
+      ParseBlockDescription(msi[i].track_number, msi[i].block_description,
+                            &track_blocks);
+
+      for (size_t j = 0; j < track_blocks.size(); ++j)
+        block_queue.push(track_blocks[j]);
     }
+
+    std::vector<BlockInfo> blocks(block_queue.size());
+    for (size_t i = 0; !block_queue.empty(); ++i) {
+      blocks[i] = block_queue.top();
+      block_queue.pop();
+    }
+
+    AppendCluster(kSourceId, GenerateCluster(blocks));
   }
 
   void AppendData(const std::string& source_id,
                   const uint8* data, size_t length) {
     EXPECT_CALL(host_, AddBufferedTimeRange(_, _)).Times(AnyNumber());
 
     demuxer_->AppendData(source_id, data, length,
                          append_window_start_for_next_append_,
                          append_window_end_for_next_append_,
                          &timestamp_offset_map_[source_id]);
@@ skipping 218 matching lines @@
         sizeof(kVP8Interframe);
     cb->AddBlockGroup(track_num, timecode, duration, flags, data, size);
   }
 
   scoped_ptr<Cluster> GenerateCluster(int first_audio_timecode,
                                       int first_video_timecode,
                                       int block_count) {
     return GenerateCluster(first_audio_timecode, first_video_timecode,
                            block_count, false);
   }
   scoped_ptr<Cluster> GenerateCluster(int first_audio_timecode,

[wolenetz, 2014/07/08 19:33:53]

We also have this version of GenerateCluster that creates a muxed cluster,
optionally with an 'unknown size'. Wrap the unknown size option into the new
GenerateCluster from blocks_descriptions, above, and rework the tests further to
use just one muxed cluster generator? I'm beginning to think these
Generate*Cluster methods belong within a helper class like ClusterBuilder.
Perhaps for a separate CL?

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:38]

Done. Converted this method to use BlockInfo to generate the cluster. I think
moving code into the ClusterBuilder should be in a different CL.

                                       int first_video_timecode,
                                       int block_count,
                                       bool unknown_size) {
     CHECK_GT(block_count, 0);
 
     int size = 10;
     scoped_ptr<uint8[]> data(new uint8[size]);
 
     ClusterBuilder cb;
     cb.SetClusterTimecode(std::min(first_audio_timecode, first_video_timecode));
@@ skipping 554 matching lines @@
       .WillOnce(SaveArg<0>(&text_stream));
   ASSERT_TRUE(InitDemuxerWithEncryptionInfo(
       HAS_TEXT | HAS_AUDIO | HAS_VIDEO, false, false));
   DemuxerStream* audio_stream = demuxer_->GetStream(DemuxerStream::AUDIO);
   DemuxerStream* video_stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   ASSERT_TRUE(audio_stream && video_stream && text_stream);
 
   AppendMuxedCluster(
       MuxedStreamInfo(kAudioTrackNum, "0 23K"),
       MuxedStreamInfo(kVideoTrackNum, "0 30K"),
-      MuxedStreamInfo(kTextTrackNum, "0 40K"));
-  CheckExpectedRanges(kSourceId, "{ [30,46) }");
+      MuxedStreamInfo(kTextTrackNum, "25K 40K"));

[acolwell GONE FROM CHROMIUM, 2014/07/08 16:32:00]

This was moved to avoid the text keyframe from causing the range to start at 0
which could mask what is going on in the other streams. The original code had a
text keyframe even though the string doesn't show it. I added an assert in
ParseBlockDescription() to make sure that all text blocks are actually marked as
keyframes in the string.

+  CheckExpectedRanges(kSourceId, "{ [23,46) }");

[wolenetz, 2014/07/08 19:33:53]

IIUC, this change from 30 to 23 is unrelated to the text buffers, right?
(Rather, it's related to previous code appended multiple clusters, each
triggering a new media segment notification, and this CL appends one cluster
with minimum surviving timestamp of 23.)

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:37]

That is correct.

 
   AppendInitSegment(HAS_TEXT | HAS_AUDIO | HAS_VIDEO);
   AppendMuxedCluster(
       MuxedStreamInfo(kAudioTrackNum, "46 69K"),
       MuxedStreamInfo(kVideoTrackNum, "60 90K"),
-      MuxedStreamInfo(kTextTrackNum, "80 90K"));
-  CheckExpectedRanges(kSourceId, "{ [30,92) }");
+      MuxedStreamInfo(kTextTrackNum, "80K 90K"));
+  CheckExpectedRanges(kSourceId, "{ [23,92) }");
 
   CheckExpectedBuffers(audio_stream, "23 69");
   CheckExpectedBuffers(video_stream, "30 90");
-
-  // WebM parser marks all text buffers as keyframes.
-  CheckExpectedBuffers(text_stream, "0 40 80 90");
+  CheckExpectedBuffers(text_stream, "25 40 80 90");
 }
 
 // Make sure that the demuxer reports an error if Shutdown()
 // is called before all the initialization segments are appended.
 TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppended) {
   EXPECT_CALL(*this, DemuxerOpened());
   demuxer_->Initialize(
       &host_, CreateInitDoneCB(
           kDefaultDuration(), DEMUXER_ERROR_COULD_NOT_OPEN), true);
 
@@ skipping 1143 matching lines @@
   // Verify that the range extends to the end of the video data.
   CheckExpectedRanges("{ [0,66) }");
 
   // Verify that the range reverts to the intersection when end of stream
   // has been cancelled.
   demuxer_->UnmarkEndOfStream();
   CheckExpectedRanges("{ [0,46) }");
 
   // Append and remove data so that the 2 streams' end ranges do not overlap.
 
-  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(246)));

[acolwell GONE FROM CHROMIUM, 2014/07/08 16:32:00]

This extra call was being triggered because there were 2 appends. Now there is
only one append so only 1 duration change occurs.

   EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(398)));
   AppendMuxedCluster(
       MuxedStreamInfo(kAudioTrackNum, "200K 223K"),
       MuxedStreamInfo(kVideoTrackNum, "200K 233 266 299 332K 365"));
 
   // At this point, the per-stream ranges are as follows:
   // Audio: [0,46) [200,246)
   // Video: [0,66) [200,398)
   CheckExpectedRanges("{ [0,46) [200,246) }");
 
@@ skipping 864 matching lines @@
   // Append a cluster that starts before and ends after the append
   // window.
   AppendMuxedCluster(
       MuxedStreamInfo(kVideoTrackNum,
                       "0K 30 60 90 120K 150 180 210 240K 270 300 330K"),
       MuxedStreamInfo(kTextTrackNum, "0K 100K 200K 300K" ));
 
   // Verify that text cues that start outside the window are not included
   // in the buffer. Also verify that cues that extend beyond the
   // window are not included.
-  CheckExpectedRanges(kSourceId, "{ [120,270) }");
+  CheckExpectedRanges(kSourceId, "{ [100,270) }");

[acolwell GONE FROM CHROMIUM, 2014/07/08 16:32:00]

The text buffer is the first frame within the append window so it becomes the
media segment start time.

   CheckExpectedBuffers(video_stream, "120 150 180 210 240");
   CheckExpectedBuffers(text_stream, "100");
 
   // Extend the append window to [20,650).
   append_window_end_for_next_append_ = base::TimeDelta::FromMilliseconds(650);
 
   // Append more data and verify that a new range is created.
   AppendMuxedCluster(
       MuxedStreamInfo(kVideoTrackNum,
                       "360 390 420K 450 480 510 540K 570 600 630K"),
       MuxedStreamInfo(kTextTrackNum, "400K 500K 600K 700K" ));
-  CheckExpectedRanges(kSourceId, "{ [120,270) [420,630) }");
+  CheckExpectedRanges(kSourceId, "{ [100,270) [400,630) }");
 
   // Seek to the new range and verify that the expected buffers are returned.
   Seek(base::TimeDelta::FromMilliseconds(420));
   CheckExpectedBuffers(video_stream, "420 450 480 510 540 570 600");
   CheckExpectedBuffers(text_stream, "400 500");
 }
 
 TEST_F(ChunkDemuxerTest, StartWaitingForSeekAfterParseError) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
   EXPECT_CALL(host_, OnDemuxerError(PIPELINE_ERROR_DECODE));
@@ skipping 112 matching lines @@
   // Append text cues that start after the seek point and verify that
   // they are returned by Read() calls.
   AppendMuxedCluster(
       MuxedStreamInfo(kAudioTrackNum, "220K 240K 260K 280K"),
       MuxedStreamInfo(kVideoTrackNum, "240K 270 300 330"),
       MuxedStreamInfo(kTextTrackNum, "225K 275K 325K"));
 
   message_loop_.RunUntilIdle();
   EXPECT_TRUE(text_read_done);
 
   // NOTE: we start at 175 here because the buffer at 125 was returned

[wolenetz, 2014/07/08 19:33:53]

nit: Precursor CL introduced change to 275. s/175/275 and s/125/225 to fix the
comment.

[acolwell GONE FROM CHROMIUM, 2014/07/08 22:00:37]

Done.

   // to the pending read initiated above.
   CheckExpectedBuffers(text_stream, "275 325");
 
   // Verify that audio & video streams continue to return expected values.
   CheckExpectedBuffers(audio_stream, "160 180");
   CheckExpectedBuffers(video_stream, "180 210");
 }
 
 TEST_F(ChunkDemuxerTest, ClusterWithUnknownSize) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
@@ skipping 23 matching lines @@
 TEST_F(ChunkDemuxerTest, CuesBetweenClusters) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
 
   AppendCluster(GenerateCluster(0, 0, 4));
   AppendData(kCuesHeader, sizeof(kCuesHeader));
   AppendCluster(GenerateCluster(46, 66, 5));
   CheckExpectedRanges(kSourceId, "{ [0,115) }");
 }
 
 }  // namespace media