Add support for incremental cluster parsing.

Adding support for incremental cluster parsing.


BUG=104160
TEST=Covered by ChunkDemuxer unittests.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114163

[scherkus (not reviewing), 2011-12-03 01:56:50]

LGTM w/ some nits

I'm not 100% clear on what's going on -- would updated docs and/or a test
exercising incremental parsing help?

http://codereview.chromium.org/8775035/diff/2004/media/webm/webm_cluster_pars...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/8775035/diff/2004/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:35: int WebMClusterParser::Parse(const uint8*
buf, int size) {
perhaps the .h docs need to be updated because I'm not quite clear on what this
function is supposed to be doing

http://codereview.chromium.org/8775035/diff/2004/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:46: if (id == kWebMIdCues) {
sanity check: this branch checks for a CUES section then skips it?

http://codereview.chromium.org/8775035/diff/2004/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:53: } else if (id != kWebMIdCluster) {
nit: no need for else

http://codereview.chromium.org/8775035/diff/2004/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:54: VLOG(1) << "Unexpected ID 0x" << std::hex
<< id;
DVLOG

[acolwell GONE FROM CHROMIUM, 2011-12-08 22:25:05]

PTAL.. 
- I've reimplemented the list element parsing so it can be done incrementally.
- Updated the existing parsers to use WebMListParser.
- Added parsing unit tests!

[scherkus (not reviewing), 2011-12-09 18:18:03]

nice!

Q: not seeing any modified unittest files.. did you forget to add them to the
CL?

http://codereview.chromium.org/8775035/diff/10002/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/8775035/diff/10002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:752: return 0;
let's say the next time we iterate I only pass in one byte of data ... does this
branch get executed again? do I have to pass in the previous iteration of data?

I think my confusion comes from when we're parsing via peeking at data (if we do
that at all) versus actually parsing by consuming data and who is responsible
for buffering what

In in-person explanation might help me sort this out in my head :)

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc
File media/webm/webm_parser.cc (right):

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:57: {SKIP, kWebMIdSeekHead}, // TODO(acolwell): add
SeekHead info
two spaces before // comments

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:130: sizeof(kListElementInfo) /
sizeof(ListElementInfo);
nit: I think we have an arraysize() macro or something of that sort that should
do this calculation in a safe portable manner

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:444: } break;
nit: this style is somewhat strange .. mind moving the break inside the } here +
below?

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.h
File media/webm/webm_parser.h (right):

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.h#ne...
media/webm/webm_parser.h:137: };
DISALLOW etc

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_tracks_pars...
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_tracks_pars...
media/webm/webm_tracks_parser.cc:13: base::TimeDelta::FromMicroseconds(-1);
I recently defined kInfiniteDuration if that helps

[acolwell GONE FROM CHROMIUM, 2011-12-09 19:06:09]

http://codereview.chromium.org/8775035/diff/10002/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/8775035/diff/10002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:752: return 0;
On 2011/12/09 18:18:04, scherkus wrote:
> let's say the next time we iterate I only pass in one byte of data ... does
this
> branch get executed again? 
Yes. Returning 0 indicates that the parser hasn't consumed anything and needs
more data before it can move forward.

> do I have to pass in the previous iteration of data?
Yes. This is handled by the caller of ParseCluster_Locked().

> 
> I think my confusion comes from when we're parsing via peeking at data (if we
do
> that at all) versus actually parsing by consuming data and who is responsible
> for buffering what
The model being used here is that the parser just tries to parse as much of the
buffer as it can and reports how much of the data was parsed. If the parser
discovers that it needs more data to parse an element it either returns the
number of bytes it used so far for fully parsed elements or returns 0 if no
complete elements have been parsed yet. If result > 0 the caller will adjust
data & size by this value and call this method again. Since we still don't have
enough data to parse the element 0 will get returned. Then the caller knows that
more data is needed before parsing can continue.

