Adding ChunkDemuxer implementation.

Adding ChunkDemuxer implementation.

BUG=86536
TEST=ChunkDemuxerTest.*

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

[acolwell GONE FROM CHROMIUM, 2011-06-21 20:25:14]

Chromium changes for Media Chunk API.

[scherkus (not reviewing), 2011-06-22 17:31:09]

combination of nits + some high level stuff we should consider, mostly:
  1) Breaking classes out into their own separate files
  2) Possibly creating media/webm folder if we want to expand that code
  3) Providing some helper docs / links to webm/ebml specs

also... nary a test is in sight!

I'm not sure about having unit-level testing for some of these things, but a
medium/integration-style test of say... running a WebM parse or appending some
data or something like that sounds like a good thing to have

http://codereview.chromium.org/7203002/diff/8001/media/base/buffers.h
File media/base/buffers.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/base/buffers.h#newcode96
media/base/buffers.h:96: class BufferImpl : public Buffer {
so DataBuffer didn't do the trick?

also: this has to be moved to different file we to keep it

http://codereview.chromium.org/7203002/diff/8001/media/base/buffers.h#newcode108
media/base/buffers.h:108: uint8* data_;
scoped_ptr ?

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:210: if (!format_context)
we expect to receive NULL?

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:1: 
?

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

http://codereview.chromium.org/7203002/diff/8001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:581: uint8* buf = new uint8[buf_size];
scoped_ptr seems nice here considering  this isn't a trivial method (seeing the
random "delete buf" all alone down there is unsettling)

http://codereview.chromium.org/7203002/diff/8001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.h:35: // WebMediaPlayerImpl to talk to the Demuxer.
media doesn't known about WebMediaPlayerImpl

this comment should describe the general usage w/o explicitly stating its direct
dependency

