Return buffers from StreamParsers in a single unified map

media/base/stream_parser_unittest.cc

 // Copyright 2014 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 <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
+#include <map>
 #include <sstream>
 
 #include "base/macros.h"
 #include "media/base/stream_parser.h"
 #include "media/base/stream_parser_buffer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media {
 
 typedef StreamParser::TrackId TrackId;
 typedef StreamParser::BufferQueue BufferQueue;
-typedef StreamParser::TextBufferQueueMap TextBufferQueueMap;
 
 const int kEnd = -1;
 const uint8_t kFakeData[] = {0xFF};
 const TrackId kAudioTrackId = 0;
 const TrackId kVideoTrackId = 1;
 const TrackId kTextTrackIdA = 2;
 const TrackId kTextTrackIdB = 3;
 
 static bool IsAudio(scoped_refptr<StreamParserBuffer> buffer) {
   return buffer->type() == DemuxerStream::AUDIO;
@@ skipping 40 matching lines @@
       bool (*predicate)(scoped_refptr<StreamParserBuffer> buffer)) {
     return static_cast<size_t>(count_if(merged_buffers_.begin(),
                                         merged_buffers_.end(),
                                         predicate));
   }
 
   // Appends test audio buffers in the sequence described by |decode_timestamps|
   // 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]);
     }
   }
 
   // Returns a string that describes the sequence of buffers in
   // |merged_buffers_|. The string is a concatenation of space-delimited buffer
   // descriptors in the same sequence as |merged_buffers_|. Each descriptor is
   // the concatenation of
   // 1) a single character that describes the buffer's type(), e.g. A, V, or T
   //    for audio, video, or text, respectively
   // 2) the buffer's track_id()
@@ skipping 48 matching lines @@
   // also verified to match the number of buffers in |audio_buffers_|,
   // |video_buffers_|, and |text_map_|, respectively.
   void VerifyMergeSuccess(const std::string& expected,
                           bool verify_type_and_text_track_sequence) {
     // |merged_buffers| may already have some buffers. Count them by type for
     // later inclusion in verification.
     size_t original_audio_in_merged = CountMatchingMergedBuffers(IsAudio);
     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,
               MergedBufferQueueString(verify_type_and_text_track_sequence));
 
     // Verify that the correct number of each type of buffer is in the merge
     // result.
     size_t audio_in_merged = CountMatchingMergedBuffers(IsAudio);
     size_t video_in_merged = CountMatchingMergedBuffers(IsVideo);
     size_t text_in_merged = CountMatchingMergedBuffers(IsText);
 
     EXPECT_GE(audio_in_merged, original_audio_in_merged);
     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);
   }
 
   // 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);
 };
 
 TEST_F(StreamParserTest, MergeBufferQueues_AllEmpty) {
   std::string expected = "";
   VerifyMergeSuccess(expected, true);
 }
 
 TEST_F(StreamParserTest, MergeBufferQueues_SingleAudioBuffer) {
@@ skipping 129 matching lines @@
   VerifyMergeSuccess(expected, true);
 
   // But appending something with a lower timestamp than the last timestamp
   // in the pre-existing merge result should fail.
   int more_audio_timestamps[] = { 106, kEnd };
   GenerateAudioBuffers(more_audio_timestamps);
   VerifyMergeFailure();
 }
 
 }  // namespace media