Return buffers from StreamParsers in a single unified map

Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581
Committed: https://crrev.com/83f4c6d26c21cb52c5ec8f4e1d3b409a606e4377
Cr-Commit-Position: refs/heads/master@{#414274}

[servolk, 2016-08-18 01:37:07]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 01:37:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 01:50:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 01:50:34]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[servolk, 2016-08-18 02:03:48]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 02:04:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-08-18 02:19:40]

Description was changed from

==========
Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581
==========

to

==========
Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581
==========

[servolk, 2016-08-18 02:19:40]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, wolenetz@chromium.org

[commit-bot: I haz the power, 2016-08-18 03:41:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 03:41:05]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-08-18 21:44:21]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 21:45:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2016-08-19 01:06:25]

Seems reasonable to me, defer to wolenetz@ for review. I think there are some
cases here where you could use emplace to avoid make_pair as well as inserting
whole swaths at once with an iterator based insert, but I haven't looked deeply.

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
media/base/stream_parser.cc:123: std::vector<const StreamParser::BufferQueue*>
buffer_queues;
Isn't there an insert() method that can do this without the iteration?

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:175: for (const auto& it :
buffer_queue_map_)
Needs {} -- again it seems like there should be an emplace/insert method for
doing this.

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:210: for (const auto& it :
buffer_queue_map_)
needs {}, ditto.

[servolk, 2016-08-19 01:29:29]

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:175: for (const auto& it :
buffer_queue_map_)
On 2016/08/19 01:06:25, DaleCurtis wrote:
> Needs {} -- again it seems like there should be an emplace/insert method for
> doing this.

Sure I can add {}.
According to https://chromium-cpp.appspot.com/:
std::map::emplace() is not yet available on all libstdc++ versions we support.

I've just tried and indeed std::map::emplace is not found:
../../media/base/stream_parser_unittest.cc:178:26: error: no member named
'emplace' in 'std::__debug::map<int,
std::__debug::deque<scoped_refptr<media::StreamParserBuffer>,
std::allocator<scoped_refptr<media::StreamParserBuffer> > >, std::less<int>,
std::allocator<std::pair<const int,
std::__debug::deque<scoped_refptr<media::StreamParserBuffer>,
std::allocator<scoped_refptr<media::StreamParserBuffer> > > > > >'
        buffer_queue_map.emplace(it.first, it.second);
        ~~~~~~~~~~~~~~~~ ^

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:210: for (const auto& it :
buffer_queue_map_)
On 2016/08/19 01:06:25, DaleCurtis wrote:
> needs {}, ditto.

Acknowledged.

[servolk, 2016-08-19 01:37:31]

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/40001/media/base/stream_parse...
media/base/stream_parser.cc:123: std::vector<const StreamParser::BufferQueue*>
buffer_queues;
On 2016/08/19 01:06:25, DaleCurtis wrote:
> Isn't there an insert() method that can do this without the iteration?

Do you mean something like std::copy_if or something else?

[servolk, 2016-08-19 01:40:07]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 01:40:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 03:26:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 03:26:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[servolk, 2016-08-19 18:34:09]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 18:34:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wolenetz, 2016-08-19 20:06:50]

I haven't reviewed this full CL, rather from the description alone I'm wondering
the utility of this versus actual usage of the newly emitted buffers.

Please first consult
https://bugs.chromium.org/p/chromium/issues/detail?id=338484 and the usage of
StreamParser::MergeBufferQueues() as a current shim.

tl;dr: The coded frame processing algorithm processes new frames *from all
tracks* in DTS sequence. Though the parsers emit such frames in distinct, per
type, queues (and a map of text queues), the frame processor immediately merges
these in DTS sequence using the utility method.

Versus making the parsers do the merging themselves, what is the utility of
further breaking out the buffers by track in the longer run?

[commit-bot: I haz the power, 2016-08-19 21:03:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 21:03:45]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-08-19 22:20:33]

