Refactor WritableBuffer interface for more useful ptr management.

Refactor WritableBuffer interface for more useful ptr management.
BUG=16011
TEST=DataBuffer unittest

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

[kylep, 2009-07-13 22:21:51]

Review please! This change is so I don't have to change AudioRendererBase's
FillBuffer() signature.

[scherkus (not reviewing), 2009-07-14 00:52:45]

sorry it took a bit of time getting to, but looks great!

few nits on your memory usage

http://codereview.chromium.org/149573/diff/28/1013
File media/base/buffers.h (right):

http://codereview.chromium.org/149573/diff/28/1013#newcode109
Line 109: // to the |buffer_size| passed in the ctor.
since this is an interface and doesn't have a constructor, I'd change this to
say "less than or equal to GetBufferSize()."

http://codereview.chromium.org/149573/diff/28/1014
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/28/1014#newcode8
Line 8: // DataBuffer assumes that memory was allocated with new char[].
update this to say uint8

http://codereview.chromium.org/149573/diff/28/1014#newcode34
Line 34: uint8* data_;
use a scoped_ptr instead

http://codereview.chromium.org/149573/diff/28/1011
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/28/1011#newcode39
Line 39: TEST(DataBufferTest, WrittableBufferImpl) {
WrittableBufferImpl -> WritableBufferImpl

http://codereview.chromium.org/149573/diff/28/1011#newcode46
Line 46: uint8* data = new uint8[kDataSize];
memory leak!

use scoped_ptr

http://codereview.chromium.org/149573/diff/28/1011#newcode58
Line 58: data = new uint8[kNewDataSize + 10];
memory leak!

use scoped_ptr

http://codereview.chromium.org/149573/diff/28/1008
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/149573/diff/28/1008#newcode110
Line 110: uint8* data = new uint8[output_buffer_size];
potential memory leak!

good practice:
 1) Allocate into a scoped_ptr
 2) Check if it succeeded and fail gracefully if possible
 3) Release the pointer into your destination

[kylep, 2009-07-14 17:33:19]

http://codereview.chromium.org/149573/diff/28/1013
File media/base/buffers.h (right):

http://codereview.chromium.org/149573/diff/28/1013#newcode109
Line 109: // to the |buffer_size| passed in the ctor.
On 2009/07/14 00:52:45, scherkus wrote:
> since this is an interface and doesn't have a constructor, I'd change this to
> say "less than or equal to GetBufferSize()."

Done.

http://codereview.chromium.org/149573/diff/28/1014
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/28/1014#newcode8
Line 8: // DataBuffer assumes that memory was allocated with new char[].
On 2009/07/14 00:52:45, scherkus wrote:
> update this to say uint8

Done.

http://codereview.chromium.org/149573/diff/28/1014#newcode34
Line 34: uint8* data_;
On 2009/07/14 00:52:45, scherkus wrote:
> use a scoped_ptr instead

Done.

http://codereview.chromium.org/149573/diff/28/1011
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/28/1011#newcode39
Line 39: TEST(DataBufferTest, WrittableBufferImpl) {
On 2009/07/14 00:52:45, scherkus wrote:
> WrittableBufferImpl -> WritableBufferImpl

Done.

http://codereview.chromium.org/149573/diff/28/1011#newcode46
Line 46: uint8* data = new uint8[kDataSize];
On 2009/07/14 00:52:45, scherkus wrote:
> memory leak!
> 
> use scoped_ptr

Done.

http://codereview.chromium.org/149573/diff/28/1011#newcode58
Line 58: data = new uint8[kNewDataSize + 10];
On 2009/07/14 00:52:45, scherkus wrote:
> memory leak!
> 
> use scoped_ptr

Done.

http://codereview.chromium.org/149573/diff/28/1008
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/149573/diff/28/1008#newcode110
Line 110: uint8* data = new uint8[output_buffer_size];
On 2009/07/14 00:52:45, scherkus wrote:
> potential memory leak!
> 
> good practice:
>  1) Allocate into a scoped_ptr
>  2) Check if it succeeded and fail gracefully if possible
>  3) Release the pointer into your destination