> 
> In in-person explanation might help me sort this out in my head :)
Hopefully the description above helps. If not I'll explain it in person.

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc
File media/webm/webm_parser.cc (right):

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:57: {SKIP, kWebMIdSeekHead}, // TODO(acolwell): add
SeekHead info
On 2011/12/09 18:18:04, scherkus wrote:
> two spaces before // comments

Done.

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:130: sizeof(kListElementInfo) /
sizeof(ListElementInfo);
On 2011/12/09 18:18:04, scherkus wrote:
> nit: I think we have an arraysize() macro or something of that sort that
should
> do this calculation in a safe portable manner

Done.

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:444: } break;
On 2011/12/09 18:18:04, scherkus wrote:
> nit: this style is somewhat strange .. mind moving the break inside the } here
+
> below?

Done.

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.h
File media/webm/webm_parser.h (right):

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_parser.h#ne...
media/webm/webm_parser.h:137: };
On 2011/12/09 18:18:04, scherkus wrote:
> DISALLOW etc

Done.

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_tracks_pars...
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/8775035/diff/10002/media/webm/webm_tracks_pars...
media/webm/webm_tracks_parser.cc:13: base::TimeDelta::FromMicroseconds(-1);
On 2011/12/09 18:18:04, scherkus wrote:
> I recently defined kInfiniteDuration if that helps
I think that might make things confusing. This isn't an infinite duration. It is
supposed to convey that the duration isn't set.

[scherkus (not reviewing), 2011-12-09 21:36:03]

fit nits/suggestions but LGTM

nice tests!

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
File media/webm/webm_parser_unittest.cc (right):

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:39: static void
AddSimpleBlock(ClusterBuilder* cb, int track_num,
nit: move all these static methods to outside the class

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:70: ).WillOnce(Return(true));
nit: move ) to previous line and have this .WillOnce() indented by 4

OR

you can drop OnSimpleBlock(...) to next line w/ 4 space indent

whatever looks better

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:83:
scoped_ptr<StrictMock<MockWebMParserClient> > client(
nit: does this need to be a pointer? Looks like stack allocation would work here

also you could move this into the WebMParserTest

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:123: 0xE7, 0x81, 0x01, // Timecode (size=1,
value=1)
two space

[acolwell GONE FROM CHROMIUM, 2011-12-09 22:53:53]

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
File media/webm/webm_parser_unittest.cc (right):

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:39: static void
AddSimpleBlock(ClusterBuilder* cb, int track_num,
On 2011/12/09 21:36:03, scherkus wrote:
> nit: move all these static methods to outside the class

Done.

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:70: ).WillOnce(Return(true));
On 2011/12/09 21:36:03, scherkus wrote:
> nit: move ) to previous line and have this .WillOnce() indented by 4
> 
> OR
> 
> you can drop OnSimpleBlock(...) to next line w/ 4 space indent
> 
> whatever looks better

Done. Not sure why I did that in the first place.

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:83:
scoped_ptr<StrictMock<MockWebMParserClient> > client(
On 2011/12/09 21:36:03, scherkus wrote:
> nit: does this need to be a pointer? Looks like stack allocation would work
here
> 
> also you could move this into the WebMParserTest

Done. Removed pointer and placed it in WebMParserTest.

http://codereview.chromium.org/8775035/diff/14002/media/webm/webm_parser_unit...
media/webm/webm_parser_unittest.cc:123: 0xE7, 0x81, 0x01, // Timecode (size=1,
value=1)
On 2011/12/09 21:36:03, scherkus wrote:
> two space

Done.

[commit-bot: I haz the power, 2011-12-12 16:06:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/8775035/11039

[commit-bot: I haz the power, 2011-12-12 17:43:05]

Change committed as 114030

[commit-bot: I haz the power, 2011-12-13 01:39:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/8775035/20002

[commit-bot: I haz the power, 2011-12-13 02:56:36]

Change committed as 114163