Return buffers from StreamParsers in a single unified map

media/base/stream_parser.cc

Index: media/base/stream_parser.cc
diff --git a/media/base/stream_parser.cc b/media/base/stream_parser.cc
index 9f3119b33b004e0eb27366d223595587d35e7473..78fff1a173ff697e4b7b11b28a246890fb97077d 100644
--- a/media/base/stream_parser.cc
+++ b/media/base/stream_parser.cc
@@ -111,24 +111,25 @@ static bool MergeBufferQueuesInternal(
   }
 }
 
-bool MergeBufferQueues(const StreamParser::BufferQueue& audio_buffers,
-                       const StreamParser::BufferQueue& video_buffers,
-                       const StreamParser::TextBufferQueueMap& text_buffers,
+bool MergeBufferQueues(const StreamParser::BufferQueueMap& buffer_queue_map,
                        StreamParser::BufferQueue* merged_buffers) {
   DCHECK(merged_buffers);
 
   // Prepare vector containing pointers to any provided non-empty buffer queues.
+
+  // Append audio buffer queues first, before all other types, since

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

Now that we're working on a collection of bufferqueues, ISTM that
MergeBufferQueuesInternal should just work on a BufferQueueMap and any test
buffer ordering sensitivity might be fixed in the test. WDYT?

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

I'd like that, but I couldn't quickly figure out how to fix the
AudioVideo_Discontinuity test, which explicitly relies on audio buffers being
processed before video (see
https://cs.chromium.org/chromium/src/media/filters/frame_processor_unittest.c...).
So I've kept this behavior for now, it shouldn't affect anything else, besides
that test (and maybe runtime performance here, but it shouldn't be too bad
either way).

[wolenetz, 2016/08/24 21:57:51]

Acknowledged. When FrameProcessor TrackId unification is done, then that text
and this part of the merge shim can be fixed. For now, as a quick fix, can you
try swapping the values for FrameProcessor::kAudioTrackId and kVideoTrackId? If
that doesn't regress any other unit test, it should enable that AV discontinuity
test to work and this part of the merge shim could be fixed now.

[servolk, 2016/08/24 23:17:44]

I've tried swapping FrameProcessor kAudio/VideoTrackId, but the
AudioVideo_Discontinuity test still fails after that if we don't process audio
BQs first here (no other tests fail though).

[wolenetz, 2016/08/24 23:52:17]

Silly me, keys aren't sorted in a map :) Sorry for this side-trip.

+  // FrameProcessorTest.AudioVideo_Discontinuity currently depends on audio
+  // buffers being processed first.
   std::vector<const StreamParser::BufferQueue*> buffer_queues;
-  if (!audio_buffers.empty())
-    buffer_queues.push_back(&audio_buffers);
-  if (!video_buffers.empty())
-    buffer_queues.push_back(&video_buffers);
-  for (StreamParser::TextBufferQueueMap::const_iterator map_itr =
-           text_buffers.begin();
-       map_itr != text_buffers.end();
-       map_itr++) {
-    if (!map_itr->second.empty())
-      buffer_queues.push_back(&(map_itr->second));
+  for (const auto& it : buffer_queue_map) {
+    DCHECK(!it.second.empty());
+    if (it.second[0]->type() == DemuxerStream::AUDIO)
+      buffer_queues.push_back(&it.second);
+  }
+  for (const auto& it : buffer_queue_map) {
+    DCHECK(!it.second.empty());
+    if (it.second[0]->type() != DemuxerStream::AUDIO)
+      buffer_queues.push_back(&it.second);
   }
 
   // Do the merge.