MSE: Add StreamParser buffer remuxing utility and tests

MSE: Add StreamParser buffer remuxing utility and tests

Introduces remuxing utility in stream_parser.h for later refactoring
into fine-grained usage by each stream parser. Adds associated
unit tests.
Modifies StreamParserBuffer so buffers are annotated by type of media
(audio/video/text) and track ID on creation.
This is another prerequisite to implementing MSE-compliant coded frame
processing algorithm in SourceState that requires muxed-in-decode-
timestamp sequence buffers from all media types in the segment
instead of the current sequence per each media type.

R=xhwang@chromium.org
BUG=249422, 341581
TEST=media unittests and MSE layout tests pass locally on Linux.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249634

[wolenetz, 2014-01-28 22:06:19]

PTAL. FYI - I had to delete the previous WIP CL due to Rietveld/upload was hosed
(.gitmodules was in PS1 but not in later, and diff enumeration vs newer upload
attempts was also broken)

Versus that WIP CL, I've removed the actual usage (other than in
media_unittests) of the remuxing utility. The new fields in StreamParserBuffer
are populated by the various parsers, but not used yet in coded-frame-processing
in SourceState since that change depends on this.

[wolenetz, 2014-01-28 22:41:22]

As mentioned offline, PTAL @ patch set 2 instead. Thanks!

[xhwang, 2014-01-29 08:04:49]