http://codereview.chromium.org/7203002/diff/8001/media/filters/ffmpeg_glue.h
File media/filters/ffmpeg_glue.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/ffmpeg_glue.h#...
media/filters/ffmpeg_glue.h:72: class BufferUrlProtocol : public
FFmpegURLProtocol {
any reason why this class can't be a part of the chunk demuxer / moved to its
own file?

this file contains FFmpegGlue + its interface (FFmpegURLProtocol) but no
concrete impls

http://codereview.chromium.org/7203002/diff/8001/media/filters/ffmpeg_glue.h#...
media/filters/ffmpeg_glue.h:89: };
DISALLOW etc.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc
File media/filters/webm_parser.cc (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:6: 
I believe you're doing ebml parsing here, so how about a link to a wikipedia
page or something so people don't need to reverse engineer what you're up here

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:13: static const int kMaxLevelDepth = 6;
docs?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:15: WebMParserClient::~WebMParserClient() {}
you can actually inline this in the .h if you want (the interface class doesn't
have any members, etc... so inlining the dtor is a no-op)

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:17: int webm_parse_num(const uint8* buf, int size,
int max_bytes,
docs?

I don't think we need to document the bit-twiddling going on below, but a solid
one-sentence should be enough

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:49: int* id, int64* element_size) {
docs

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:64: *id = (int)tmp;
static_cast

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:66: int num_sz_bytes = webm_parse_num(buf +
num_id_bytes,
sz?

I'm guessing it's size but I see a lot of other variables with "size" so let's
go with that

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:161: 
nit: remove extra blank lines

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:182: int count = id_info_size /
sizeof(ElementIdInfo);
nit: sizeof(*id_info)

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:192: int count = sizeof(kListElementInfo) /
sizeof(ListElementInfo);
isn't this a contsant?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:241: const uint8* pCur = buf;
pCur -> cur ?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:287: if (info->type_ == SBLOCK) {
use switch/case + helper functions to parse each type?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:322: for (int i = 0; i < element_size; ++i)
endianess?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:328: 
nit: remove blank line

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:343: tmp2.src = (int32)tmp;
does ebml specify anything w.r.t. endianess?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:349: } tmp2;
ditto

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:362: std::string s((const char*)pCur,
element_size);
reinterpret_cast<>

is the string null terminated or will the std::string ctor copy + do the proper
thing?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h
File media/filters/webm_parser.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:4: #ifndef MEDIA_FILTERS_WEBM_PARSER_H_
nit: add space before

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:11: namespace media {
I don't see any reason why this needs to be inside media/filters as opposed to
media/base

if we're really interested it might be worth having media/webm folder

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:13: const int kWebMIdAspectRatioType = 0x54B3;
typically initialize these in .cc

would it make sense to move these to a webm_constants.h file?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:65: // end of the list. Any pointers passed to these
methods are only guarenteed
guaranteed

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:67: // may become invalid. Each method returns a
bool that indicates whether the
nit: previous sentence says pointers are only guaranteed to be valid, but
following sentence says "may" become invalid!

when in doubt, trim down the comment to absolutes

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:86: int webm_parse_headers(WebMParserClient* client,
this looks like a non-trivial method -- Perhaps ParseWebMHeaders

ParseWebMCluster

?

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:90: int webm_parse_cluster(WebMParserClient* client,
docs for these methods?

I notice they return -1 -- what does that mean?

I also noticed a ton of -1s... why not true/false?

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:394:
media_data_sink_.reset(chunk_demuxer_factory_->CreateMediaDataSink());
what if chunk_demuxer_factory_ passed a ref to a media data sink as opposed to
creating a ptr?

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:590: //LOG(ERROR) << "duration() : " << ret;
?

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:599: ret =
static_cast<float>(paused_time_.InSecondsF());
revert the changes here?

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:603: //LOG(ERROR) << "currentTime() : " <<
ret;
?

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:633: //           << " - " <<
buffered_[0].end;
?

[acolwell GONE FROM CHROMIUM, 2011-06-23 16:51:28]

Addressed CR comments.

http://codereview.chromium.org/7203002/diff/8001/media/base/buffers.h
File media/base/buffers.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/base/buffers.h#newcode96
media/base/buffers.h:96: class BufferImpl : public Buffer {
oops. totally missed that class. I've updated the code to use DataBuffer
instead.
On 2011/06/22 17:31:09, scherkus wrote:
> so DataBuffer didn't do the trick?
> 
> also: this has to be moved to different file we to keep it

http://codereview.chromium.org/7203002/diff/8001/media/base/buffers.h#newcode108
media/base/buffers.h:108: uint8* data_;
On 2011/06/22 17:31:09, scherkus wrote:
> scoped_ptr ?
No longer needed.

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:210: if (!format_context)
On 2011/06/22 17:31:09, scherkus wrote:
> we expect to receive NULL?
No. I just copied a little too much from FFmpegDemuxer. This has been removed.

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:1: 
On 2011/06/22 17:31:09, scherkus wrote:
> ?

Done.

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

http://codereview.chromium.org/7203002/diff/8001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:581: uint8* buf = new uint8[buf_size];
On 2011/06/22 17:31:09, scherkus wrote:
> scoped_ptr seems nice here considering  this isn't a trivial method (seeing
the
> random "delete buf" all alone down there is unsettling)
Done. Changed this to scoped_array<uint8>

http://codereview.chromium.org/7203002/diff/8001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.h:35: // WebMediaPlayerImpl to talk to the Demuxer.
On 2011/06/22 17:31:09, scherkus wrote:
> media doesn't known about WebMediaPlayerImpl
> 
> this comment should describe the general usage w/o explicitly stating its
direct
> dependency

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/ffmpeg_glue.h
File media/filters/ffmpeg_glue.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/ffmpeg_glue.h#...
media/filters/ffmpeg_glue.h:72: class BufferUrlProtocol : public
FFmpegURLProtocol {
On 2011/06/22 17:31:09, scherkus wrote:
> any reason why this class can't be a part of the chunk demuxer / moved to its
> own file?
> 
> this file contains FFmpegGlue + its interface (FFmpegURLProtocol) but no
> concrete impls

Done. Moved code to media/filters/buffer_url_protocol.h &
media/filters/buffer_url_protocol.cc.

http://codereview.chromium.org/7203002/diff/8001/media/filters/ffmpeg_glue.h#...
media/filters/ffmpeg_glue.h:89: };
On 2011/06/22 17:31:09, scherkus wrote:
> DISALLOW etc.

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc
File media/filters/webm_parser.cc (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:6: 
On 2011/06/22 17:31:09, scherkus wrote:
> I believe you're doing ebml parsing here, so how about a link to a wikipedia
> page or something so people don't need to reverse engineer what you're up here

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:13: static const int kMaxLevelDepth = 6;
On 2011/06/22 17:31:09, scherkus wrote:
> docs?

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:15: WebMParserClient::~WebMParserClient() {}
On 2011/06/22 17:31:09, scherkus wrote:
> you can actually inline this in the .h if you want (the interface class
doesn't
> have any members, etc... so inlining the dtor is a no-op)
If I remember correctly there was this pass over all the code to remove inline
virtual functions. I was just making this code consistent with that.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:17: int webm_parse_num(const uint8* buf, int size,
int max_bytes,
On 2011/06/22 17:31:09, scherkus wrote:
> docs?
> 
> I don't think we need to document the bit-twiddling going on below, but a
solid
> one-sentence should be enough

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:49: int* id, int64* element_size) {
On 2011/06/22 17:31:09, scherkus wrote:
> docs

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:64: *id = (int)tmp;
On 2011/06/22 17:31:09, scherkus wrote:
> static_cast

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:66: int num_sz_bytes = webm_parse_num(buf +
num_id_bytes,
On 2011/06/22 17:31:09, scherkus wrote:
> sz?
> 
> I'm guessing it's size but I see a lot of other variables with "size" so let's
> go with that

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:161: 
On 2011/06/22 17:31:09, scherkus wrote:
> nit: remove extra blank lines

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:182: int count = id_info_size /
sizeof(ElementIdInfo);
On 2011/06/22 17:31:09, scherkus wrote:
> nit: sizeof(*id_info)

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:192: int count = sizeof(kListElementInfo) /
sizeof(ListElementInfo);
On 2011/06/22 17:31:09, scherkus wrote:
> isn't this a contsant?

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:241: const uint8* pCur = buf;
On 2011/06/22 17:31:09, scherkus wrote:
> pCur -> cur ?
Done. Old habits sneak back in sometimes. :)

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:287: if (info->type_ == SBLOCK) {
On 2011/06/22 17:31:09, scherkus wrote:
> use switch/case + helper functions to parse each type?

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:322: for (int i = 0; i < element_size; ++i)
On 2011/06/22 17:31:09, scherkus wrote:
> endianess?
Added a comment indicating the integer is stored as big endian in the buffer.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:328: 
On 2011/06/22 17:31:09, scherkus wrote:
> nit: remove blank line

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:343: tmp2.src = (int32)tmp;
On 2011/06/22 17:31:09, scherkus wrote:
> does ebml specify anything w.r.t. endianess?
Yes all fields are big endian. I've added a comment mentioning this.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:349: } tmp2;
On 2011/06/22 17:31:09, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.cc...
media/filters/webm_parser.cc:362: std::string s((const char*)pCur,
element_size);
On 2011/06/22 17:31:09, scherkus wrote:
> reinterpret_cast<>
> 
> is the string null terminated or will the std::string ctor copy + do the
proper
> thing?
The string is not null terminated. The constructor will do the right thing.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h
File media/filters/webm_parser.h (right):

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:11: namespace media {
On 2011/06/22 17:31:09, scherkus wrote:
> I don't see any reason why this needs to be inside media/filters as opposed to
> media/base
> 
> if we're really interested it might be worth having media/webm folder

Done. Moved to webm folder.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:13: const int kWebMIdAspectRatioType = 0x54B3;
On 2011/06/22 17:31:09, scherkus wrote:
> typically initialize these in .cc
> 
> would it make sense to move these to a webm_constants.h file?

Done. Moved to webm_constants.h

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:65: // end of the list. Any pointers passed to these
methods are only guarenteed
On 2011/06/22 17:31:09, scherkus wrote:
> guaranteed

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:67: // may become invalid. Each method returns a
bool that indicates whether the
On 2011/06/22 17:31:09, scherkus wrote:
> nit: previous sentence says pointers are only guaranteed to be valid, but
> following sentence says "may" become invalid!
> 
> when in doubt, trim down the comment to absolutes

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:86: int webm_parse_headers(WebMParserClient* client,
On 2011/06/22 17:31:09, scherkus wrote:
> this looks like a non-trivial method -- Perhaps ParseWebMHeaders
> 
> ParseWebMCluster
> 
> ?

Done.

http://codereview.chromium.org/7203002/diff/8001/media/filters/webm_parser.h#...
media/filters/webm_parser.h:90: int webm_parse_cluster(WebMParserClient* client,
On 2011/06/22 17:31:09, scherkus wrote:
> docs for these methods?
> 
> I notice they return -1 -- what does that mean?
> 
> I also noticed a ton of -1s... why not true/false?
I've added comments to explain the return value. These methods return the number
of bytes parsed on success and -1 on parse failure.

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:394:
media_data_sink_.reset(chunk_demuxer_factory_->CreateMediaDataSink());
On 2011/06/22 17:31:09, scherkus wrote:
> what if chunk_demuxer_factory_ passed a ref to a media data sink as opposed to
> creating a ptr?
No can do. WebMediaPlayerImpl needs to be able to manage the lifetime of
MediaDataSink. The factory becomes owned by the FilterCollection and can be
destroyed at any point. I believe the collection gets destroyed after the
pipeline is initialized.

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:590: //LOG(ERROR) << "duration() : " << ret;
On 2011/06/22 17:31:09, scherkus wrote:
> ?
reverted

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:599: ret =
static_cast<float>(paused_time_.InSecondsF());
On 2011/06/22 17:31:09, scherkus wrote:
> revert the changes here?

Done.

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:603: //LOG(ERROR) << "currentTime() : " <<
ret;
On 2011/06/22 17:31:09, scherkus wrote:
> ?
reverted

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:633: //           << " - " <<
buffered_[0].end;
On 2011/06/22 17:31:09, scherkus wrote:
> ?
reverted

http://codereview.chromium.org/7203002/diff/8001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:650: LOG(ERROR) << "maxTimeSeekable() : " <<
ret;
reverted

[scherkus (not reviewing), 2011-06-23 18:50:18]

http://codereview.chromium.org/7203002/diff/14001/media/base/data_buffer.h
File media/base/data_buffer.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/base/data_buffer.h#ne...
media/base/data_buffer.h:27: DataBuffer(uint8* buffer, size_t buffer_size, bool
copy);
I don't think we should have this ctor:
  a) boolean arguments aren't very clear
  b) passing false is equivalent to the first ctor
  c) there are only 2 callees

this is where Go is kind of awesome because instead of having ctors you have
static methods like "NewDataBufferWithCopiedData". anyway I think a helper
method inside your ChunkDemuxer would do the trick

note: the ctor below where you pass in a buffer size should die -- it's only
used to create EOS buffers by passing in 0, but that would be better suited with
a "EndOfStreamBuffer" subclass or something like that :)

http://codereview.chromium.org/7203002/diff/14001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.h:112: void DestroyAVFormatContext(AVFormatContext*
format_context);
docs!

for example, this will recurse and close all codecs

http://codereview.chromium.org/7203002/diff/14001/media/filters/buffer_url_pr...
File media/filters/buffer_url_protocol.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/filters/buffer_url_pr...
media/filters/buffer_url_protocol.h:19: class BufferUrlProtocol : public
FFmpegURLProtocol {
I had a feeling this code looked oddly familiar... sorry for not noticing sooner
btu take a look at InMemoryFileReader in media/filters/audio_file_reader.h

want to take a stab at merging the two?  I have no qualms over which one you
decide to use (although InMemoryDataReader should have been in its own file -- i
forgot to catch that one)

http://codereview.chromium.org/7203002/diff/14001/media/filters/buffer_url_pr...
media/filters/buffer_url_protocol.h:40: }
// namespace media

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

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:27: static const uint8 kWebMHeader[] = {
should this go in media/webm or do we plan to delete this once we figure out how
to get rid of AVFormatContext

if so.. do we have a TODO for getting rid of the workaround?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:105: class ClusterWebMParserClient : public
WebMParserClient {
could this also move to media/webm ?

this class could simply queue all of the buffers as member variables and then
after a successful parse have the client transfer the buffers to the
ChunkDemuxer buffer queue

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:112: : timecode_multiplier_(timecode_scale /
1000.0), // will convert timecodes
what about putting this comment in the .h file?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:120: << " atn " << audio_track_num_
indentation

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:183: //           << " tc " << (cluster_timecode_
+ timecode)
?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:219: int video_track_num_;
ditto for TimeDelta here

we should do the conversion during the parse instead of forcing all clients to
do the conversion

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:261: base::TimeDelta fts =
buffers_.front()->GetTimestamp();
holy hard to understand variables!!

why not "front_ts", "back_ts" and "delta_ts"

...also if this is only for logging purposes do we really need to kepe this code
around?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:333: 
nit: get rid of blank line

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:579: void ChunkDemuxer::ChangeState(State
new_state) {
nit: should this be ChangeState_Locked and/or have a lock_.AssertAcquired ?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:743: if (track_type_ == 1) {
can these be webm_constants.h somehow?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:759: timecode_scale_ = 1000000;
constant?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:847: AddRef();
I'm guessing we have to do this because BuildState lives its life on various
message loops?

:(

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:850: ~BuildState() { delete read_buffer_; }
virtual

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:853: if (message_loop_ != MessageLoop::current())
{
as I've been discussing w/ other folks, this pattern has to die

Sure it saves a few LOC but it's rather maddening to outside viewers (not to
mention makes debugging a bit painful).  Instead:
  1) BuildDone should always post a task to message_loop_
  2) You can name the other function BuildDoneOnDemuxerThread, or something to
your liking (DoBuildDone ?)

BuildDoneTask is something I helped propagate but is a really poor name for
tasks since it reads as a noun as opposed to a verb.

The reasoning for (1) is that if/when this code get refactored / moved to
different threads, we don't want to have completely different run-time
characteristics for the callee executing the callback depending on the executing
thread.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:882: read_buffer_ = new uint8[read_size_];
who deallocates this?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:888: if (message_loop_ != MessageLoop::current())
{
ditto

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:922: };
DISALLOW etc

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:54: };
DISALLOW etc

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:82: class InfoTrackWebMParserClient : public
WebMParserClient {
any reason why we can't break this out into media/webm?

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:91: int64 video_default_duration() const;
none of these durations mention the units

why not convert to base::TimeDelta during parsing then everything is
self-documenting?

http://codereview.chromium.org/7203002/diff/14001/media/webm/webm_constants.h
File media/webm/webm_constants.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/webm/webm_constants.h...
media/webm/webm_constants.h:10: //
http://www.matroska.org/technical/specs/index.html
nice :)

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

http://codereview.chromium.org/7203002/diff/14001/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:103: enum ElementType {
nit: constants 'n stuff get declared at the top

http://codereview.chromium.org/7203002/diff/14001/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:437: // Do nothing
period

http://codereview.chromium.org/7203002/diff/14001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7203002/diff/14001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:432: bool WebMediaPlayerImpl::addData(const
unsigned char* data, unsigned length) {
crazy naming nit we can decide on later:
appendData vs addData ?

[acolwell GONE FROM CHROMIUM, 2011-06-23 22:42:17]

Addressed CR comments

http://codereview.chromium.org/7203002/diff/14001/media/base/data_buffer.h
File media/base/data_buffer.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/base/data_buffer.h#ne...
media/base/data_buffer.h:27: DataBuffer(uint8* buffer, size_t buffer_size, bool
copy);
On 2011/06/23 18:50:18, scherkus wrote:
> I don't think we should have this ctor:
>   a) boolean arguments aren't very clear
>   b) passing false is equivalent to the first ctor
>   c) there are only 2 callees
> 
> this is where Go is kind of awesome because instead of having ctors you have
> static methods like "NewDataBufferWithCopiedData". anyway I think a helper
> method inside your ChunkDemuxer would do the trick
> 
> note: the ctor below where you pass in a buffer size should die -- it's only
> used to create EOS buffers by passing in 0, but that would be better suited
with
> a "EndOfStreamBuffer" subclass or something like that :)

Done.

http://codereview.chromium.org/7203002/diff/14001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.h:112: void DestroyAVFormatContext(AVFormatContext*
format_context);
On 2011/06/23 18:50:18, scherkus wrote:
> docs!
> 
> for example, this will recurse and close all codecs

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/buffer_url_pr...
File media/filters/buffer_url_protocol.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/filters/buffer_url_pr...
media/filters/buffer_url_protocol.h:19: class BufferUrlProtocol : public
FFmpegURLProtocol {
On 2011/06/23 18:50:18, scherkus wrote:
> I had a feeling this code looked oddly familiar... sorry for not noticing
sooner
> btu take a look at InMemoryFileReader in media/filters/audio_file_reader.h
> 
> want to take a stab at merging the two?  I have no qualms over which one you
> decide to use (although InMemoryDataReader should have been in its own file --
i
> forgot to catch that one)

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/buffer_url_pr...
media/filters/buffer_url_protocol.h:40: }
On 2011/06/23 18:50:18, scherkus wrote:
> // namespace media

Done.

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

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:27: static const uint8 kWebMHeader[] = {
On 2011/06/23 18:50:18, scherkus wrote:
> should this go in media/webm or do we plan to delete this once we figure out
how
> to get rid of AVFormatContext
> 
> if so.. do we have a TODO for getting rid of the workaround?
Added a TODO. I think it should stay here because it is specific to this Demuxer
implementation.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:105: class ClusterWebMParserClient : public
WebMParserClient {
On 2011/06/23 18:50:18, scherkus wrote:
> could this also move to media/webm ?
The implementation is specific to this demuxer. It only allows 2 streams &
errors on any elements other than simple blocks and cluster timecode. There are
other WebM elements that can appear in a cluster, but we don't need to support
them for this use case.
> 
> this class could simply queue all of the buffers as member variables and then
> after a successful parse have the client transfer the buffers to the
> ChunkDemuxer buffer queue
This class already does this.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:112: : timecode_multiplier_(timecode_scale /
1000.0), // will convert timecodes
On 2011/06/23 18:50:18, scherkus wrote:
> what about putting this comment in the .h file?
This class doesn't appear in the .h file. I've moved the comment down to the
timecode_multiplier_ declaration though.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:120: << " atn " << audio_track_num_
On 2011/06/23 18:50:18, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:183: //           << " tc " << (cluster_timecode_
+ timecode)
On 2011/06/23 18:50:18, scherkus wrote:
> ?
removed

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:219: int video_track_num_;
On 2011/06/23 18:50:18, scherkus wrote:
> ditto for TimeDelta here
> 
> we should do the conversion during the parse instead of forcing all clients to
> do the conversion

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:261: base::TimeDelta fts =
buffers_.front()->GetTimestamp();
On 2011/06/23 18:50:18, scherkus wrote:
> holy hard to understand variables!!
> 
> why not "front_ts", "back_ts" and "delta_ts"
> 
> ...also if this is only for logging purposes do we really need to kepe this
code
> around?
removed

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:333: 
On 2011/06/23 18:50:18, scherkus wrote:
> nit: get rid of blank line

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:579: void ChunkDemuxer::ChangeState(State
new_state) {
On 2011/06/23 18:50:18, scherkus wrote:
> nit: should this be ChangeState_Locked and/or have a lock_.AssertAcquired ?

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:743: if (track_type_ == 1) {
On 2011/06/23 18:50:18, scherkus wrote:
> can these be webm_constants.h somehow?

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:759: timecode_scale_ = 1000000;
On 2011/06/23 18:50:18, scherkus wrote:
> constant?

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:847: AddRef();
On 2011/06/23 18:50:18, scherkus wrote:
> I'm guessing we have to do this because BuildState lives its life on various
> message loops?
> 
> :(
This is needed because BuildState manages it's own lifetime. The factory doesn't
keep track of the object once it has been created. BuildState is responsible for
deleting itself once the build completes.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:850: ~BuildState() { delete read_buffer_; }
On 2011/06/23 18:50:18, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:853: if (message_loop_ != MessageLoop::current())
{
On 2011/06/23 18:50:18, scherkus wrote:
> as I've been discussing w/ other folks, this pattern has to die
> 
> Sure it saves a few LOC but it's rather maddening to outside viewers (not to
> mention makes debugging a bit painful).  Instead:
>   1) BuildDone should always post a task to message_loop_
>   2) You can name the other function BuildDoneOnDemuxerThread, or something to
> your liking (DoBuildDone ?)
> 
> BuildDoneTask is something I helped propagate but is a really poor name for
> tasks since it reads as a noun as opposed to a verb.
> 
> The reasoning for (1) is that if/when this code get refactored / moved to
> different threads, we don't want to have completely different run-time
> characteristics for the callee executing the callback depending on the
executing
> thread.
Done. I wish there was a better way to do this. It is annoying having to create
2 methods all the time for this. I wish there was some variant of NewCallback()
or base::Bind() that allowed you to specify a MessageLoop along with the
function signature and it would auto generate the PostTask() method.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:882: read_buffer_ = new uint8[read_size_];
On 2011/06/23 18:50:18, scherkus wrote:
> who deallocates this?
It was deleted in the destructor, but I've converted this to a
scoped_array<uint8> so I don't have to explicitly delete it.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:888: if (message_loop_ != MessageLoop::current())
{
On 2011/06/23 18:50:18, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:922: };
On 2011/06/23 18:50:18, scherkus wrote:
> DISALLOW etc

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:54: };
On 2011/06/23 18:50:18, scherkus wrote:
> DISALLOW etc

Done.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:82: class InfoTrackWebMParserClient : public
WebMParserClient {
On 2011/06/23 18:50:18, scherkus wrote:
> any reason why we can't break this out into media/webm?
This implementation is specific to this demuxer. For example it only allows a
single audio & video stream and only selects fields that are important to this
demuxer. I think media/webm should be reserved for code that is of more general
utility.

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:91: int64 video_default_duration() const;
On 2011/06/23 18:50:18, scherkus wrote:
> none of these durations mention the units
> 
> why not convert to base::TimeDelta during parsing then everything is
> self-documenting?

Done.

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

http://codereview.chromium.org/7203002/diff/14001/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:103: enum ElementType {
On 2011/06/23 18:50:18, scherkus wrote:
> nit: constants 'n stuff get declared at the top

Done.

http://codereview.chromium.org/7203002/diff/14001/media/webm/webm_parser.cc#n...
media/webm/webm_parser.cc:437: // Do nothing
On 2011/06/23 18:50:18, scherkus wrote:
> period

Done.

http://codereview.chromium.org/7203002/diff/14001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7203002/diff/14001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:432: bool WebMediaPlayerImpl::addData(const
unsigned char* data, unsigned length) {
On 2011/06/23 18:50:18, scherkus wrote:
> crazy naming nit we can decide on later:
> appendData vs addData ?
This will probably change to appendData() once I start reworking the WebKit side
of the changes.

[scherkus (not reviewing), 2011-06-23 23:31:17]

nits + as discussed offline we need to separate things out into files

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

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:105: class ClusterWebMParserClient : public
WebMParserClient {
On 2011/06/23 22:42:17, acolwell wrote:
> On 2011/06/23 18:50:18, scherkus wrote:
> > could this also move to media/webm ?
> The implementation is specific to this demuxer. It only allows 2 streams &
> errors on any elements other than simple blocks and cluster timecode. There
are
> other WebM elements that can appear in a cluster, but we don't need to support
> them for this use case.
> > 
> > this class could simply queue all of the buffers as member variables and
then
> > after a successful parse have the client transfer the buffers to the
> > ChunkDemuxer buffer queue
> This class already does this.

Ahh sorry I had read the code using ChunkDemuxerStream::BufferQueue as a
dependency on ChunkDemuxerStream class + was thinking of a way to break that
dependency

http://codereview.chromium.org/7203002/diff/14001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:853: if (message_loop_ != MessageLoop::current())
{
On 2011/06/23 22:42:17, acolwell wrote:
> On 2011/06/23 18:50:18, scherkus wrote:
> > as I've been discussing w/ other folks, this pattern has to die
> > 
> > Sure it saves a few LOC but it's rather maddening to outside viewers (not to
> > mention makes debugging a bit painful).  Instead:
> >   1) BuildDone should always post a task to message_loop_
> >   2) You can name the other function BuildDoneOnDemuxerThread, or something
to
> > your liking (DoBuildDone ?)
> > 
> > BuildDoneTask is something I helped propagate but is a really poor name for
> > tasks since it reads as a noun as opposed to a verb.
> > 
> > The reasoning for (1) is that if/when this code get refactored / moved to
> > different threads, we don't want to have completely different run-time
> > characteristics for the callee executing the callback depending on the
> executing
> > thread.
> Done. I wish there was a better way to do this. It is annoying having to
create
> 2 methods all the time for this. I wish there was some variant of
NewCallback()
> or base::Bind() that allowed you to specify a MessageLoop along with the
> function signature and it would auto generate the PostTask() method.

In Chrome land I believe the solution is to essentially hardcode the threads
things are supposed to be run on.

So you see code where it DCHECK()s that certain things are on IO/UI threads, and
then callees post directly to those threads.

In media code we inject/abstract the thread that it runs on, which means we
can't use a similar trick and instead would have to carry around the
message_loop we want to have callees execute callbacks on.

...but yes I agree :)

http://codereview.chromium.org/7203002/diff/21001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:208: return true;
ysed -> used

http://codereview.chromium.org/7203002/diff/21001/media/filters/in_memory_url...
File media/filters/in_memory_url_protocol.h (right):

http://codereview.chromium.org/7203002/diff/21001/media/filters/in_memory_url...
media/filters/in_memory_url_protocol.h:21: InMemoryUrlProtocol(const char* buf,
int64 size, bool streaming);
mind making this a uint8*?

I believe crogers' code uses char* but the rest of media is uint8*

http://codereview.chromium.org/7203002/diff/21001/media/filters/in_memory_url...
media/filters/in_memory_url_protocol.h:32: const char* data_;
ditto

[acolwell GONE FROM CHROMIUM, 2011-06-24 16:49:38]

Addressed comments & split ChunkDemuxerFactory & WebM parser code into separate
files.

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

http://codereview.chromium.org/7203002/diff/21001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:208: double timecode_multiplier_; // Multiplier
ysed to convert timecodes into
On 2011/06/23 23:31:17, scherkus wrote:
> ysed -> used

Done.

http://codereview.chromium.org/7203002/diff/21001/media/filters/in_memory_url...
File media/filters/in_memory_url_protocol.h (right):

http://codereview.chromium.org/7203002/diff/21001/media/filters/in_memory_url...
media/filters/in_memory_url_protocol.h:21: InMemoryUrlProtocol(const char* buf,
int64 size, bool streaming);
On 2011/06/23 23:31:17, scherkus wrote:
> mind making this a uint8*?
> 
> I believe crogers' code uses char* but the rest of media is uint8*

Done.

http://codereview.chromium.org/7203002/diff/21001/media/filters/in_memory_url...
media/filters/in_memory_url_protocol.h:32: const char* data_;
On 2011/06/23 23:31:17, scherkus wrote:
> ditto

Done.

[scherkus (not reviewing), 2011-06-24 18:27:37]

style nits but other than tests we're good!

http://codereview.chromium.org/7203002/diff/4021/media/base/buffers.cc
File media/base/buffers.cc (right):

http://codereview.chromium.org/7203002/diff/4021/media/base/buffers.cc#newcode7
media/base/buffers.cc:7: #include "base/logging.h"
?

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

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:17: #include
"media/filters/in_memory_url_protocol.h"
f > i

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:34: 0x1A, 0x45, 0xDF, 0xA3, 0x9F, // EBML (size =
0x1f)
nit: two spaces between code + comments

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:53: 0x1F, 0x43, 0xB6, 0x75, 0x80 // CLUSTER (size
= 0)
ditto

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:339: void ChunkDemuxer::SetPreload(Preload
preload) {}
NOTIMPLEMENTED?

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

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:12: 
extra line

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:41: };
remove semicolon OR you can inline this in .h since it's trivial accessor

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:45: };
remove semicolon OR you can inline this in .h since it's trivial accessor

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:117: 
extra line

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:118: Buffer*
WebMClusterParser::CreateBuffer(const uint8* data, size_t size) const {
if you want this can be static

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:124: 
extra line

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
File media/webm/webm_cluster_parser.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:16: class WebMClusterParser : private
WebMParserClient {
nit: use public
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:17: 
nit: blank line

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:29: void Reset();
these methods can certainly be dangerous (i.e., forgetting to reset everything)
so a test for this might be a good call :)

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:63: };
DISALLOW etc

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_constants.h
File media/webm/webm_constants.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_constants.h#...
media/webm/webm_constants.h:7: 
almost missed it!