Does 2 entail DCHECKs? I did 1 and 3.

[scherkus (not reviewing), 2009-07-14 18:46:01]

http://codereview.chromium.org/149573/diff/28/1008
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/149573/diff/28/1008#newcode110
Line 110: uint8* data = new uint8[output_buffer_size];
On 2009/07/14 17:33:19, kylep wrote:
> On 2009/07/14 00:52:45, scherkus wrote:
> > potential memory leak!
> > 
> > good practice:
> >  1) Allocate into a scoped_ptr
> >  2) Check if it succeeded and fail gracefully if possible
> >  3) Release the pointer into your destination
> 
> Does 2 entail DCHECKs? I did 1 and 3.

for here (2) won't really matter because we'll memcpy to a NULL pointer and
crash

what I'd do for better logging is the following:

if (!data.get()) {
  NOTREACHED() << "Could not allocate output audio buffer";
}

http://codereview.chromium.org/149573/diff/35/1029
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/35/1029#newcode35
Line 35: scoped_ptr<uint8> data_;
again I made a tiny-huge mistake, this should be scoped_array<>

http://codereview.chromium.org/149573/diff/35/1026
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/35/1026#newcode46
Line 46: scoped_ptr<uint8> data(new uint8[kDataSize]);
I made a tiny-huge mistake... these should be scoped_array<> because we allocate
with new[] and need to use delete[]

sorry :(

http://codereview.chromium.org/149573/diff/35/1026#newcode50
Line 50: ASSERT_TRUE(data.get());
you can't use |data| anymore after you've called release() because it'll be NULL

what you should do is release it into |buffer|, then call
buffer->GetWritableData() and use that pointer.

http://codereview.chromium.org/149573/diff/35/1026#newcode61
Line 61: ASSERT_TRUE(data.get());
same here

I'm pretty sure these tests will fail if you run them

http://codereview.chromium.org/149573/diff/35/1023
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/149573/diff/35/1023#newcode110
Line 110: scoped_ptr<uint8> data(new uint8[output_buffer_size]);
scoped_array<>

http://codereview.chromium.org/149573/diff/35/1023#newcode113
Line 113: memcpy(data.get(), output_buffer, output_buffer_size);
yeah you can't use |data| any more

use GetWritableData() or cache the pointer

[scherkus (not reviewing), 2009-07-14 18:54:27]

Also fix up your changelist description:

WrittableBuffer -> WritableBuffer

Add TEST= and BUG= fields (can tag it with your audio work)

[awong, 2009-07-14 19:43:37]

couple small comments.

http://codereview.chromium.org/149573/diff/28/1008
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/149573/diff/28/1008#newcode110
Line 110: uint8* data = new uint8[output_buffer_size];
On 2009/07/14 18:46:01, scherkus wrote:
> On 2009/07/14 17:33:19, kylep wrote:
> > On 2009/07/14 00:52:45, scherkus wrote:
> > > potential memory leak!
> > > 
> > > good practice:
> > >  1) Allocate into a scoped_ptr
> > >  2) Check if it succeeded and fail gracefully if possible
> > >  3) Release the pointer into your destination
> > 
> > Does 2 entail DCHECKs? I did 1 and 3.
> 
> for here (2) won't really matter because we'll memcpy to a NULL pointer and
> crash

kibitz from ajwong: If it's an error condition that you do not have a plan for
sensible recovery, use a CHECK instead of a DCHECK.  DCHECK is a sanity check
for things that cannot happen (ie. programitcally, this *should* be impossible).
 CHECK is for invariants that cannot be violated.  Out of memory is one of
these.  Almost no one writes code that can handle OOM, so if you get it, crash!

