Add StreamParserBuffer to ChunkDemuxer

Add StreamParserBuffer to ChunkDemuxer

This will allow SourceBufferStreams to identify keyframes.

BUG=125680
TEST=NONE

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

[vrk (LEFT CHROMIUM), 2012-05-01 21:27:30]

+fischman

Rewrote as discussed!

[acolwell GONE FROM CHROMIUM, 2012-05-01 22:08:50]

LGTM % nits

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
File media/base/data_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
media/base/data_buffer.cc:41: scoped_refptr<DataBuffer> data_buffer(
nit: fits on one line now?

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
File media/base/stream_parser_buffer.h (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
media/base/stream_parser_buffer.h:15: static StreamParserBuffer*
CreateEOSBuffer();
nit: Any reason we can't return a scoped_refptr<StreamParserBuffer> like
CopyFrom?

[Ami GONE FROM CHROMIUM, 2012-05-01 22:34:16]

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
File media/base/data_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
media/base/data_buffer.cc:43: memcpy(data_buffer->data_.get(), data, data_size);
If you put this in a protected DataBuffer ctor, then I think you could avoid the
code-duplication between DataBuffer::CopyFrom() and
StreamParserBuffer::CopyFrom().

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
media/base/data_buffer.cc:44: data_buffer->SetDataSize(data_size);
Shouldn't the ctor be in charge of this call?
(afaict this is only to account for the padding)

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
File media/base/stream_parser_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
media/base/stream_parser_buffer.cc:23: buffer->SetDataSize(data_size);
see elsewhere; I think you can drop this.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/webm/webm_clu...
File media/webm/webm_cluster_parser.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/webm/webm_clu...
media/webm/webm_cluster_parser.cc:118: bool is_keyframe = ((flags & 0x80) != 0);
IWBN if 0x80 came from some NUMERIC_CONSTANT.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/webm/webm_clu...
media/webm/webm_cluster_parser.cc:118: bool is_keyframe = ((flags & 0x80) != 0);
((flags & 0x80) != 0)
==
flags & 0x80
FWIW

[scherkus (not reviewing), 2012-05-01 23:23:51]

(drive-by props: having seen the before and after I commend you on introducing
StreamParserBuffer instead of adding IsKeyframe() to Buffer!)

[vrk (LEFT CHROMIUM), 2012-05-02 17:24:51]

Thanks for the review!

With fischman's suggestion to add the protected constructor, it no longer made
sense to modify the "DataBuffer(int buffer_size)" constructor, so I reverted my
changes there.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
File media/base/data_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
media/base/data_buffer.cc:41: scoped_refptr<DataBuffer> data_buffer(
On 2012/05/01 22:08:50, acolwell wrote:
> nit: fits on one line now?

Done.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
media/base/data_buffer.cc:43: memcpy(data_buffer->data_.get(), data, data_size);
On 2012/05/01 22:34:16, Ami Fischman wrote:
> If you put this in a protected DataBuffer ctor, then I think you could avoid
the
> code-duplication between DataBuffer::CopyFrom() and
> StreamParserBuffer::CopyFrom().

Done.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/data_buf...
media/base/data_buffer.cc:44: data_buffer->SetDataSize(data_size);
On 2012/05/01 22:34:16, Ami Fischman wrote:
> Shouldn't the ctor be in charge of this call?
> (afaict this is only to account for the padding)

Done.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
File media/base/stream_parser_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
media/base/stream_parser_buffer.cc:23: buffer->SetDataSize(data_size);
On 2012/05/01 22:34:16, Ami Fischman wrote:
> see elsewhere; I think you can drop this.

Done.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
File media/base/stream_parser_buffer.h (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/base/stream_p...
media/base/stream_parser_buffer.h:15: static StreamParserBuffer*
CreateEOSBuffer();
On 2012/05/01 22:08:50, acolwell wrote:
> nit: Any reason we can't return a scoped_refptr<StreamParserBuffer> like
> CopyFrom?

Done.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/webm/webm_clu...
File media/webm/webm_cluster_parser.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/webm/webm_clu...
media/webm/webm_cluster_parser.cc:118: bool is_keyframe = ((flags & 0x80) != 0);
On 2012/05/01 22:34:16, Ami Fischman wrote:
> ((flags & 0x80) != 0)
> ==
> flags & 0x80
> FWIW
> 

Done.

https://chromiumcodereview.appspot.com/10269022/diff/8001/media/webm/webm_clu...
media/webm/webm_cluster_parser.cc:118: bool is_keyframe = ((flags & 0x80) != 0);
On 2012/05/01 22:34:16, Ami Fischman wrote:
> IWBN if 0x80 came from some NUMERIC_CONSTANT.

I'm not sure where to put it, so I'm going to hold off on this for now. (There
is webm_constants.h, but looks like they only contain IDs, and webm_parser.cc
uses some literal values as well.) Added comment and link to spec.

[Ami GONE FROM CHROMIUM, 2012-05-02 17:57:25]

LGTM % nits

(also, I'd have in-lined AllocateBuffer into the ctor, but that's pure personal
preference)

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
File media/base/data_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
media/base/data_buffer.cc:36: data_size_(0) {
Why init to 0 here and then SetDataSize in the body instead of just init'ing to
data_size here?

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
media/base/data_buffer.cc:42: void DataBuffer::AllocateBuffer(int data_size) {
Can this be inlined into the ctor?

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
File media/base/data_buffer.h (right):

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
media/base/data_buffer.h:48: DataBuffer(const uint8* data, int size);
// Copies from [data,data+size) to owned array.

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/stream_...
File media/base/stream_parser_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/stream_...
media/base/stream_parser_buffer.cc:16: return scoped_refptr<StreamParserBuffer>(
FWIW, 
return scoped_refptr<T>(new T(...));
is shorter as
return make_scoped_refptr(new T(...));
when T's name is longer than 2 characters :)

[vrk (LEFT CHROMIUM), 2012-05-02 19:55:18]

Thanks everyone!

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
File media/base/data_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
media/base/data_buffer.cc:36: data_size_(0) {
On 2012/05/02 17:57:25, Ami Fischman wrote:
> Why init to 0 here and then SetDataSize in the body instead of just init'ing
to
> data_size here?

Talked offline.

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
media/base/data_buffer.cc:42: void DataBuffer::AllocateBuffer(int data_size) {
On 2012/05/02 17:57:25, Ami Fischman wrote:
> Can this be inlined into the ctor?

Done.

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
File media/base/data_buffer.h (right):

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/data_bu...
media/base/data_buffer.h:48: DataBuffer(const uint8* data, int size);
On 2012/05/02 17:57:25, Ami Fischman wrote:
> // Copies from [data,data+size) to owned array.

Done.

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/stream_...
File media/base/stream_parser_buffer.cc (right):

https://chromiumcodereview.appspot.com/10269022/diff/15001/media/base/stream_...
media/base/stream_parser_buffer.cc:16: return scoped_refptr<StreamParserBuffer>(
On 2012/05/02 17:57:25, Ami Fischman wrote:
> FWIW, 
> return scoped_refptr<T>(new T(...));
> is shorter as
> return make_scoped_refptr(new T(...));
> when T's name is longer than 2 characters :)

Forgot about that function, thanks fischman! Changed here and elsewhere.