On 2016/08/19 20:06:50, wolenetz wrote:
> I haven't reviewed this full CL, rather from the description alone I'm
wondering
> the utility of this versus actual usage of the newly emitted buffers.
> 
> Please first consult
> https://bugs.chromium.org/p/chromium/issues/detail?id=338484 and the usage of
> StreamParser::MergeBufferQueues() as a current shim.
> 
> tl;dr: The coded frame processing algorithm processes new frames *from all
> tracks* in DTS sequence. Though the parsers emit such frames in distinct, per
> type, queues (and a map of text queues), the frame processor immediately
merges
> these in DTS sequence using the utility method.
> 
> Versus making the parsers do the merging themselves, what is the utility of
> further breaking out the buffers by track in the longer run?

Yeah, that's a good point and I was pondering this myself. Now that each emitted
buffer has a proper track id assigned to it we can go even further and flatten
the BufferQueueMap into a single BufferQueue (and that's essentially what
FrameProcessor does). But there is a lot of code (mostly in unit tests, but also
in MediaSourceState::OnNewBuffers) that relied on being able to access only
audio or only video buffers emitted by the parser. So for now I've decided to
keep them separate until they are passed into FrameProcessor which will flatten
that map into a single stream of buffers for further processing (essentially the
same as we do currently, only with a slightly different input structure - a
unified map rather than separate BufferQueues + the map for text tracks). We can
certainly look into simplifying it further in the future, but I think this CL
represents a nice intermediate step that will unblock, for example, adding
support for multiple track in stream parsers (e.g. here is the next CL that I
was planning to land https://codereview.chromium.org/2254733006/, that will
allow .mp4 parser to handle multiple tracks). So the short answer: it was simply
easier and quicker to do it this way given the current code structure.

[wolenetz, 2016-08-19 22:38:40]

On 2016/08/19 22:20:33, servolk wrote:
> On 2016/08/19 20:06:50, wolenetz wrote:
> > I haven't reviewed this full CL, rather from the description alone I'm
> wondering
> > the utility of this versus actual usage of the newly emitted buffers.
> > 
> > Please first consult
> > https://bugs.chromium.org/p/chromium/issues/detail?id=338484 and the usage
of
> > StreamParser::MergeBufferQueues() as a current shim.
> > 
> > tl;dr: The coded frame processing algorithm processes new frames *from all
> > tracks* in DTS sequence. Though the parsers emit such frames in distinct,
per
> > type, queues (and a map of text queues), the frame processor immediately
> merges
> > these in DTS sequence using the utility method.
> > 
> > Versus making the parsers do the merging themselves, what is the utility of
> > further breaking out the buffers by track in the longer run?
> 
> Yeah, that's a good point and I was pondering this myself. Now that each
emitted
> buffer has a proper track id assigned to it we can go even further and flatten
> the BufferQueueMap into a single BufferQueue (and that's essentially what
> FrameProcessor does). But there is a lot of code (mostly in unit tests, but
also
> in MediaSourceState::OnNewBuffers) that relied on being able to access only
> audio or only video buffers emitted by the parser. So for now I've decided to
> keep them separate until they are passed into FrameProcessor which will
flatten
> that map into a single stream of buffers for further processing (essentially
the
> same as we do currently, only with a slightly different input structure - a
> unified map rather than separate BufferQueues + the map for text tracks). We
can
> certainly look into simplifying it further in the future, but I think this CL
> represents a nice intermediate step that will unblock, for example, adding
> support for multiple track in stream parsers (e.g. here is the next CL that I
> was planning to land https://codereview.chromium.org/2254733006/, that will
> allow .mp4 parser to handle multiple tracks). So the short answer: it was
simply
> easier and quicker to do it this way given the current code structure.

That makes sense. I'll do deep review of this Monday. Thanks for providing
reasoning around this :)

[servolk, 2016-08-22 17:08:45]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 17:09:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-08-22 17:47:16]

