Use scoped_ptr<> and deleter for media::DecoderBuffer::data_.

Use scoped_ptr<> and deleter for media::DecoderBuffer::data_.

BUG=150920
TEST=media_unittests pass


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

[xhwang, 2012-12-21 00:49:46]

Hi, just want give the new scoped_ptr a try. PTAL!

[DaleCurtis, 2012-12-21 01:11:44]

drive by!

https://codereview.chromium.org/11642054/diff/1/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/11642054/diff/1/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:24: struct AlignedFreeDeleter {
Just use ScopedPtrAlignedFree from base/memory/aligned_memory.h ?

[xhwang, 2012-12-21 01:31:05]

https://codereview.chromium.org/11642054/diff/1/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/11642054/diff/1/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:24: struct AlignedFreeDeleter {
On 2012/12/21 01:11:44, DaleCurtis wrote:
> Just use ScopedPtrAlignedFree from base/memory/aligned_memory.h ?

Indeed. Thanks for pointing that out.

[scherkus (not reviewing), 2012-12-21 15:13:25]

lgtm w/ nit

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer...
media/base/decoder_buffer.cc:37: memset(data_.get() + buffer_size_, 0,
kPaddingSize);
nit: do we still need the .get()s?

AFAIK we added an operator*() to scoped_ptr<> so .get() is optional (dropping
the .get() saves characters but above all it should be consistent within a file)

[awong, 2012-12-28 21:17:26]

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer...
media/base/decoder_buffer.h:66: scoped_ptr<uint8[], base::ScopedPtrAlignedFree>
data_;
Is uint8* or uint8[] more appropriate here?  Do you want the array-access API,
or the pointer API?

[xhwang, 2012-12-29 22:37:26]

PTAL again!

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer...
media/base/decoder_buffer.cc:37: memset(data_.get() + buffer_size_, 0,
kPaddingSize);
On 2012/12/21 15:13:25, scherkus wrote:
> nit: do we still need the .get()s?
> 
> AFAIK we added an operator*() to scoped_ptr<> so .get() is optional (dropping
> the .get() saves characters but above all it should be consistent within a
file)

For implicit conversion to uint8*, we need a conversion operator like:
operator uint8*().

operator*() is for indirection, not conversion.

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/11642054/diff/15002/media/base/decoder_buffer...
media/base/decoder_buffer.h:66: scoped_ptr<uint8[], base::ScopedPtrAlignedFree>
data_;
On 2012/12/28 21:17:26, awong wrote:
> Is uint8* or uint8[] more appropriate here?  Do you want the array-access API,
> or the pointer API?

Right. I don't use array-access API. Changed to scoped_ptr<uint8, ...>.

[DaleCurtis, 2013-01-02 19:09:05]

lgtm % nit.

https://codereview.chromium.org/11642054/diff/21001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/11642054/diff/21001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:22: if (!data || !buffer_size) {
Instead of adding this, you could instead just change the DCHECK_GE() in
initialize to a CHECK_GE(). Since buffer_size is an int, otherwise it'd make
sense for this to be <=0.

[awong, 2013-01-02 19:47:32]

LGTM

Have until next week to commit though please.

[xhwang, 2013-01-03 17:27:15]

awong: will hold off until next week. Is anytime next week be good to commit? Or
should I wait for a specific CL or certain date/time?

https://codereview.chromium.org/11642054/diff/21001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/11642054/diff/21001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:22: if (!data || !buffer_size) {
On 2013/01/02 19:09:05, DaleCurtis wrote:
> Instead of adding this, you could instead just change the DCHECK_GE() in
> initialize to a CHECK_GE(). Since buffer_size is an int, otherwise it'd make
> sense for this to be <=0.

We actually have a test for 0-byte frame:
http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/ffmpeg_video...

Changed to <=0

[commit-bot: I haz the power, 2013-01-09 00:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11642054/25002

[commit-bot: I haz the power, 2013-01-09 04:19:16]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-09 04:48:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11642054/25002

[commit-bot: I haz the power, 2013-01-09 04:52:01]

Change committed as 175710