This patch looks good. Here are my first round of comments.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser.cc
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.cc:16: bool StreamParser::MergeBufferQueues(const
BufferQueue& audio_buffers,
This function doesn't use any member variable. Does it make sense to declare
this as a standalone helper function instead of a class static function? (I am
not clear about how this function will be used so I am not sure.)

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.cc:40:
StreamParser::MergeBufferQueues(std::vector<const BufferQueue*> buffer_queues,
The implementation is complicated, we may need more comments about what we are
doing.

I haven't reviewed the detail of this function yet. Just OOC, this seems like
merge-sorting three already sorted sequences. Can we use std::merge to help us?

http://www.cplusplus.com/reference/algorithm/merge/

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.cc:41: BufferQueue* merged_buffers) {
format looks odd; put bool and function name in the same line?

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser.h
File media/base/stream_parser.h (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.h:125: BufferQueue* merged_buffers);
put static function before non-static functions.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.h:129: static bool MergeBufferQueues(std::vector<const
BufferQueue*> buffer_queues,
pass the vector by const-ref?

We usually don't like function overload; rename to MergeBufferQueuesInternal?

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.cc:23: kAudio /* meaningless */));
see above about the possibility of using UNKNOWN for EOS buffer.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
File media/base/stream_parser_buffer.h (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.h:18: enum Type {
Does it make sense to reuse DemuxerStream::Type? If we do that, we can use
UNKNOWN for EOS.

e.g. you can do typedef DemuxerStream::Type Type; But unfortunately we still
need to use StreamType::VIDEO et al.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.h:22: };
FYI, we are moving from kCamelCase to UPPER_CASE style for enums.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.h:67: int text_track_number_;
document range of |text_track_number|, e.g. -1 is invalid.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:15: bool
IsAudio(scoped_refptr<StreamParserBuffer> buffer) {
should these functions be static?

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:18: return false;
return buffer->type() == StreamParserBuffer::kAudio;

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:24: return false;
ditto

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:30: return false;
ditto

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:36: 
remove extra empty line

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:38: void
GenerateBuffers(StreamParser::BufferQueue* queue,
add doc about what this function do

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:41: int text_track_number) {
reorder parameters: we usually put input before output parameters.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:43: for (int i = 0; decode_timestamps[i] !=
kEnd; i++) {
nit: ++i

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:55: void
GenerateBuffers(StreamParser::BufferQueue* queue,
Usually we don't like function overloading. How about GenerateVideoBuffers and
GenerateAudioBuffers? This way we can save the |type| parameter and avoid the
long StreamParserBuffer::kAudio in the call site.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:69: std::string BufferQueueToString(const
StreamParser::BufferQueue* queue,
pass as const-ref?

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:69: std::string BufferQueueToString(const
StreamParser::BufferQueue* queue,
We use StreamParser::BufferQueue a lot in this test. Feel free to 

typedef StreamParser::BufferQueue BufferQueue;

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:69: std::string BufferQueueToString(const
StreamParser::BufferQueue* queue,
doc about the output string format

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:71: std::stringstream results_stream;
add #include <sstream> for this

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:77: scoped_refptr<StreamParserBuffer>
buffer = *itr;
nit: you can avoid increment/decrement the ref count by doing
scoped_refptr<StreamParserBuffer>& buffer = *itr;
or 
const StreamParserBuffer& buffer = *(*itr);

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:102: const std::string expected,
pass by const-ref

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:104: StreamParser::BufferQueue*
merged_buffers) {
Add a doc. It's not obvious that |merged_buffers| is both input and output.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:108:
static_cast<size_t>(count_if(merged_buffers->begin(),
s/count_if/std::count_if

add #include <algorithm>

Actually you can wrap
static_cast<size_t>(count_if(merged_buffers->begin(), merged_buffers->end(),
Func)); into a helper function.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:170: StreamParser::TextBufferQueueMap
text_map;
these are used in all tests, may be you can just declare them as class memebers.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:223: std::string expected = "T1:100 T1:101
T2:103 T1:104 T2:105 T2:106";
We are using ":" for text because it has text track numbers. But T1:100 is not
consistent with A100. How about: T100:1 A100 or T1:100 A:100 ?

https://codereview.chromium.org/149153002/diff/20001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:99: StreamParserBuffer::Type
expected_type = StreamParserBuffer::kAudio;
kAudio/UNKNOWN here, then set expected_type to kAudio after line 103?

https://codereview.chromium.org/149153002/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:182:
EXPECT_EQ(buffer->text_track_number(), text_track_num);
FYI, we use EXPECT_EQ(expected, variable) style, not EXPECT_EQ(variable,
expected);

[wolenetz, 2014-02-05 02:49:53]

Thank you for these helpful comments.
PTAL @ PS3.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser.cc
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.cc:16: bool StreamParser::MergeBufferQueues(const
BufferQueue& audio_buffers,
On 2014/01/29 08:04:50, xhwang wrote:
> This function doesn't use any member variable. Does it make sense to declare
> this as a standalone helper function instead of a class static function? (I am
> not clear about how this function will be used so I am not sure.)

Yeah, there's really no need other than possibly name-scoping to StreamParser
class. This helper will hopefully not outlive http://crbug.com/338484's fix.
I've removed it from the class.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.cc:40:
StreamParser::MergeBufferQueues(std::vector<const BufferQueue*> buffer_queues,
On 2014/01/29 08:04:50, xhwang wrote:
> The implementation is complicated, we may need more comments about what we are
> doing.
> 
> I haven't reviewed the detail of this function yet. Just OOC, this seems like
> merge-sorting three already sorted sequences. Can we use std::merge to help
us?
> 
> http://www.cplusplus.com/reference/algorithm/merge/

I've added more documentation now.

We'd need to hierarchically use std::merge for N, not 3, sorted sequences (since
there could be multiple text tracks, each with their own BufferQueue). Also,
std::merge behavior is undefined for non-sorted inputs. Granted, we could
presort, or pre-detect out-of-sort each queue for the latter, but I'm not
convinced the resulting code would be less complex or better performant than the
existing approach. I've also retained the non-critical allowance of pre-existing
content in |merged_buffers| and associated checks around new input buffers' DTS
vs the last buffer, if any, in |merged_buffers|.

This helper is really only a stop-gap to enable parallel work on
1) coded frame processing in SourceState::OnNewBuffers() from a single sorted
mixed-media-typed BufferQueue (another prereq for fixing
http://crbug.com/249422), and
2) refactoring this large-grain merge into fine-grained merges done directly by
the stream parsers (next step for fixing http://crbug.com/338484).

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.cc:41: BufferQueue* merged_buffers) {
On 2014/01/29 08:04:50, xhwang wrote:
> format looks odd; put bool and function name in the same line?

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser.h
File media/base/stream_parser.h (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.h:125: BufferQueue* merged_buffers);
On 2014/01/29 08:04:50, xhwang wrote:
> put static function before non-static functions.

Done (actually changed to non-member function per other comment).

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser.h:129: static bool MergeBufferQueues(std::vector<const
BufferQueue*> buffer_queues,
On 2014/01/29 08:04:50, xhwang wrote:
> pass the vector by const-ref?
> 
> We usually don't like function overload; rename to MergeBufferQueuesInternal?

Done and done. And made it a non-member .cc-scoped static function.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.cc:23: kAudio /* meaningless */));
On 2014/01/29 08:04:50, xhwang wrote:
> see above about the possibility of using UNKNOWN for EOS buffer.

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
File media/base/stream_parser_buffer.h (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.h:18: enum Type {
On 2014/01/29 08:04:50, xhwang wrote:
> Does it make sense to reuse DemuxerStream::Type? If we do that, we can use
> UNKNOWN for EOS.
> 
> e.g. you can do typedef DemuxerStream::Type Type; But unfortunately we still
> need to use StreamType::VIDEO et al.

tl;dr: Seems reasonable to me. I've made the changes.

pros of reusing DemuxerStream::Type:
* one less media type enum 
* well-defined UNKNOWN for edge cases like EOS or when constructing dummy
buffers for sorting purposes.
cons:
* Parsers must not emit UNKNOWN-annotated buffers. It seems this can be
DCHECK'ed+cause parse-failure in the upcoming coded-frame-processing
reimplementation of SourceState::OnNewBuffers().

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.h:22: };
On 2014/01/29 08:04:50, xhwang wrote:
> FYI, we are moving from kCamelCase to UPPER_CASE style for enums.

Cool. Thank you.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_buffer.h:67: int text_track_number_;
On 2014/01/29 08:04:50, xhwang wrote:
> document range of |text_track_number|, e.g. -1 is invalid.

For the current WebM Tracks Parser, -1 is indeed invalid. I'm not sure that will
apply to all MSE stream parsers that may eventually support text tracks. Perhaps
leaking too much of StreamParser, I've now commented this is the key for the
buffer's text track in the applicable TextTrackConfigMap (which is passed in
StreamParser::NewConfigCB).

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:15: bool
IsAudio(scoped_refptr<StreamParserBuffer> buffer) {
On 2014/01/29 08:04:50, xhwang wrote:
> should these functions be static?

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:18: return false;
On 2014/01/29 08:04:50, xhwang wrote:
> return buffer->type() == StreamParserBuffer::kAudio;

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:24: return false;
On 2014/01/29 08:04:50, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:30: return false;
On 2014/01/29 08:04:50, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:36: 
On 2014/01/29 08:04:50, xhwang wrote:
> remove extra empty line

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:38: void
GenerateBuffers(StreamParser::BufferQueue* queue,
On 2014/01/29 08:04:50, xhwang wrote:
> add doc about what this function do

Done, and also made it a static nonmember.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:41: int text_track_number) {
On 2014/01/29 08:04:50, xhwang wrote:
> reorder parameters: we usually put input before output parameters.

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:43: for (int i = 0; decode_timestamps[i] !=
kEnd; i++) {
On 2014/01/29 08:04:50, xhwang wrote:
> nit: ++i

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:55: void
GenerateBuffers(StreamParser::BufferQueue* queue,
On 2014/01/29 08:04:50, xhwang wrote:
> Usually we don't like function overloading. How about GenerateVideoBuffers and
> GenerateAudioBuffers? This way we can save the |type| parameter and avoid the
> long StreamParserBuffer::kAudio in the call site.

Done, also saving the |queue| parameter by using well-defined member queue for
each.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:69: std::string BufferQueueToString(const
StreamParser::BufferQueue* queue,
On 2014/01/29 08:04:50, xhwang wrote:
> pass as const-ref?

Changed to just use well-defined member |merged_buffers_| always.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:69: std::string BufferQueueToString(const
StreamParser::BufferQueue* queue,
On 2014/01/29 08:04:50, xhwang wrote:
> We use StreamParser::BufferQueue a lot in this test. Feel free to 
> 
> typedef StreamParser::BufferQueue BufferQueue;

Done and ditto for TextBufferQueueMap.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:69: std::string BufferQueueToString(const
StreamParser::BufferQueue* queue,
On 2014/01/29 08:04:50, xhwang wrote:
> doc about the output string format

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:71: std::stringstream results_stream;
On 2014/01/29 08:04:50, xhwang wrote:
> add #include <sstream> for this

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:77: scoped_refptr<StreamParserBuffer>
buffer = *itr;
On 2014/01/29 08:04:50, xhwang wrote:
> nit: you can avoid increment/decrement the ref count by doing
> scoped_refptr<StreamParserBuffer>& buffer = *itr;
> or 
> const StreamParserBuffer& buffer = *(*itr);

Nice :) Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:102: const std::string expected,
On 2014/01/29 08:04:50, xhwang wrote:
> pass by const-ref

Changed to just use well-defined members |audio_buffers_|, |video_buffers_|,
|text_map_| always.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:104: StreamParser::BufferQueue*
merged_buffers) {
On 2014/01/29 08:04:50, xhwang wrote:
> Add a doc. It's not obvious that |merged_buffers| is both input and output.

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:108:
static_cast<size_t>(count_if(merged_buffers->begin(),
On 2014/01/29 08:04:50, xhwang wrote:
> s/count_if/std::count_if
> 
> add #include <algorithm>
> 
> Actually you can wrap
> static_cast<size_t>(count_if(merged_buffers->begin(), merged_buffers->end(),
> Func)); into a helper function.

Done.

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:170: StreamParser::TextBufferQueueMap
text_map;
On 2014/01/29 08:04:50, xhwang wrote:
> these are used in all tests, may be you can just declare them as class
memebers.

Done. Saves spending a lot of lines: thank you!

https://codereview.chromium.org/149153002/diff/20001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:223: std::string expected = "T1:100 T1:101
T2:103 T1:104 T2:105 T2:106";
On 2014/01/29 08:04:50, xhwang wrote:
> We are using ":" for text because it has text track numbers. But T1:100 is not
> consistent with A100. How about: T100:1 A100 or T1:100 A:100 ?

Done (T1:100 A:100).

https://codereview.chromium.org/149153002/diff/20001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:99: StreamParserBuffer::Type
expected_type = StreamParserBuffer::kAudio;
On 2014/01/29 08:04:50, xhwang wrote:
> kAudio/UNKNOWN here, then set expected_type to kAudio after line 103?

Done.

https://codereview.chromium.org/149153002/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:182:
EXPECT_EQ(buffer->text_track_number(), text_track_num);
On 2014/01/29 08:04:50, xhwang wrote:
> FYI, we use EXPECT_EQ(expected, variable) style, not EXPECT_EQ(variable,
> expected);

Done, also conformed a bunch of other places in this file to this.

[xhwang, 2014-02-06 00:36:31]

Thanks for the patience!

lgtm % nits

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser.cc
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser.cc:36:
std::vector<StreamParser::BufferQueue::const_iterator> itrs;
Use vector::vector(size_type n) to avoid expanding the vector in the following
for loop.

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser.cc:67: if (itrs[i] != buffer_queues[i]->end()) {
if (itrs[i] == buffer_queues[i]->end())
  continue;

...

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser.cc:83: }
This block is inefficient. Ideally, we should use a heap to store the head of
all queues, so that we can pop the head with lowest decode timestamp in log(N)
time. Since we don't have a lot of queues and this code is going away, I think
this implementation is okay. But can you add a comment just in case people
wonder?

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:227: BufferQueue audio_buffers_,
video_buffers_, text_buffers_a_, text_buffers_b_;
I don't find a rule against this, but I rarely see this style. Usually we
declare each variable on a separate line. This style tends to be tricky for
pointers (which isn't the case here though), e.g. int* a, b;

https://codereview.chromium.org/149153002/diff/70001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/70001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:247: EXPECT_EQ(result,
cluster->size());
why reverse this order?

[wolenetz, 2014-02-06 23:56:02]

Thanks for CR'ing. Based on some offline comments from acolwell@, I have added a
typedef for StreamParser::TrackId (still just int for now) and modified
StreamParserBuffer constructors/CopyFrom() to require some TrackId, regardless
of buffer type(). This is to help prepare for fixing http://crbug.com/341581.

Please take another look due to the fairly scattered updates introduced in PS4.

Thanks!

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser.cc
File media/base/stream_parser.cc (right):

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser.cc:36:
std::vector<StreamParser::BufferQueue::const_iterator> itrs;
On 2014/02/06 00:36:32, xhwang wrote:
> Use vector::vector(size_type n) to avoid expanding the vector in the following
> for loop.

Done.

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser.cc:67: if (itrs[i] != buffer_queues[i]->end()) {
On 2014/02/06 00:36:32, xhwang wrote:
> if (itrs[i] == buffer_queues[i]->end())
>   continue;
> 
> ...

Done.

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser.cc:83: }
On 2014/02/06 00:36:32, xhwang wrote:
> This block is inefficient. Ideally, we should use a heap to store the head of
> all queues, so that we can pop the head with lowest decode timestamp in log(N)
> time. Since we don't have a lot of queues and this code is going away, I think
> this implementation is okay. But can you add a comment just in case people
> wonder?

Done.

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
File media/base/stream_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/70001/media/base/stream_parser...
media/base/stream_parser_unittest.cc:227: BufferQueue audio_buffers_,
video_buffers_, text_buffers_a_, text_buffers_b_;
On 2014/02/06 00:36:32, xhwang wrote:
> I don't find a rule against this, but I rarely see this style. Usually we
> declare each variable on a separate line. This style tends to be tricky for
> pointers (which isn't the case here though), e.g. int* a, b;

Done.

https://codereview.chromium.org/149153002/diff/70001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/70001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:247: EXPECT_EQ(result,
cluster->size());
On 2014/02/06 00:36:32, xhwang wrote:
> why reverse this order?

My mistake. Fixed.

[xhwang, 2014-02-07 00:09:56]

lgtm++ with nits

https://codereview.chromium.org/149153002/diff/460001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/460001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:306: // Buffer type and track ID
is meaningless to these tests.
s/is/are

https://codereview.chromium.org/149153002/diff/460001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:396: // Create buffer. Buffer
type and track ID is meaningless to these tests.
ditto

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

https://codereview.chromium.org/149153002/diff/460001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser.cc:21: int64 timecode_scale, int
audio_track_num, int video_track_num,
Shall we s/track_num/track_id/ in this file?

[wolenetz, 2014-02-07 00:29:05]

The CQ bit was checked by wolenetz@chromium.org

[wolenetz, 2014-02-07 00:29:18]

Thanks! Sending to CQ.

https://codereview.chromium.org/149153002/diff/460001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/149153002/diff/460001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:306: // Buffer type and track ID
is meaningless to these tests.
On 2014/02/07 00:09:56, xhwang wrote:
> s/is/are

Done.

https://codereview.chromium.org/149153002/diff/460001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:396: // Create buffer. Buffer
type and track ID is meaningless to these tests.
On 2014/02/07 00:09:56, xhwang wrote:
> ditto

Done.

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

https://codereview.chromium.org/149153002/diff/460001/media/formats/webm/webm...
media/formats/webm/webm_cluster_parser.cc:21: int64 timecode_scale, int
audio_track_num, int video_track_num,
On 2014/02/07 00:09:56, xhwang wrote:
> Shall we s/track_num/track_id/ in this file?

Longer term, a track configuration or buffer's track_id() will not necessarily
be the bytestream's track number. I would rather not reinforce confusion :) See
http://crbug.com/341581.

I've now added a comment for WebM's CopyFrom()'s referencing that bug like I've
done in the other MSE stream parsers.

Also, I've filed http://crbug.com/341601 to track smelly WebM parser track
number datatype/range discrepancies. Those again are bytestream, not necessarily
track config or buffer track_id() in long run.

[commit-bot: I haz the power, 2014-02-07 00:30:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/149153002/420002

[commit-bot: I haz the power, 2014-02-07 02:21:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/149153002/420002

[commit-bot: I haz the power, 2014-02-07 08:43:06]

Change committed as 249634