Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11308310:
Replace av_malloc with AlignedAlloc for memory allocation in VideoFrame. (Closed)
|
Created:
8 years ago by xhwang Modified:
8 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace av_malloc with AlignedAlloc for memory allocation in VideoFrame.
See also r157836.
BUG=150920
TEST=media_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171976
Patch Set 1 #
Total comments: 7
Patch Set 2 : remove #ifdef #
Total comments: 7
Patch Set 3 : #
Total comments: 5
Patch Set 4 : nits #
Messages
Total messages: 18 (0 generated)
This CL is not finished. I have a few questions to ask. PTAL! https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (left): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc#old... media/base/video_frame.cc:207: // TODO(dalecurtis): use DataAligned or so, so this #ifdef hackery I wonder how we can remove #ifdef if for Android we don't use aligned alloc but otherwise use aligned alloc? Maybe use alligned alloc on all platforms? https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc#new... media/base/video_frame.cc:169: size_t aligned_height = RoundUp(coded_size_.height(), kFrameSizeAlignment); Why we always need to align height? https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.h#newc... media/base/video_frame.h:23: kAlignmentSize = 32 This is copied from DecoderBuffer. For VideoFrame, what exactly is the requirement for alignment? How about just use 32 bytes alignment for all VideoFrames?
(replacing myself w/ rbultje -- he can likely answer your questions better than myself!)
https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (left): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc#old... media/base/video_frame.cc:207: // TODO(dalecurtis): use DataAligned or so, so this #ifdef hackery On 2012/12/02 02:13:13, xhwang wrote: > I wonder how we can remove #ifdef if for Android we don't use aligned alloc but > otherwise use aligned alloc? Maybe use alligned alloc on all platforms? Android doesn't use FFmpeg, so we can't use av_malloc(). If there were a generic AlignedMalloc class, we could use it. So it's not about aligned alloc, but rather about the FFmpeg dependency. https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc#new... media/base/video_frame.cc:169: size_t aligned_height = RoundUp(coded_size_.height(), kFrameSizeAlignment); On 2012/12/02 02:13:13, xhwang wrote: > Why we always need to align height? Codecs work in complete 16x16 macroblocks, and decode as such. So a video of size, say, 2x2 pixels, contains 1 macroblock of 16x16 pixels, so we _need_ to have a buffer holding at least 16x16 pixels, else we will cause overwrites.
Thanks for the comments. Updated, PTAL again! https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (left): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc#old... media/base/video_frame.cc:207: // TODO(dalecurtis): use DataAligned or so, so this #ifdef hackery On 2012/12/03 19:39:35, rbultje1 wrote: > On 2012/12/02 02:13:13, xhwang wrote: > > I wonder how we can remove #ifdef if for Android we don't use aligned alloc > but > > otherwise use aligned alloc? Maybe use alligned alloc on all platforms? > > Android doesn't use FFmpeg, so we can't use av_malloc(). If there were a generic > AlignedMalloc class, we could use it. So it's not about aligned alloc, but > rather about the FFmpeg dependency. base::AlignedAlloc should be available on Android too. Removed #ifdef's. https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/1/media/base/video_frame.cc#new... media/base/video_frame.cc:169: size_t aligned_height = RoundUp(coded_size_.height(), kFrameSizeAlignment); On 2012/12/03 19:39:35, rbultje1 wrote: > On 2012/12/02 02:13:13, xhwang wrote: > > Why we always need to align height? > > Codecs work in complete 16x16 macroblocks, and decode as such. So a video of > size, say, 2x2 pixels, contains 1 macroblock of 16x16 pixels, so we _need_ to > have a buffer holding at least 16x16 pixels, else we will cause overwrites. I see, thanks.
lgtm
DaleCurtis: could you please do an OWNERS review?
https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:145: static const int kFrameAddressAlignment = 32; Hmm, I'm worried about the proliferation of these constants and FFmpeg updates. Would it be possible to use the constants from DecoderBuffer:: which are compile_assert checked against ffmpeg's? https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:148: static const int kFramePadBytes = 15; This should be 16 and just set to DecoderBuffer::kPaddingSize.
https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:145: static const int kFrameAddressAlignment = 32; On 2012/12/04 18:54:24, DaleCurtis wrote: > Hmm, I'm worried about the proliferation of these constants and FFmpeg updates. > Would it be possible to use the constants from DecoderBuffer:: which are > compile_assert checked against ffmpeg's? I am not sure if VideoFrame should depend on DecoderBuffer at all. How about adding a separate set of enums VideoFrame and doing compile_assert for them too? The reason I didn't do this in the first place is that I thought FF_INPUT_BUFFER_PADDING_SIZE is only for input (not output). Is there one for output too? The same question applies to kFFmpegInputBufferAlignmentSize, does the output share the same values as the input? https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:148: static const int kFramePadBytes = 15; On 2012/12/04 18:54:24, DaleCurtis wrote: > This should be 16 and just set to DecoderBuffer::kPaddingSize. Will do.
https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:145: static const int kFrameAddressAlignment = 32; On 2012/12/04 19:40:50, xhwang wrote: > On 2012/12/04 18:54:24, DaleCurtis wrote: > > Hmm, I'm worried about the proliferation of these constants and FFmpeg > updates. > > Would it be possible to use the constants from DecoderBuffer:: which are > > compile_assert checked against ffmpeg's? > > I am not sure if VideoFrame should depend on DecoderBuffer at all. How about > adding a separate set of enums VideoFrame and doing compile_assert for them too? > > The reason I didn't do this in the first place is that I thought > FF_INPUT_BUFFER_PADDING_SIZE is only for input (not output). Is there one for > output too? The same question applies to kFFmpegInputBufferAlignmentSize, does > the output share the same values as the input? Can we add a header file with the constants and use that header here and in decoderbuffer? The padding is traditionally for INPUT yes, but it seems odd to have two padding constants with different values.
https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:145: static const int kFrameAddressAlignment = 32; On 2012/12/04 19:40:50, xhwang wrote: > On 2012/12/04 18:54:24, DaleCurtis wrote: > > Hmm, I'm worried about the proliferation of these constants and FFmpeg > updates. > > Would it be possible to use the constants from DecoderBuffer:: which are > > compile_assert checked against ffmpeg's? > > I am not sure if VideoFrame should depend on DecoderBuffer at all. How about > adding a separate set of enums VideoFrame and doing compile_assert for them too? > > The reason I didn't do this in the first place is that I thought > FF_INPUT_BUFFER_PADDING_SIZE is only for input (not output). Is there one for > output too? The same question applies to kFFmpegInputBufferAlignmentSize, does > the output share the same values as the input? FFmpeg has no constants for output padding, it is simply "15" (see utils.c). Alignment constant can be shared, it is always 32.
https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/6001/media/base/video_frame.cc#... media/base/video_frame.cc:145: static const int kFrameAddressAlignment = 32; On 2012/12/04 19:46:53, DaleCurtis wrote: > On 2012/12/04 19:40:50, xhwang wrote: > > On 2012/12/04 18:54:24, DaleCurtis wrote: > > > Hmm, I'm worried about the proliferation of these constants and FFmpeg > > updates. > > > Would it be possible to use the constants from DecoderBuffer:: which are > > > compile_assert checked against ffmpeg's? > > > > I am not sure if VideoFrame should depend on DecoderBuffer at all. How about > > adding a separate set of enums VideoFrame and doing compile_assert for them > too? > > > > The reason I didn't do this in the first place is that I thought > > FF_INPUT_BUFFER_PADDING_SIZE is only for input (not output). Is there one for > > output too? The same question applies to kFFmpegInputBufferAlignmentSize, does > > the output share the same values as the input? > > Can we add a header file with the constants and use that header here and in > decoderbuffer? > > The padding is traditionally for INPUT yes, but it seems odd to have two padding > constants with different values. Hmm, I feel we should keep input and output buffer alignment/padding constants separate. For example, what if later some codec needs to add some special alignment/padding requirement on the input and/or output buffer? If we do this, it's probably a better idea to keep those constants close to where it's used, i.e. in decoder_buffer.h and video_frame.h separately. I am worrying about this also because fbarchard@ proposed some other requirements (http://code.google.com/p/chromium/issues/detail?id=150920#c2; not included in this CL though) that we may need to implement in the future. WDYT?
lgtm then.
Made a few more changes. PTAL again!
lgtm % nit. https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.cc... media/base/video_frame.cc:146: DCHECK(data); This doesn't really make sense. You DCHECK() and then if(). free(NULL) is a noop, so I'd remove either the if or the dcheck or both. https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.h#... media/base/video_frame.h:20: kFrameSizePadding = 16, I'm assuming there's nothing wrong with making this 16 vs 15, Ronald please chime in otherwise.
https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.cc... media/base/video_frame.cc:146: DCHECK(data); On 2012/12/07 19:11:15, DaleCurtis wrote: > This doesn't really make sense. You DCHECK() and then if(). free(NULL) is a > noop, so I'd remove either the if or the dcheck or both. Done. https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.h#... media/base/video_frame.h:20: kFrameSizePadding = 16, On 2012/12/07 19:11:15, DaleCurtis wrote: > I'm assuming there's nothing wrong with making this 16 vs 15, Ronald please > chime in otherwise. In ffmpeg code this is actually 16 (See video_get_buffer() in libavcodec/utils.c). I assume the larger the safer ;)
https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/11308310/diff/14001/media/base/video_frame.h#... media/base/video_frame.h:20: kFrameSizePadding = 16, On 2012/12/07 20:25:33, xhwang wrote: > On 2012/12/07 19:11:15, DaleCurtis wrote: > > I'm assuming there's nothing wrong with making this 16 vs 15, Ronald please > > chime in otherwise. > > In ffmpeg code this is actually 16 (See video_get_buffer() in > libavcodec/utils.c). I assume the larger the safer ;) Right, 15 or 16 will make no practical difference.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11308310/20002
Message was sent while issue was closed.
Change committed as 171976 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
