Return buffers from StreamParsers in a single unified map

media/base/stream_parser_unittest.cc

Index: media/base/stream_parser_unittest.cc
diff --git a/media/base/stream_parser_unittest.cc b/media/base/stream_parser_unittest.cc
index 4aa96170124b2cdff52949f843cac42140eb967a..d44469808e50a179b7fd82c909c227411478d380 100644
--- a/media/base/stream_parser_unittest.cc
+++ b/media/base/stream_parser_unittest.cc
@@ -6,6 +6,7 @@
 #include <stdint.h>
 
 #include <algorithm>
+#include <map>
 #include <sstream>
 
 #include "base/macros.h"
@@ -17,7 +18,6 @@ namespace media {
 
 typedef StreamParser::TrackId TrackId;
 typedef StreamParser::BufferQueue BufferQueue;
-typedef StreamParser::TextBufferQueueMap TextBufferQueueMap;
 
 const int kEnd = -1;
 const uint8_t kFakeData[] = {0xFF};
@@ -78,36 +78,32 @@ class StreamParserTest : public testing::Test {
   // to |audio_buffers_|. See GenerateBuffers() for |decode_timestamps| format.
   void GenerateAudioBuffers(const int* decode_timestamps) {
     GenerateBuffers(decode_timestamps, DemuxerStream::AUDIO, kAudioTrackId,
-                    &audio_buffers_);
+                    &buffer_queue_map_[kAudioTrackId]);
   }
 
   // Appends test video buffers in the sequence described by |decode_timestamps|
   // to |video_buffers_|. See GenerateBuffers() for |decode_timestamps| format.
   void GenerateVideoBuffers(const int* decode_timestamps) {
     GenerateBuffers(decode_timestamps, DemuxerStream::VIDEO, kVideoTrackId,
-                    &video_buffers_);
+                    &buffer_queue_map_[kVideoTrackId]);
   }
 
   // Current tests only need up to two distinct text BufferQueues. This helper
-  // conditionally appends buffers to the underlying |text_buffers_a_| and
-  // |text_buffers_b_| and conditionally inserts these BufferQueues into
-  // |text_map_| keyed by the respective track ID. If |decode_timestamps_{a,b}|
-  // is NULL, then the corresponding BufferQueue is neither appended to nor
-  // inserted into |text_map_| (though it may previously have been inserted).
+  // conditionally appends buffers to the underlying |buffer_queue_map_| keyed
+  // by the respective track ID. If |decode_timestamps_{a,b}|
+  // is NULL, then the corresponding BufferQueue is not changed at all.
   // Note that key collision on map insertion does not replace the previous
   // value.
   void GenerateTextBuffers(const int* decode_timestamps_a,
                            const int* decode_timestamps_b) {
     if (decode_timestamps_a) {
       GenerateBuffers(decode_timestamps_a, DemuxerStream::TEXT, kTextTrackIdA,
-                      &text_buffers_a_);
-      text_map_.insert(std::make_pair(kTextTrackIdA, text_buffers_a_));
+                      &buffer_queue_map_[kTextTrackIdA]);
     }
 
     if (decode_timestamps_b) {
       GenerateBuffers(decode_timestamps_b, DemuxerStream::TEXT, kTextTrackIdB,
-                      &text_buffers_b_);
-      text_map_.insert(std::make_pair(kTextTrackIdB, text_buffers_b_));
+                      &buffer_queue_map_[kTextTrackIdB]);
     }
   }
 