namespace media { }

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_info_parser.h
File media/webm/webm_info_parser.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_info_parser....
media/webm/webm_info_parser.h:25: double duration() const;
these can be inlined if you so desire

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_info_parser....
media/webm/webm_info_parser.h:40: };
DISALLOW etc

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

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_parser.cc#ne...
media/webm/webm_parser.cc:482: int ParseWebMInfo(const uint8* buf, int size,
WebMParserClient* client) {
are these three methods supposed to be in the .h or are they defunct?

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

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_parser.h#new...
media/webm/webm_parser.h:41: int WebMParseListElement(const uint8* buf, int
size, int id,
OOC why is this method out here by itself?

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.cc:11: 
remove blank line

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.cc:60: << " TrackType " << track_type_
indentation

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parser.h
File media/webm/webm_tracks_parser.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.h:15: class WebMTracksParser : private
WebMParserClient {
public

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.h:26: int64 audio_track_num() const;
inline if you want

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.h:52: 
DISALLOW etc

[acolwell GONE FROM CHROMIUM, 2011-06-27 23:48:25]

Fixed nits & created unit tests.

http://codereview.chromium.org/7203002/diff/4021/media/base/buffers.cc
File media/base/buffers.cc (right):

http://codereview.chromium.org/7203002/diff/4021/media/base/buffers.cc#newcode7
media/base/buffers.cc:7: #include "base/logging.h"
On 2011/06/24 18:27:37, scherkus wrote:
> ?

Done.

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

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:17: #include
"media/filters/in_memory_url_protocol.h"
On 2011/06/24 18:27:37, scherkus wrote:
> f > i

Done.

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:34: 0x1A, 0x45, 0xDF, 0xA3, 0x9F, // EBML (size =
0x1f)
On 2011/06/24 18:27:37, scherkus wrote:
> nit: two spaces between code + comments

Done.

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:53: 0x1F, 0x43, 0xB6, 0x75, 0x80 // CLUSTER (size
= 0)
On 2011/06/24 18:27:37, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7203002/diff/4021/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:339: void ChunkDemuxer::SetPreload(Preload
preload) {}
On 2011/06/24 18:27:37, scherkus wrote:
> NOTIMPLEMENTED?
No. This gets called, but we can't really do anything since JavaScript is what
passes data to us. This is supposed to be a hint about how much data we preload,
but JS is totally in control of that.

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

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:12: 
On 2011/06/24 18:27:37, scherkus wrote:
> extra line

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:41: };
On 2011/06/24 18:27:37, scherkus wrote:
> remove semicolon OR you can inline this in .h since it's trivial accessor

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:45: };
On 2011/06/24 18:27:37, scherkus wrote:
> remove semicolon OR you can inline this in .h since it's trivial accessor

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:117: 
On 2011/06/24 18:27:37, scherkus wrote:
> extra line

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:118: Buffer*
WebMClusterParser::CreateBuffer(const uint8* data, size_t size) const {
On 2011/06/24 18:27:37, scherkus wrote:
> if you want this can be static

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:124: 
On 2011/06/24 18:27:37, scherkus wrote:
> extra line

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
File media/webm/webm_cluster_parser.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:16: class WebMClusterParser : private
WebMParserClient {
On 2011/06/24 18:27:37, scherkus wrote:
> nit: use public
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:17: 
On 2011/06/24 18:27:37, scherkus wrote:
> nit: blank line

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:29: void Reset();
On 2011/06/24 18:27:37, scherkus wrote:
> these methods can certainly be dangerous (i.e., forgetting to reset
everything)
> so a test for this might be a good call :)
Decided to remove Reset(). Parse resets the object before parsing now.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.h:63: };
On 2011/06/24 18:27:37, scherkus wrote:
> DISALLOW etc

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_constants.h
File media/webm/webm_constants.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_constants.h#...
media/webm/webm_constants.h:7: 
On 2011/06/24 18:27:37, scherkus wrote:
> almost missed it!
> 
> namespace media { }

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_info_parser.h
File media/webm/webm_info_parser.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_info_parser....
media/webm/webm_info_parser.h:25: double duration() const;
On 2011/06/24 18:27:37, scherkus wrote:
> these can be inlined if you so desire

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_info_parser....
media/webm/webm_info_parser.h:40: };
On 2011/06/24 18:27:37, scherkus wrote:
> DISALLOW etc