On 2016/08/19 22:38:40, wolenetz wrote:
> On 2016/08/19 22:20:33, servolk wrote:
> > On 2016/08/19 20:06:50, wolenetz wrote:
> > > I haven't reviewed this full CL, rather from the description alone I'm
> > wondering
> > > the utility of this versus actual usage of the newly emitted buffers.
> > > 
> > > Please first consult
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=338484 and the usage
> of
> > > StreamParser::MergeBufferQueues() as a current shim.
> > > 
> > > tl;dr: The coded frame processing algorithm processes new frames *from all
> > > tracks* in DTS sequence. Though the parsers emit such frames in distinct,
> per
> > > type, queues (and a map of text queues), the frame processor immediately
> > merges
> > > these in DTS sequence using the utility method.
> > > 
> > > Versus making the parsers do the merging themselves, what is the utility
of
> > > further breaking out the buffers by track in the longer run?
> > 
> > Yeah, that's a good point and I was pondering this myself. Now that each
> emitted
> > buffer has a proper track id assigned to it we can go even further and
flatten
> > the BufferQueueMap into a single BufferQueue (and that's essentially what
> > FrameProcessor does). But there is a lot of code (mostly in unit tests, but
> also
> > in MediaSourceState::OnNewBuffers) that relied on being able to access only
> > audio or only video buffers emitted by the parser. So for now I've decided
to
> > keep them separate until they are passed into FrameProcessor which will
> flatten
> > that map into a single stream of buffers for further processing (essentially
> the
> > same as we do currently, only with a slightly different input structure - a
> > unified map rather than separate BufferQueues + the map for text tracks). We
> can
> > certainly look into simplifying it further in the future, but I think this
CL
> > represents a nice intermediate step that will unblock, for example, adding
> > support for multiple track in stream parsers (e.g. here is the next CL that
I
> > was planning to land https://codereview.chromium.org/2254733006/, that will
> > allow .mp4 parser to handle multiple tracks). So the short answer: it was
> simply
> > easier and quicker to do it this way given the current code structure.
> 
> That makes sense. I'll do deep review of this Monday. Thanks for providing
> reasoning around this :)

Btw, one more thing that I have recently realized. When we are going to support
multiple media tracks in the future, FrameProcessor will probably have to use
demuxer-assigned track ids, instead of bytestream track ids, since bytestream
track ids are not guaranteed to be unique. One way to do that would be to change
the keys in this map to be media track ids instead of the bytestream ids.
Alternatively we could tell stream parsers how bytestream ids are mapped to
media track ids so that they could assign media track ids instead of bytestream
ids to the emitted frames, but that would probably be a bit more work and would
make stream parser implementations more complicated.

[commit-bot: I haz the power, 2016-08-22 19:23:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 19:23:24]

Dry run: This issue passed the CQ dry run.

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