http://codereview.chromium.org/149573/diff/35/1029
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/35/1029#newcode20
Line 20: DataBuffer(uint8* buffer, size_t buffer_size);
I might have missed it, but is there a use case where you will be passed in a
raw buffer that you didn't allocate yourself?  It feels like the usage is always
"new []" then pass ownership to the Databuffer.  In that case, why not just put
the new into the constructor?

http://codereview.chromium.org/149573/diff/35/1026
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/35/1026#newcode46
Line 46: scoped_ptr<uint8> data(new uint8[kDataSize]);
On 2009/07/14 18:46:01, scherkus wrote:
> I made a tiny-huge mistake... these should be scoped_array<> because we
allocate
> with new[] and need to use delete[]
> 
> sorry :(

Can't wait till they finally kill this distinction in C++0x.

Point of trivia, there isn't necessarily a distinction between new X and new []
X.  It's compiler dependent. However, mismatching them is invalid by spec. :P

[scherkus (not reviewing), 2009-07-14 20:12:55]

On 2009/07/14 19:43:37, awong wrote:
> couple small comments.
> 
> http://codereview.chromium.org/149573/diff/28/1008
> File media/filters/ffmpeg_audio_decoder.cc (right):
> 
> http://codereview.chromium.org/149573/diff/28/1008#newcode110
> Line 110: uint8* data = new uint8[output_buffer_size];
> On 2009/07/14 18:46:01, scherkus wrote:
> > On 2009/07/14 17:33:19, kylep wrote:
> > > On 2009/07/14 00:52:45, scherkus wrote:
> > > > potential memory leak!
> > > > 
> > > > good practice:
> > > >  1) Allocate into a scoped_ptr
> > > >  2) Check if it succeeded and fail gracefully if possible
> > > >  3) Release the pointer into your destination
> > > 
> > > Does 2 entail DCHECKs? I did 1 and 3.
> > 
> > for here (2) won't really matter because we'll memcpy to a NULL pointer and
> > crash
> 
> kibitz from ajwong: If it's an error condition that you do not have a plan for
> sensible recovery, use a CHECK instead of a DCHECK.  DCHECK is a sanity check
> for things that cannot happen (ie. programitcally, this *should* be
impossible).
>  CHECK is for invariants that cannot be violated.  Out of memory is one of
> these.  Almost no one writes code that can handle OOM, so if you get it,
crash!
> 
> http://codereview.chromium.org/149573/diff/35/1029
> File media/base/data_buffer.h (right):
> 
> http://codereview.chromium.org/149573/diff/35/1029#newcode20
> Line 20: DataBuffer(uint8* buffer, size_t buffer_size);
> I might have missed it, but is there a use case where you will be passed in a
> raw buffer that you didn't allocate yourself?  It feels like the usage is
always
> "new []" then pass ownership to the Databuffer.  In that case, why not just
put
> the new into the constructor?

I recommended this API to him on the assumption that attempting to allocate a
bunch of memory in a constructor would be bad.  However I guess if we combined
your kibitz with your recommendation here, we can allocate in the ctor and
CHECK+die if it failed.