@@ -176,8 +172,12 @@ class StreamParserTest : public testing::Test {
     size_t original_video_in_merged = CountMatchingMergedBuffers(IsVideo);
     size_t original_text_in_merged = CountMatchingMergedBuffers(IsText);
 
-    EXPECT_TRUE(MergeBufferQueues(audio_buffers_, video_buffers_, text_map_,
-                                  &merged_buffers_));
+    StreamParser::BufferQueueMap buffer_queue_map;
+    for (const auto& it : buffer_queue_map_) {

[wolenetz, 2016/08/23 21:31:21]

why are we copying bqm into bqm_ here? Why not just use bqm_ in call to
MergeBufferQueues?

[servolk, 2016/08/23 23:56:32]

The second parameter of StreamParser::TextBufferQueueMap used to be a 
'const BufferQueue'. So in the first iteration of this CL I've simply renamed
TextBufferQueueMap into a BufferQueueMap and kept the second map parameter
'const BufferQueue', which made this necessary. But then I realized that it's
probably better to allow stream parsers to generate BQMs themselves, and in that
case the BMQ should contain non-const BQs. So I've changed BQ to non-const, but
only updated the places where the broke the compilation and/or unit tests. Now
updated this one as well.

+      if (!it.second.empty())
+        buffer_queue_map.insert(std::make_pair(it.first, it.second));
+    }
+    EXPECT_TRUE(MergeBufferQueues(buffer_queue_map, &merged_buffers_));
 
     // Verify resulting contents of |merged_buffers| matches |expected|.
     EXPECT_EQ(expected,
@@ -193,17 +193,14 @@ class StreamParserTest : public testing::Test {
     EXPECT_GE(video_in_merged, original_video_in_merged);
     EXPECT_GE(text_in_merged, original_text_in_merged);
 
-    EXPECT_EQ(audio_buffers_.size(),
+    EXPECT_EQ(buffer_queue_map_[kAudioTrackId].size(),
               audio_in_merged - original_audio_in_merged);
-    EXPECT_EQ(video_buffers_.size(),
+    EXPECT_EQ(buffer_queue_map_[kVideoTrackId].size(),
               video_in_merged - original_video_in_merged);
 
     size_t expected_text_buffer_count = 0;
-    for (TextBufferQueueMap::const_iterator itr = text_map_.begin();
-         itr != text_map_.end();
-         ++itr) {
-      expected_text_buffer_count += itr->second.size();
-    }
+    expected_text_buffer_count += buffer_queue_map_[kTextTrackIdA].size();
+    expected_text_buffer_count += buffer_queue_map_[kTextTrackIdB].size();
     EXPECT_EQ(expected_text_buffer_count,
               text_in_merged - original_text_in_merged);
   }
@@ -211,28 +208,24 @@ class StreamParserTest : public testing::Test {
   // Verifies that MergeBufferQueues() of the current |audio_buffers_|,

[wolenetz, 2016/08/23 21:31:21]

nit: update comment. these members no longer exist.

[servolk, 2016/08/23 23:56:31]

Done.

   // |video_buffers_|, |text_map_|, and |merged_buffers_| returns false.
   void VerifyMergeFailure() {
-    EXPECT_FALSE(MergeBufferQueues(audio_buffers_, video_buffers_, text_map_,
-                                   &merged_buffers_));
+    StreamParser::BufferQueueMap buffer_queue_map;

[wolenetz, 2016/08/23 21:31:21]

ditto: why copy bqm_ into bqm?

[servolk, 2016/08/23 23:56:32]

see above.

+    for (const auto& it : buffer_queue_map_) {
+      if (!it.second.empty())
+        buffer_queue_map.insert(std::make_pair(it.first, it.second));
+    }
+    EXPECT_FALSE(MergeBufferQueues(buffer_queue_map, &merged_buffers_));
   }
 
   // Helper to allow tests to clear all the input BufferQueues (except
-  // |merged_buffers_|) and the TextBufferQueueMap that are used in
+  // |merged_buffers_|) and the BufferQueueMap that are used in
   // VerifyMerge{Success/Failure}().
   void ClearQueuesAndTextMapButKeepAnyMergedBuffers() {

[wolenetz, 2016/08/23 21:31:21]

nit: rename seems appropriate (ClearBufferQueuesButKeep.... ? )

[servolk, 2016/08/23 23:56:32]

Done.

-    audio_buffers_.clear();
-    video_buffers_.clear();
-    text_buffers_a_.clear();
-    text_buffers_b_.clear();
-    text_map_.clear();
+    buffer_queue_map_.clear();
   }
 
  private:
-  BufferQueue audio_buffers_;
-  BufferQueue video_buffers_;
-  BufferQueue text_buffers_a_;
-  BufferQueue text_buffers_b_;
+  std::map<StreamParser::TrackId, StreamParser::BufferQueue> buffer_queue_map_;

[wolenetz, 2016/08/23 21:31:21]

nit: StreamParser::BufferQueueMap ?

[servolk, 2016/08/23 23:56:31]

Done.

   BufferQueue merged_buffers_;
-  TextBufferQueueMap text_map_;
 
   DISALLOW_COPY_AND_ASSIGN(StreamParserTest);
 };