Looking pretty good.

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser.cc:120: // Append audio buffer queues first, before all
other types, since
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?

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:176: for (const auto& it :
buffer_queue_map_) {
why are we copying bqm into bqm_ here? Why not just use bqm_ in call to
MergeBufferQueues?

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:208: // Verifies that MergeBufferQueues()
of the current |audio_buffers_|,
nit: update comment. these members no longer exist.

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:211: StreamParser::BufferQueueMap
buffer_queue_map;
ditto: why copy bqm_ into bqm?

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:222: void
ClearQueuesAndTextMapButKeepAnyMergedBuffers() {
nit: rename seems appropriate (ClearBufferQueuesButKeep.... ? )

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:227: std::map<StreamParser::TrackId,
StreamParser::BufferQueue> buffer_queue_map_;
nit: StreamParser::BufferQueueMap ?

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:155: const auto& audio_buffers =
Seems wrong to me to take the reference of retval of a method. audio_buffers
thus becomes a reference to a temporary that is no longer alive, right? (Or is
this some C++11-ism I'm missing?)
There are multiple occurrences of const auto& locals added in this CL that hold
retvals. Please tell me I'm wrong :) or fix.

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:337: const auto& audio_buffers =
ditto here and elsewhere const auto& = retval of some generator method smells
quite funny.

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
media/filters/media_source_state.cc:807: EndTimestamp(bufq) < min_end_timestamp)
{
aside: text track EndTimestamp minimums are now included, though currently I
don't think any auto_update_timestamp_offset-enabled parser emits text.

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser.cc:154: if (!it.second.empty()) {
per ::GetTextBuffers(), it.second is guaranteed to be !empty(). change to DCHECK
and unconditionally do the assertion here.

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser_unittest.cc:514: 
The point of this unit test is to verify the behavior of incremental parses,
which involve incremental GetBuffers(). Please restore that to this test.

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

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser.cc:120: // Append audio buffer queues first, before all
other types, since
On 2016/08/23 21:31:21, wolenetz wrote:
> 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?

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).

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:176: for (const auto& it :
buffer_queue_map_) {
On 2016/08/23 21:31:21, wolenetz wrote:
> why are we copying bqm into bqm_ here? Why not just use bqm_ in call to
> MergeBufferQueues?

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.

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:208: // Verifies that MergeBufferQueues()
of the current |audio_buffers_|,
On 2016/08/23 21:31:21, wolenetz wrote:
> nit: update comment. these members no longer exist.

Done.

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:211: StreamParser::BufferQueueMap
buffer_queue_map;
On 2016/08/23 21:31:21, wolenetz wrote:
> ditto: why copy bqm_ into bqm?

see above.

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:222: void
ClearQueuesAndTextMapButKeepAnyMergedBuffers() {
On 2016/08/23 21:31:21, wolenetz wrote:
> nit: rename seems appropriate (ClearBufferQueuesButKeep.... ? )

Done.

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser_unittest.cc:227: std::map<StreamParser::TrackId,
StreamParser::BufferQueue> buffer_queue_map_;
On 2016/08/23 21:31:21, wolenetz wrote:
> nit: StreamParser::BufferQueueMap ?

Done.

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:155: const auto& audio_buffers =
On 2016/08/23 21:31:21, wolenetz wrote:
> Seems wrong to me to take the reference of retval of a method. audio_buffers
> thus becomes a reference to a temporary that is no longer alive, right? (Or is
> this some C++11-ism I'm missing?)
> There are multiple occurrences of const auto& locals added in this CL that
hold
> retvals. Please tell me I'm wrong :) or fix.

Yes, it's actually pre-C++11 feature.
See
https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-....
AFAIK base::Bind also relies on this feature, and it's been robust so far :)

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:337: const auto& audio_buffers =
On 2016/08/23 21:31:21, wolenetz wrote:
> ditto here and elsewhere const auto& = retval of some generator method smells
> quite funny.

same as above

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
media/filters/media_source_state.cc:807: EndTimestamp(bufq) < min_end_timestamp)
{
On 2016/08/23 21:31:21, wolenetz wrote:
> aside: text track EndTimestamp minimums are now included, though currently I
> don't think any auto_update_timestamp_offset-enabled parser emits text.

Acknowledged. Yes, that's a good point. But would presense of text tracks
actually break this? I was hoping to unify audio, video and text processing as
much as possible (probably not any time soon, but eventually).

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser.cc:154: if (!it.second.empty()) {
On 2016/08/23 21:31:21, wolenetz wrote:
> per ::GetTextBuffers(), it.second is guaranteed to be !empty(). change to
DCHECK
> and unconditionally do the assertion here.

Done.

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser_unittest.cc:514: 
On 2016/08/23 21:31:21, wolenetz wrote:
> The point of this unit test is to verify the behavior of incremental parses,
> which involve incremental GetBuffers(). Please restore that to this test.

Wait, perhaps I'm missing something. This test still does multiple calls to
parser_->Parse. As far as I can see AppendToEnd haven't done any additional
verification of those intermediate results, and simply appended the buffers that
were ready so far to audio/video_buffers. The final verification was done
outside the loop. So I thought I've preserved the intended behavior here, no?

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

looking good % swapping hardcoded frameprocessor trackIDs in attempt to simplify
mergebufferqueues, and % restoring incremental getbuffers in
webm_cluster_parser_unittests

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser.cc:120: // Append audio buffer queues first, before all
other types, since
On 2016/08/23 23:56:31, servolk wrote:
> On 2016/08/23 21:31:21, wolenetz wrote:
> > 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?
> 
> 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).

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.

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:155: const auto& audio_buffers =
On 2016/08/23 23:56:32, servolk wrote:
> On 2016/08/23 21:31:21, wolenetz wrote:
> > Seems wrong to me to take the reference of retval of a method. audio_buffers
> > thus becomes a reference to a temporary that is no longer alive, right? (Or
is
> > this some C++11-ism I'm missing?)
> > There are multiple occurrences of const auto& locals added in this CL that
> hold
> > retvals. Please tell me I'm wrong :) or fix.
> 
> Yes, it's actually pre-C++11 feature.
> See
>
https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-....
> AFAIK base::Bind also relies on this feature, and it's been robust so far :)

TIL :) Thanks. I don't think your usage in this CL violates this, but "Note this
only applies to stack-based references. It doesn’t work for references that are
members of objects."

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:337: const auto& audio_buffers =
On 2016/08/23 23:56:32, servolk wrote:
> On 2016/08/23 21:31:21, wolenetz wrote:
> > ditto here and elsewhere const auto& = retval of some generator method
smells
> > quite funny.
> 
> same as above

Acknowledged.

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
media/filters/media_source_state.cc:807: EndTimestamp(bufq) < min_end_timestamp)
{
On 2016/08/23 23:56:32, servolk wrote:
> On 2016/08/23 21:31:21, wolenetz wrote:
> > aside: text track EndTimestamp minimums are now included, though currently I
> > don't think any auto_update_timestamp_offset-enabled parser emits text.
> 
> Acknowledged. Yes, that's a good point. But would presense of text tracks
> actually break this? I was hoping to unify audio, video and text processing as
> much as possible (probably not any time soon, but eventually).

I'm not sure, though the right eventual fix will be to force sequence mode and
use that logic in frame processor rather than this custom logic for
auto-generate-timestamps-flag bytestreams. That fix is tracked by
https://bugs.chromium.org/p/chromium/issues/detail?id=607372.

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser_unittest.cc:514: 
On 2016/08/23 23:56:32, servolk wrote:
> On 2016/08/23 21:31:21, wolenetz wrote:
> > The point of this unit test is to verify the behavior of incremental parses,
> > which involve incremental GetBuffers(). Please restore that to this test.
> 
> Wait, perhaps I'm missing something. This test still does multiple calls to
> parser_->Parse. As far as I can see AppendToEnd haven't done any additional
> verification of those intermediate results, and simply appended the buffers
that
> were ready so far to audio/video_buffers. The final verification was done
> outside the loop. So I thought I've preserved the intended behavior here, no?

Hmm. I can see how this was confusing. WebMStreamParser executes incremental
parses, with getbuffers between each. This test was intended to mimic that
behavior. Especially on a muxed cluster, the "ready buffers" calculations are
important to not emit any frames with timestamp greater than any
parsed-but-with-duration-not-known-yet frames in the cluster. At cluster
boundary, all such frames are known. Incremental getBuffers tests the common
case where a full cluster might not be appended to the stream parser yet;
conforming to the reality where each Parse is interleaved with a GetBuffers.
Other tests in this file overlap this a little, but with specific cases around
where durations might be estimated or unknown until cluster end. Please restore
incremental getbuffers.

[servolk, 2016-08-24 23:17:45]

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser.cc:120: // Append audio buffer queues first, before all
other types, since
On 2016/08/24 21:57:51, wolenetz wrote:
> On 2016/08/23 23:56:31, servolk wrote:
> > On 2016/08/23 21:31:21, wolenetz wrote:
> > > 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?
> > 
> > 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).
> 
> 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.

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).

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:155: const auto& audio_buffers =
On 2016/08/24 21:57:51, wolenetz wrote:
> On 2016/08/23 23:56:32, servolk wrote:
> > On 2016/08/23 21:31:21, wolenetz wrote:
> > > Seems wrong to me to take the reference of retval of a method.
audio_buffers
> > > thus becomes a reference to a temporary that is no longer alive, right?
(Or
> is
> > > this some C++11-ism I'm missing?)
> > > There are multiple occurrences of const auto& locals added in this CL that
> > hold
> > > retvals. Please tell me I'm wrong :) or fix.
> > 
> > Yes, it's actually pre-C++11 feature.
> > See
> >
>
https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-....
> > AFAIK base::Bind also relies on this feature, and it's been robust so far :)
> 
> TIL :) Thanks. I don't think your usage in this CL violates this, but "Note
this
> only applies to stack-based references. It doesn’t work for references that
are
> members of objects."

Acknowledged.

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/filters/media_sou...
media/filters/media_source_state.cc:807: EndTimestamp(bufq) < min_end_timestamp)
{
On 2016/08/24 21:57:51, wolenetz wrote:
> On 2016/08/23 23:56:32, servolk wrote:
> > On 2016/08/23 21:31:21, wolenetz wrote:
> > > aside: text track EndTimestamp minimums are now included, though currently
I
> > > don't think any auto_update_timestamp_offset-enabled parser emits text.
> > 
> > Acknowledged. Yes, that's a good point. But would presense of text tracks
> > actually break this? I was hoping to unify audio, video and text processing
as
> > much as possible (probably not any time soon, but eventually).
> 
> I'm not sure, though the right eventual fix will be to force sequence mode and
> use that logic in frame processor rather than this custom logic for
> auto-generate-timestamps-flag bytestreams. That fix is tracked by
> https://bugs.chromium.org/p/chromium/issues/detail?id=607372.

Acknowledged.

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser_unittest.cc:514: 
On 2016/08/24 21:57:51, wolenetz wrote:
> On 2016/08/23 23:56:32, servolk wrote:
> > On 2016/08/23 21:31:21, wolenetz wrote:
> > > The point of this unit test is to verify the behavior of incremental
parses,
> > > which involve incremental GetBuffers(). Please restore that to this test.
> > 
> > Wait, perhaps I'm missing something. This test still does multiple calls to
> > parser_->Parse. As far as I can see AppendToEnd haven't done any additional
> > verification of those intermediate results, and simply appended the buffers
> that
> > were ready so far to audio/video_buffers. The final verification was done
> > outside the loop. So I thought I've preserved the intended behavior here,
no?
> 
> Hmm. I can see how this was confusing. WebMStreamParser executes incremental
> parses, with getbuffers between each. This test was intended to mimic that
> behavior. Especially on a muxed cluster, the "ready buffers" calculations are
> important to not emit any frames with timestamp greater than any
> parsed-but-with-duration-not-known-yet frames in the cluster. At cluster
> boundary, all such frames are known. Incremental getBuffers tests the common
> case where a full cluster might not be appended to the stream parser yet;
> conforming to the reality where each Parse is interleaved with a GetBuffers.
> Other tests in this file overlap this a little, but with specific cases around
> where durations might be estimated or unknown until cluster end. Please
restore
> incremental getbuffers.

Ah, ok, restored calling GetBuffers after each Parse.

[servolk, 2016-08-24 23:18:26]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 23:19:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

LGTM

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/2254093002/diff/60001/media/base/stream_parse...
media/base/stream_parser.cc:120: // Append audio buffer queues first, before all
other types, since
On 2016/08/24 23:17:44, servolk wrote:
> On 2016/08/24 21:57:51, wolenetz wrote:
> > On 2016/08/23 23:56:31, servolk wrote:
> > > On 2016/08/23 21:31:21, wolenetz wrote:
> > > > 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?
> > > 
> > > 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).
> > 
> > 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.
> 
> 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).

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

[servolk, 2016-08-24 23:53:14]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2016-08-24 23:53:19]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-08-24 23:54:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 02:40:55]

Description was changed from

==========
Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581
==========

to

==========
Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581
==========

[commit-bot: I haz the power, 2016-08-25 02:40:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-25 02:42:45]

Description was changed from

==========
Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581
==========

to

==========
Return buffers from StreamParsers in a single unified map

Currently various StreamParsers return output buffers via 3 separate
collections: a queue for audio, a queue for video and a map of queues
keyed by the track id for text tracks. Now that all stream parsers
properly assign track id to each buffer we can unify all those 3
collections into a single BufferQueueMap keyed by track ids.

BUG=341581

Committed: https://crrev.com/83f4c6d26c21cb52c5ec8f4e1d3b409a606e4377
Cr-Commit-Position: refs/heads/master@{#414274}
==========

[commit-bot: I haz the power, 2016-08-25 02:42:47]

Patchset 6 (id:??) landed as
https://crrev.com/83f4c6d26c21cb52c5ec8f4e1d3b409a606e4377
Cr-Commit-Position: refs/heads/master@{#414274}