Merging SeekableBuffer and BufferQueue

Merging SeekableBuffer and BufferQueue:
1. Removed SeekableBuffer::Buffer.
2. Extended SeekableBuffer to support all the features of BufferQueue.
3. BufferQueue removed and replaced with SeekableBuffer.
BUG=28654
TEST=none

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

[Alpha Left Google, 2010-04-26 22:33:41]

http://codereview.chromium.org/1736012/diff/18001/19004
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/1736012/diff/18001/19004#newcode242
media/base/seekable_buffer.cc:242: offset) / (*buffer)->GetDataSize();
nit: indentation

http://codereview.chromium.org/1736012/diff/18001/19005
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/1736012/diff/18001/19005#newcode61
media/base/seekable_buffer.h:61: size_t Read(uint8* data, size_t size);
nit: The order of parameter is not consistent in this class. Should be size
first and buffer afterwards.

http://codereview.chromium.org/1736012/diff/18001/19005#newcode65
media/base/seekable_buffer.h:65: size_t Copy(uint8* data, size_t size);
nit: maybe call it Peek()? I always find Copy() is a somewhat mismatch name with
what it does...

http://codereview.chromium.org/1736012/diff/18001/19005#newcode76
media/base/seekable_buffer.h:76: bool Append(size_t size, const uint8* data);
nit: You can also change this to match argument order.

http://codereview.chromium.org/1736012/diff/18001/19005#newcode106
media/base/seekable_buffer.h:106: base::TimeDelta get_time() const { return
most_recent_time_; }
nit: maybe give a bit more explaination in the comment, like this is the
projection of the timestamp of the front buffer and bytes consumed?

http://codereview.chromium.org/1736012/diff/18001/19005#newcode110
media/base/seekable_buffer.h:110: typedef std::list< scoped_refptr<Buffer> >
BufferQueue;
nit: list<scoped_refptr<Buffer> > BufferQueue;

http://codereview.chromium.org/1736012/diff/18001/19006
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/1736012/diff/18001/19006#newcode9
media/base/seekable_buffer_unittest.cc:9: #include "media/base/data_buffer.h"
nit: order includes

http://codereview.chromium.org/1736012/diff/18001/19006#newcode281
media/base/seekable_buffer_unittest.cc:281: // { 0, 1000000, 4 },
what's wrong with these inputs?

[Sergey Ulanov, 2010-04-26 23:03:29]

http://codereview.chromium.org/1736012/diff/18001/19004
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/1736012/diff/18001/19004#newcode242
media/base/seekable_buffer.cc:242: offset) / (*buffer)->GetDataSize();
On 2010/04/26 22:33:42, Alpha wrote:
> nit: indentation

Done.

http://codereview.chromium.org/1736012/diff/18001/19005
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/1736012/diff/18001/19005#newcode61
media/base/seekable_buffer.h:61: size_t Read(uint8* data, size_t size);
On 2010/04/26 22:33:42, Alpha wrote:
> nit: The order of parameter is not consistent in this class. Should be size
> first and buffer afterwards.
Right. I wanted to make it consistent with all other code (e.g. data_buffer.h),
but forgot to change Append(). Thanks.

http://codereview.chromium.org/1736012/diff/18001/19005#newcode65
media/base/seekable_buffer.h:65: size_t Copy(uint8* data, size_t size);
On 2010/04/26 22:33:42, Alpha wrote:
> nit: maybe call it Peek()? I always find Copy() is a somewhat mismatch name
with
> what it does...

Done.

http://codereview.chromium.org/1736012/diff/18001/19005#newcode76
media/base/seekable_buffer.h:76: bool Append(size_t size, const uint8* data);
On 2010/04/26 22:33:42, Alpha wrote:
> nit: You can also change this to match argument order.

Done.

http://codereview.chromium.org/1736012/diff/18001/19005#newcode106
media/base/seekable_buffer.h:106: base::TimeDelta get_time() const { return
most_recent_time_; }
On 2010/04/26 22:33:42, Alpha wrote:
> nit: maybe give a bit more explaination in the comment, like this is the
> projection of the timestamp of the front buffer and bytes consumed?

Done.

http://codereview.chromium.org/1736012/diff/18001/19005#newcode110
media/base/seekable_buffer.h:110: typedef std::list< scoped_refptr<Buffer> >
BufferQueue;
On 2010/04/26 22:33:42, Alpha wrote:
> nit: list<scoped_refptr<Buffer> > BufferQueue;
> 

Done.

http://codereview.chromium.org/1736012/diff/18001/19006
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/1736012/diff/18001/19006#newcode9
media/base/seekable_buffer_unittest.cc:9: #include "media/base/data_buffer.h"
On 2010/04/26 22:33:42, Alpha wrote:
> nit: order includes

Done.