Done.

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

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_parser.cc#ne...
media/webm/webm_parser.cc:482: int ParseWebMInfo(const uint8* buf, int size,
WebMParserClient* client) {
On 2011/06/24 18:27:37, scherkus wrote:
> are these three methods supposed to be in the .h or are they defunct?
They are defunct. Removed.

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

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_parser.h#new...
media/webm/webm_parser.h:41: int WebMParseListElement(const uint8* buf, int
size, int id,
On 2011/06/24 18:27:37, scherkus wrote:
> OOC why is this method out here by itself?

It is the only method that the various WebMXXParser objects need to parse their
data. As other parsing functions in webm_parser.cc become useful I'll expose
them here.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
File media/webm/webm_tracks_parser.cc (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.cc:11: 
On 2011/06/24 18:27:37, scherkus wrote:
> remove blank line

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.cc:60: << " TrackType " << track_type_
On 2011/06/24 18:27:37, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parser.h
File media/webm/webm_tracks_parser.h (right):

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.h:15: class WebMTracksParser : private
WebMParserClient {
On 2011/06/24 18:27:37, scherkus wrote:
> public

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.h:26: int64 audio_track_num() const;
On 2011/06/24 18:27:37, scherkus wrote:
> inline if you want

Done.

http://codereview.chromium.org/7203002/diff/4021/media/webm/webm_tracks_parse...
media/webm/webm_tracks_parser.h:52: 
On 2011/06/24 18:27:37, scherkus wrote:
> DISALLOW etc

Done.

[scherkus (not reviewing), 2011-06-28 20:03:29]

LGTM w/ nits

I like the tests!

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

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:102: scoped_ptr<base::TimeDelta>
last_buffer_timestamp_;
why not use kNoTimestamp instead of a pointer?

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:59: ~ChunkDemuxerTest() {
virtual

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:138: void SetupAVFormatContext(bool
has_audio, bool has_video) {
sadness

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:161: bool InitDemuxer(bool has_audio,
bool has_video) {
all of your InitDemuxer EXPECT_EQs are true, so why not make this void + have
the EXPECT_EQ around demuxer_->Init()

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:172: .WillRepeatedly(Return(0));
indent

http://codereview.chromium.org/7203002/diff/37002/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7203002/diff/37002/media/media.gyp#newcode200
media/media.gyp:200: 'webm/cluster_builder.cc',
these two files should be part of media_unittests target

in fact I've been thinking of pulling a page out of base's playbook and having:
media
  - stays as is
media_test_support
  - contains all the mock/test stuff
media_unittests
  - contains only the unittest files

...but that's for another CL

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder.cc
File media/webm/cluster_builder.cc (right):

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.cc:12: 0x1F, 0x43, 0xB6, 0x75, // CLUSTER ID
two spaces

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.cc:24: 0xA3, // SimpleBlock ID
two spaces

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.cc:83: void ClusterBuilder::Finish() {
any checks for calling Finish twice?

also what if Finish() returned an object (i.e., a Cluster) that contained+owned
the data+size, meaning ClusterBuilder was more like a factory that generates
clusters for a given timecode

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder.h
File media/webm/cluster_builder.h (right):

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.h:15: ClusterBuilder(int64 cluster_timecode);
explicit

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.h:40: #endif // MEDIA_WEBM_CLUSTER_BUILDER_H_
two spaces

http://codereview.chromium.org/7203002/diff/37002/media/webm/webm_cluster_par...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/7203002/diff/37002/media/webm/webm_cluster_par...
media/webm/webm_cluster_parser.cc:128: Buffer*
WebMClusterParser::CreateBuffer(const uint8* data, size_t size) {
does this need to be part of the class?

I'd move to top of this file + remove from .h

[acolwell GONE FROM CHROMIUM, 2011-06-29 16:38:42]

Fixed nits

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

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:102: scoped_ptr<base::TimeDelta>
last_buffer_timestamp_;
On 2011/06/28 20:03:29, scherkus wrote:
> why not use kNoTimestamp instead of a pointer?
oh yeah.. duh.. Done.

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:59: ~ChunkDemuxerTest() {
On 2011/06/28 20:03:29, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:161: bool InitDemuxer(bool has_audio,
bool has_video) {
On 2011/06/28 20:03:29, scherkus wrote:
> all of your InitDemuxer EXPECT_EQs are true, so why not make this void + have
> the EXPECT_EQ around demuxer_->Init()

Done.

http://codereview.chromium.org/7203002/diff/37002/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:172: .WillRepeatedly(Return(0));
On 2011/06/28 20:03:29, scherkus wrote:
> indent

Done.

http://codereview.chromium.org/7203002/diff/37002/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7203002/diff/37002/media/media.gyp#newcode200
media/media.gyp:200: 'webm/cluster_builder.cc',
On 2011/06/28 20:03:29, scherkus wrote:
> these two files should be part of media_unittests target
> 
> in fact I've been thinking of pulling a page out of base's playbook and
having:
> media
>   - stays as is
> media_test_support
>   - contains all the mock/test stuff
> media_unittests
>   - contains only the unittest files
> 
> ...but that's for another CL

Done.

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder.cc
File media/webm/cluster_builder.cc (right):

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.cc:12: 0x1F, 0x43, 0xB6, 0x75, // CLUSTER ID
On 2011/06/28 20:03:29, scherkus wrote:
> two spaces

Done.

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.cc:24: 0xA3, // SimpleBlock ID
On 2011/06/28 20:03:29, scherkus wrote:
> two spaces

Done.

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.cc:83: void ClusterBuilder::Finish() {
On 2011/06/28 20:03:29, scherkus wrote:
> any checks for calling Finish twice?
> 
> also what if Finish() returned an object (i.e., a Cluster) that
contained+owned
> the data+size, meaning ClusterBuilder was more like a factory that generates
> clusters for a given timecode
Added a DCHECK, created Cluster object, & updated ChunkDemuxerTest

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder.h
File media/webm/cluster_builder.h (right):

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.h:15: ClusterBuilder(int64 cluster_timecode);
On 2011/06/28 20:03:29, scherkus wrote:
> explicit

Done.

http://codereview.chromium.org/7203002/diff/37002/media/webm/cluster_builder....
media/webm/cluster_builder.h:40: #endif // MEDIA_WEBM_CLUSTER_BUILDER_H_
On 2011/06/28 20:03:29, scherkus wrote:
> two spaces

Done.

http://codereview.chromium.org/7203002/diff/37002/media/webm/webm_cluster_par...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/7203002/diff/37002/media/webm/webm_cluster_par...
media/webm/webm_cluster_parser.cc:128: Buffer*
WebMClusterParser::CreateBuffer(const uint8* data, size_t size) {
On 2011/06/28 20:03:29, scherkus wrote:
> does this need to be part of the class?
> 
> I'd move to top of this file + remove from .h

Done.