> 
> http://codereview.chromium.org/149573/diff/35/1026
> File media/base/data_buffer_unittest.cc (right):
> 
> http://codereview.chromium.org/149573/diff/35/1026#newcode46
> Line 46: scoped_ptr<uint8> data(new uint8[kDataSize]);
> On 2009/07/14 18:46:01, scherkus wrote:
> > I made a tiny-huge mistake... these should be scoped_array<> because we
> allocate
> > with new[] and need to use delete[]
> > 
> > sorry :(
> 
> Can't wait till they finally kill this distinction in C++0x.
> 
> Point of trivia, there isn't necessarily a distinction between new X and new
[]
> X.  It's compiler dependent. However, mismatching them is invalid by spec. :P

[kylep, 2009-07-14 22:51:43]

http://codereview.chromium.org/149573/diff/35/1029
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/35/1029#newcode20
Line 20: DataBuffer(uint8* buffer, size_t buffer_size);
On 2009/07/14 19:43:38, awong wrote:
> I might have missed it, but is there a use case where you will be passed in a
> raw buffer that you didn't allocate yourself?  It feels like the usage is
always
> "new []" then pass ownership to the Databuffer.  In that case, why not just
put
> the new into the constructor?

Actually, the main point of this refactor is so I can encapsulate a pointer
passed to AudioRendererBase's FillBuffer method in a DataBuffer for delegation
to an AudioRendererAlgorithm for filling. So yes, I need that ctor (or at least
a way to give it a pointer to own).

http://codereview.chromium.org/149573/diff/35/1029#newcode35
Line 35: scoped_ptr<uint8> data_;
On 2009/07/14 18:46:01, scherkus wrote:
> again I made a tiny-huge mistake, this should be scoped_array<>

Done.

http://codereview.chromium.org/149573/diff/35/1026
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/35/1026#newcode46
Line 46: scoped_ptr<uint8> data(new uint8[kDataSize]);
On 2009/07/14 19:43:38, awong wrote:
> On 2009/07/14 18:46:01, scherkus wrote:
> > I made a tiny-huge mistake... these should be scoped_array<> because we
> allocate
> > with new[] and need to use delete[]
> > 
> > sorry :(
> 
> Can't wait till they finally kill this distinction in C++0x.
> 
> Point of trivia, there isn't necessarily a distinction between new X and new
[]
> X.  It's compiler dependent. However, mismatching them is invalid by spec. :P

I can see that. You can treat an "array" just like a pointer after you make it.
This is pretty much a moot point though - I made a new DataBuffer ctor.

[scherkus (not reviewing), 2009-07-15 02:17:24]

LGTM with some nits

http://codereview.chromium.org/149573/diff/44/51
File media/base/data_buffer.cc (right):

http://codereview.chromium.org/149573/diff/44/51#newcode19
Line 19: data_size_(buffer_size) {
nit: data_size_ is reserved for the amount of valid data, which upon
constructing should be set to 0 (back this up with a unit test for bonus points)

http://codereview.chromium.org/149573/diff/44/53
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/44/53#newcode21
Line 21: explicit DataBuffer(size_t buffer_size);
comments for both ctors

http://codereview.chromium.org/149573/diff/44/50
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/44/50#newcode48
Line 48: uint8* data = buffer->GetWritableData();
nit add a blank line above this line and delete the blank line beneath it

[kylep, 2009-07-15 17:21:22]

I made some more minor changes when I added to data_buffer_unittest.cc Some of
the EXPECT_EQ conditions were backwards. Also, it might be useful for
base::TimeDelta to have an operator<< so we can use EXPECT_EQ to compare them
instead of EXPECT_TRUE(td1 == td2).

http://codereview.chromium.org/149573/diff/44/51
File media/base/data_buffer.cc (right):

http://codereview.chromium.org/149573/diff/44/51#newcode19
Line 19: data_size_(buffer_size) {
On 2009/07/15 02:17:24, scherkus wrote:
> nit: data_size_ is reserved for the amount of valid data, which upon
> constructing should be set to 0 (back this up with a unit test for bonus
points)

Done.

http://codereview.chromium.org/149573/diff/44/53
File media/base/data_buffer.h (right):

http://codereview.chromium.org/149573/diff/44/53#newcode21
Line 21: explicit DataBuffer(size_t buffer_size);
On 2009/07/15 02:17:24, scherkus wrote:
> comments for both ctors

Done.

http://codereview.chromium.org/149573/diff/44/50
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/149573/diff/44/50#newcode48
Line 48: uint8* data = buffer->GetWritableData();
On 2009/07/15 02:17:24, scherkus wrote:
> nit add a blank line above this line and delete the blank line beneath it

Done.

[scherkus (not reviewing), 2009-07-15 20:40:47]

LGTM