http://codereview.chromium.org/1736012/diff/18001/19006#newcode281
media/base/seekable_buffer_unittest.cc:281: // { 0, 1000000, 4 },
On 2010/04/26 22:33:42, Alpha wrote:
> what's wrong with these inputs? 
This was copied from buffer_queue_unittest.cc. get_time() returns 0 if timestamp
of the data buffer is set to 0. Uncommented this and changed how |expected| is
calculated.

[Alpha Left Google, 2010-04-26 23:09:36]

LGTM

[scherkus (not reviewing), 2010-04-26 23:54:51]

http://codereview.chromium.org/1736012/diff/5003/4008
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/1736012/diff/5003/4008#newcode1
media/base/seekable_buffer.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
copyright year

http://codereview.chromium.org/1736012/diff/5003/4008#newcode21
media/base/seekable_buffer.cc:21: most_recent_time_(base::TimeDelta()) {
not needed

http://codereview.chromium.org/1736012/diff/5003/4008#newcode38
media/base/seekable_buffer.cc:38: size_t SeekableBuffer::Read(uint8* data,
size_t size) {
any reason for swapping the function order?

from style guide:
"When ordering function parameters, put all input-only parameters before any
output parameters."

We haven't stuck to this rule 100%, but we're trying to :)

http://codereview.chromium.org/1736012/diff/5003/4008#newcode48
media/base/seekable_buffer.cc:48: bool SeekableBuffer::Append(Buffer *buffer_in)
{
pointer goes with type

http://codereview.chromium.org/1736012/diff/5003/4009
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/1736012/diff/5003/4009#newcode1
media/base/seekable_buffer.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
copyright year

http://codereview.chromium.org/1736012/diff/5003/4009#newcode69
media/base/seekable_buffer.h:69: // are always successful. A return value of
false only means that
I know you didn't originally write this, but can you take a stab at rewriting
this comment?  It repeats itself in a few places and is confusing.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode74
media/base/seekable_buffer.h:74: // Appends |side| bytes of |data| to the
buffer. Result is the same
side -> size

http://codereview.chromium.org/1736012/diff/5003/4009#newcode75
media/base/seekable_buffer.h:75: // as for Append(buffer_in).
nit: as for Append(buffer_in) -> as Append(Buffer*)
(clearer when using types)

http://codereview.chromium.org/1736012/diff/5003/4009#newcode108
media/base/seekable_buffer.h:108: // Append(data, size), then returns value that
corresponds to the last
nit: Append(data, size) -> Append(const uint8*, size_t)

http://codereview.chromium.org/1736012/diff/5003/4009#newcode111
media/base/seekable_buffer.h:111: base::TimeDelta get_time() const { return
most_recent_time_; }
unix_hacker style functions shouldn't be prefixed with get_

for example, given variable Foo bar you'd have:
Foo bar()
void set_bar(Foo f)

in fact I'd say rename this function to current_time() and the variable
most_recent_time_ to current_time() (they're practically synonyms of each
other!)

I'd also prefer if this function returned StreamSample::kInvalidTimestamp during
the strange cases (no buffers, uint8* Append, etc...) but you'll have to inspect
the callees to make sure nothing breaks

http://codereview.chromium.org/1736012/diff/5003/4009#newcode156
media/base/seekable_buffer.h:156: // Keeps track of the most recent time we've
seen in case the |queue_| is
queue_ doesn't exist any more

http://codereview.chromium.org/1736012/diff/5003/4009#newcode159
media/base/seekable_buffer.h:159: };
this class is missing DISALLOW_COPY_AND_ASSIGN(SeekableBuffer)

http://codereview.chromium.org/1736012/diff/5003/4010
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/1736012/diff/5003/4010#newcode1
media/base/seekable_buffer_unittest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
copyright year

http://codereview.chromium.org/1736012/diff/5003/4010#newcode310
media/base/seekable_buffer_unittest.cc:310: int64 expected =
tests[i].first_time_useconds == 0 ? 0 :
problem with this particular test is that we're essentially duplicating the
formula

I'd make the expected values part of the tests[] array.  pick some easy numbers
for kWriteSize, etc.. to make the math easier

[scherkus (not reviewing), 2010-04-26 23:56:29]

if the StreamSample::kInvalidTimestamp change becomes too involved, feel free to
write down a TODO (I believe that constant didn't exist when BufferQueue was
originally written)

[Sergey Ulanov, 2010-04-27 00:38:47]

Thanks!

http://codereview.chromium.org/1736012/diff/5003/4008
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/1736012/diff/5003/4008#newcode1
media/base/seekable_buffer.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2010/04/26 23:54:52, scherkus wrote:
> copyright year

Done.

http://codereview.chromium.org/1736012/diff/5003/4008#newcode21
media/base/seekable_buffer.cc:21: most_recent_time_(base::TimeDelta()) {
On 2010/04/26 23:54:52, scherkus wrote:
> not needed

Done.

http://codereview.chromium.org/1736012/diff/5003/4008#newcode38
media/base/seekable_buffer.cc:38: size_t SeekableBuffer::Read(uint8* data,
size_t size) {
On 2010/04/26 23:54:52, scherkus wrote:
> any reason for swapping the function order?
> 
> from style guide:
> "When ordering function parameters, put all input-only parameters before any
> output parameters."
> 
> We haven't stuck to this rule 100%, but we're trying to :)
All other code that deals with buffers uses this order, and it is not only in
media/*. For example look in net/base/file_stream.h.
Besides, depending on how you look at it you can consider data as an input
parameter or size as an output parameter :). If we stick to the rule then we
might end up having these two parameters separated by some other parameter,
which, IMO, would make code unreadable (e.g. imagine you need to pass two output
buffers to a function). Ideally this should be just one parameter, instead of
two, e.g. WritableBuffer*.

http://codereview.chromium.org/1736012/diff/5003/4008#newcode48
media/base/seekable_buffer.cc:48: bool SeekableBuffer::Append(Buffer *buffer_in)
{
On 2010/04/26 23:54:52, scherkus wrote:
> pointer goes with type
ouch.

http://codereview.chromium.org/1736012/diff/5003/4009
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/1736012/diff/5003/4009#newcode1
media/base/seekable_buffer.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2010/04/26 23:54:52, scherkus wrote:
> copyright year

Done.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode69
media/base/seekable_buffer.h:69: // are always successful. A return value of
false only means that
On 2010/04/26 23:54:52, scherkus wrote:
> I know you didn't originally write this, but can you take a stab at rewriting
> this comment?  It repeats itself in a few places and is confusing.

Done.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode74
media/base/seekable_buffer.h:74: // Appends |side| bytes of |data| to the
buffer. Result is the same
On 2010/04/26 23:54:52, scherkus wrote:
> side -> size

Done.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode75
media/base/seekable_buffer.h:75: // as for Append(buffer_in).
On 2010/04/26 23:54:52, scherkus wrote:
> nit: as for Append(buffer_in) -> as Append(Buffer*)
> (clearer when using types)

Done.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode108
media/base/seekable_buffer.h:108: // Append(data, size), then returns value that
corresponds to the last
On 2010/04/26 23:54:52, scherkus wrote:
> nit: Append(data, size) -> Append(const uint8*, size_t)

Done.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode111
media/base/seekable_buffer.h:111: base::TimeDelta get_time() const { return
most_recent_time_; }
On 2010/04/26 23:54:52, scherkus wrote:
> unix_hacker style functions shouldn't be prefixed with get_
> 
> for example, given variable Foo bar you'd have:
> Foo bar()
> void set_bar(Foo f)
> 
> in fact I'd say rename this function to current_time() and the variable
> most_recent_time_ to current_time() (they're practically synonyms of each
> other!)
Done.


> I'd also prefer if this function returned StreamSample::kInvalidTimestamp
during
> the strange cases (no buffers, uint8* Append, etc...) but you'll have to
inspect
> the callees to make sure nothing breaks
Agree. For now I wanted to make it work the same way BufferQueue worked. I will
change this to StreamSample::kInvalidTimestamp in another changelist.

http://codereview.chromium.org/1736012/diff/5003/4009#newcode156
media/base/seekable_buffer.h:156: // Keeps track of the most recent time we've
seen in case the |queue_| is
On 2010/04/26 23:54:52, scherkus wrote:
> queue_ doesn't exist any more
Changed to buffers_

http://codereview.chromium.org/1736012/diff/5003/4009#newcode159
media/base/seekable_buffer.h:159: };
On 2010/04/26 23:54:52, scherkus wrote:
> this class is missing DISALLOW_COPY_AND_ASSIGN(SeekableBuffer)

Done.

http://codereview.chromium.org/1736012/diff/5003/4010
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/1736012/diff/5003/4010#newcode1
media/base/seekable_buffer_unittest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/04/26 23:54:52, scherkus wrote:
> copyright year

Done.

http://codereview.chromium.org/1736012/diff/5003/4010#newcode310
media/base/seekable_buffer_unittest.cc:310: int64 expected =
tests[i].first_time_useconds == 0 ? 0 :
On 2010/04/26 23:54:52, scherkus wrote:
> problem with this particular test is that we're essentially duplicating the
> formula
> 
> I'd make the expected values part of the tests[] array.  pick some easy
numbers
> for kWriteSize, etc.. to make the math easier

Done.

[Sergey Ulanov, 2010-04-27 00:41:20]

On 2010/04/26 23:56:29, scherkus wrote:
> if the StreamSample::kInvalidTimestamp change becomes too involved, feel free
to
> write down a TODO (I believe that constant didn't exist when BufferQueue was
> originally written)

Added TODO.

[scherkus (not reviewing), 2010-04-27 02:19:26]

LGTM w/ one tiny comment nit

feel free to check in

http://codereview.chromium.org/1736012/diff/33001/3014
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/1736012/diff/33001/3014#newcode69
media/base/seekable_buffer.h:69: // added to the buffer in any case.
nit: added -> is added