Replace av_malloc with AlignedAlloc for memory allocation in DecoderBuffer.

Replace av_malloc with AlignedAlloc for memory allocation in DecoderBuffer.

BUG=150920, 145551
TEST=media_unittests


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

[xhwang, 2012-09-19 23:48:03]

As discussed. The numbers are defined in the header file instead of cc file so
that they are available at compile time for COMPILE_ASSERT.

PTAL!

[ddorwin, 2012-09-19 23:59:18]

http://codereview.chromium.org/10949029/diff/1/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10949029/diff/1/media/base/decoder_buffer.cc#n...
media/base/decoder_buffer.cc:8: #include "base/memory/aligned_memory.h"
This does no harm, but is not technically required for Android. Not sure what
the preference is for such situations.

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

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:45: COMPILE_ASSERT(DecoderBuffer::kPaddingSize >=
FF_INPUT_BUFFER_PADDING_SIZE,
Shouldn't the new constants be multiples (1 or greater) of the FFmpeg values?

Examples:
Is it okay if kPaddingSize is 17?
Is it not okay if kAlignmentSize is 64?

[DaleCurtis, 2012-09-20 00:00:04]

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

http://codereview.chromium.org/10949029/diff/1/media/base/decoder_buffer.h#ne...
media/base/decoder_buffer.h:24: static const int kPaddingSize = 16;
Comments for both.

enum { kPaddingBytes = 16, kAlignmentBytes = 32} ?

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

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:45: COMPILE_ASSERT(DecoderBuffer::kPaddingSize >=
FF_INPUT_BUFFER_PADDING_SIZE,
Do this in the cc file instead.

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:48: COMPILE_ASSERT(DecoderBuffer::kAlignmentSize ==
32,
>= 32?

[acolwell GONE FROM CHROMIUM, 2012-09-20 00:16:21]

Just a few comments. You should also update the decoder_buffer_unittest so it
doesn't depend on ffmpeg_common.h.

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

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:16: #include "media/base/decoder_buffer.h"
move this into cc file too.

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:43: // padded.  Using av_malloc with padding
ensures FFmpeg only receives data
Comment should be updated. We don't use av_malloc anymore right?

[xhwang, 2012-09-20 01:43:03]

Comments resolved. Also updated DecoderBufferTest as acolwell@ pointed out.

PTAL!

http://codereview.chromium.org/10949029/diff/1/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10949029/diff/1/media/base/decoder_buffer.cc#n...
media/base/decoder_buffer.cc:8: #include "base/memory/aligned_memory.h"
On 2012/09/19 23:59:18, ddorwin wrote:
> This does no harm, but is not technically required for Android. Not sure what
> the preference is for such situations.

Done.

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

http://codereview.chromium.org/10949029/diff/1/media/base/decoder_buffer.h#ne...
media/base/decoder_buffer.h:24: static const int kPaddingSize = 16;
On 2012/09/20 00:00:04, DaleCurtis wrote:
> Comments for both.
> 
> enum { kPaddingBytes = 16, kAlignmentBytes = 32} ?

Done.

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

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:16: #include "media/base/decoder_buffer.h"
On 2012/09/20 00:16:21, acolwell wrote:
> move this into cc file too.

Done.

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:43: // padded.  Using av_malloc with padding
ensures FFmpeg only receives data
On 2012/09/20 00:16:21, acolwell wrote:
> Comment should be updated. We don't use av_malloc anymore right?

Done.

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:45: COMPILE_ASSERT(DecoderBuffer::kPaddingSize >=
FF_INPUT_BUFFER_PADDING_SIZE,
On 2012/09/19 23:59:18, ddorwin wrote:
> Shouldn't the new constants be multiples (1 or greater) of the FFmpeg values?
> 
> Examples:
> Is it okay if kPaddingSize is 17?
This should be okay. (+dale to be sure.)

> Is it not okay if kAlignmentSize is 64?
This should also be okay. Done.

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:45: COMPILE_ASSERT(DecoderBuffer::kPaddingSize >=
FF_INPUT_BUFFER_PADDING_SIZE,
On 2012/09/20 00:00:04, DaleCurtis wrote:
> Do this in the cc file instead.

Done.

http://codereview.chromium.org/10949029/diff/1/media/ffmpeg/ffmpeg_common.h#n...
media/ffmpeg/ffmpeg_common.h:48: COMPILE_ASSERT(DecoderBuffer::kAlignmentSize ==
32,
On 2012/09/20 00:00:04, DaleCurtis wrote:
> >= 32?

The alignment should be positive multiple of 32. E.g. 33 won't work.

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

http://codereview.chromium.org/10949029/diff/7001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:23: static const int
kFFmpegInputBufferAlignmentSize = 16;
Not using enum here to avoid comparing two enums.

[acolwell GONE FROM CHROMIUM, 2012-09-20 01:48:21]

LGTM % comment

http://codereview.chromium.org/10949029/diff/7001/media/base/decoder_buffer_u...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10949029/diff/7001/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:65: buffer2->GetData()) &
(DecoderBuffer::kPaddingSize - 1));
I think this is supposed to be kAlignmentSize instead

[xhwang, 2012-09-20 03:23:01]

http://codereview.chromium.org/10949029/diff/7001/media/base/decoder_buffer_u...
File media/base/decoder_buffer_unittest.cc (right):

http://codereview.chromium.org/10949029/diff/7001/media/base/decoder_buffer_u...
media/base/decoder_buffer_unittest.cc:65: buffer2->GetData()) &
(DecoderBuffer::kPaddingSize - 1));
On 2012/09/20 01:48:21, acolwell wrote:
> I think this is supposed to be kAlignmentSize instead

Done.

[DaleCurtis, 2012-09-20 03:36:13]

lgtm

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer....
media/base/decoder_buffer.cc:8: #if !defined(OS_ANDROID)
platform specific headers go under everything else, separated by a blank line.

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer....
media/base/decoder_buffer.h:26: #if defined(ARCH_CPU_ARM_FAMILY)
Probably need to include "build/build_config.h" for this.

[xhwang, 2012-09-20 04:16:25]

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer....
media/base/decoder_buffer.cc:8: #if !defined(OS_ANDROID)
On 2012/09/20 03:36:13, DaleCurtis wrote:
> platform specific headers go under everything else, separated by a blank line.

Done.

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

http://codereview.chromium.org/10949029/diff/12002/media/base/decoder_buffer....
media/base/decoder_buffer.h:26: #if defined(ARCH_CPU_ARM_FAMILY)
On 2012/09/20 03:36:13, DaleCurtis wrote:
> Probably need to include "build/build_config.h" for this.

Done.

[ddorwin, 2012-09-20 04:19:17]

lgtm

[commit-bot: I haz the power, 2012-09-20 16:28:19]

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

[commit-bot: I haz the power, 2012-09-20 20:56:55]

Change committed as 157836