Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10451051:
Provide a Chrome-owned buffer to FFmpeg for video decoding, instead of (Closed)
|
Created:
8 years, 7 months ago by rbultje1 Modified:
8 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionProvide a Chrome-owned buffer to FFmpeg for video decoding, instead of
letting it allocate a buffer internally and then copy it to our own
buffers after decoding finishes. This saves one memcpy() per decoded
video frame.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143115
Patch Set 1 #
Total comments: 25
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 24
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #
Total comments: 14
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #
Total comments: 9
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 49 (0 generated)
Using posix_memalign() is likely wrong, but I couldn't find the right way to allocate an aligned buffer in Chromium and Andrew didn't respond to my query fast enough. Also, it's probably obvious I don't know much c++ yet. Willing to learn. ;-).
Thanks for sending this out. Have you seen any performance effect of making this change? (CPU consumption / power / user-visible something?) https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:136: extern "C" { this should not be necessary https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:137: #include <stdlib.h> this belongs at the top of the file https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:140: int VideoFrame::AllocateAlignedWithStrides(int strides[], int aligned_h) Welcome to C++! We've got bools! https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:144: uint8_t *data; unnecessary; just use data_[VideoFrame::kRGBPlane] in the memalign call and make things clearer. https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, strides[0] * aligned_h)) { 32 is magic here and elsewhere. Name-constant (enum) it? https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, strides[0] * aligned_h)) { Does this even compile on windows? https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:149: } else if (format_ == VideoFrame::YV12 || format_ == VideoFrame::YV16) { Other than the memalign's this function is a dup of AllocateYUV / AllocateRGB above. Hopefully you can drop one of the copies (and assume always-aligned in the non-NativeTexture case). https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:157: if (posix_memalign((void **) &data, 32, y_bytes + (uv_bytes * 2) + kFramePadBytes)) { 80-col limit, here and elsewhere https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.h:127: int AllocateAlignedWithStrides(int strides[], int aligned_h); This change (ctor public, add public allocate method) seems strange. Why wouldn't you bury the alignment details inside the CreateFrame public entry point? For that matter, what code path wants to allocate *unaligned* memory? I suspect you can drop all changes to this file from this CL (restricting your change to the .cc file). https://chromiumcodereview.appspot.com/10451051/diff/1/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.h (right): https://chromiumcodereview.appspot.com/10451051/diff/1/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.h:31: #include <libavutil/imgutils.h> why? https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:62: int FFmpegVideoDecoder::AllocBuffer(VideoFrame **ptr) no doco, no unittest, single call-site ==> inline it into the caller. https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:91: buf->AddRef(); AddRef() is almost never the right thing to call. AllocBuffer should store the result of its "new VideoFrame" in a scoped_refptr<VideoFrame>, and return that type. When you need to pass it to C, call .release() on the scoped_refptr. https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:92: frame->opaque = buf; This sort of ascii-art alignment isn't adding anything, IMO. https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:99: for (int i = 0; i < (buf->format() == media::VideoFrame::RGB32 ? 1 : 3); i++) { I think you're assuming either RGB32 or YV12/YV16. In that case precede this for loop with DCHECK(buf->format() == VideoFrame::YV12 || buf->format() == VideoFrame::YV16 || buf->format() == VideoFrame::RGB32) << buf->format(); https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:100: frame->base[i] = Un-style-ish. Repeat buf->data(i) https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:108: static int c_GetVideoBuffer(AVCodecContext *s, AVFrame *frame) the "c_" prefix is un-style-ish. https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:109: { opening braces go on previous line (here and elsewhere) https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:110: FFmpegVideoDecoder *vd = (FFmpegVideoDecoder *) s->opaque; This isn't used until l.114, so it can be done there. https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:110: FFmpegVideoDecoder *vd = (FFmpegVideoDecoder *) s->opaque; use static_cast<>() instead of c-style casts (here and elsewhere) https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:111: if (!(s->flags & CODEC_FLAG_EMU_EDGE)) { When will this be true? (can you drop the "then" clause? ditto for l.121 below). https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:125: memset(frame->data, 0, sizeof(frame->data)); why? (also, do you really mean to only set the first 4/8 bytes to 0?) https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:175: codec_context_->get_buffer = c_GetVideoBuffer; Depending on ffmpeg dispatches these callbacks, it may be possible to assign to them the result of a base::Bind() which would allow you to drop the file-static trampolines. https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.cc:431: if (!(codec_context_->flags & CODEC_FLAG_EMU_EDGE)) { Drop this test here and elsewhere? https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... File media/filters/ffmpeg_video_decoder.h (right): https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... media/filters/ffmpeg_video_decoder.h:37: // called from within ffmpeg Comments need to be complete English sentences, starting w/ capital, ending with period. More importantly, this should document what the return value means, and what the method does. I suspect a pointer to the interface this satisfies is in order.
On 2012/05/26 22:52:34, Ami Fischman wrote: > Thanks for sending this out. > > Have you seen any performance effect of making this change? > (CPU consumption / power / user-visible something?) Will measure this tuesday. I expect a 3% decrease in CPU usage overall. After this, there's 3 more heavy users of memcpy() during HD playback that I'm looking into.
https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, strides[0] * aligned_h)) { On 2012/05/26 22:52:34, Ami Fischman wrote: > Does this even compile on windows? I don't think this works on other platforms; for reference see all the memalign business in av_malloc (mem.c) for different platforms. We even force "hacked-memalign" on OSX. Instead of backing this with uint8 above, we can just use the DecoderBuffer class I'm introducing here: http://codereview.chromium.org/10447035/ Then you don't need to worry about alignment, etc.
waiting for addressing fischman's comments before digging in
On 2012/05/29 17:33:19, DaleCurtis wrote: > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, > strides[0] * aligned_h)) { > On 2012/05/26 22:52:34, Ami Fischman wrote: > > Does this even compile on windows? > > I don't think this works on other platforms; for reference see all the memalign > business in av_malloc (mem.c) for different platforms. We even force > "hacked-memalign" on OSX. > > Instead of backing this with uint8 above, we can just use the DecoderBuffer > class I'm introducing here: > > http://codereview.chromium.org/10447035/ > > Then you don't need to worry about alignment, etc. Now that this has landed... DecoderBuffer is for compressed bitstream data (e.g. from protocol to demuxer, or demuxer to decoder), not for raw image data (decoder output), isn't it? I can presumably just do the same you're doing there, i.e. use av_malloc() except on Android.
On 2012/06/07 13:31:01, rbultje wrote: > On 2012/05/29 17:33:19, DaleCurtis wrote: > > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.cc > > File media/base/video_frame.cc (right): > > > > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > > media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, > > strides[0] * aligned_h)) { > > On 2012/05/26 22:52:34, Ami Fischman wrote: > > > Does this even compile on windows? > > > > I don't think this works on other platforms; for reference see all the > memalign > > business in av_malloc (mem.c) for different platforms. We even force > > "hacked-memalign" on OSX. > > > > Instead of backing this with uint8 above, we can just use the DecoderBuffer > > class I'm introducing here: > > > > http://codereview.chromium.org/10447035/ > > > > Then you don't need to worry about alignment, etc. > > Now that this has landed... DecoderBuffer is for compressed bitstream data (e.g. > from protocol to demuxer, or demuxer to decoder), not for raw image data > (decoder output), isn't it? I can presumably just do the same you're doing > there, i.e. use av_malloc() except on Android. Yeah if you follow the discussion we had in https://chromiumcodereview.appspot.com/10533004/ it looks like we need a simple wrapper for av_malloc/free that the rest of media code can use and gets #ifdef'd for android
On 2012/06/07 16:41:17, scherkus wrote: > On 2012/06/07 13:31:01, rbultje wrote: > > On 2012/05/29 17:33:19, DaleCurtis wrote: > > > > > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.cc > > > File media/base/video_frame.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > > > media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, > > > strides[0] * aligned_h)) { > > > On 2012/05/26 22:52:34, Ami Fischman wrote: > > > > Does this even compile on windows? > > > > > > I don't think this works on other platforms; for reference see all the > > memalign > > > business in av_malloc (mem.c) for different platforms. We even force > > > "hacked-memalign" on OSX. > > > > > > Instead of backing this with uint8 above, we can just use the DecoderBuffer > > > class I'm introducing here: > > > > > > http://codereview.chromium.org/10447035/ > > > > > > Then you don't need to worry about alignment, etc. > > > > Now that this has landed... DecoderBuffer is for compressed bitstream data > (e.g. > > from protocol to demuxer, or demuxer to decoder), not for raw image data > > (decoder output), isn't it? I can presumably just do the same you're doing > > there, i.e. use av_malloc() except on Android. > > Yeah if you follow the discussion we had in > https://chromiumcodereview.appspot.com/10533004/ it looks like we need a simple > wrapper for av_malloc/free that the rest of media code can use and gets #ifdef'd > for android Shall I mark that with a "TODO: dalecurtis refactor DecoderBuffer into AlignedData" as suggested in that thread? I do somehow feel that the "our classes multiply like rabbits as wrappers for one-line things" applies here, too...
On 2012/06/07 17:21:04, rbultje wrote: > On 2012/06/07 16:41:17, scherkus wrote: > > On 2012/06/07 13:31:01, rbultje wrote: > > > On 2012/05/29 17:33:19, DaleCurtis wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame.cc > > > > File media/base/video_frame.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > > > > media/base/video_frame.cc:145: if (posix_memalign((void **) &data, 32, > > > > strides[0] * aligned_h)) { > > > > On 2012/05/26 22:52:34, Ami Fischman wrote: > > > > > Does this even compile on windows? > > > > > > > > I don't think this works on other platforms; for reference see all the > > > memalign > > > > business in av_malloc (mem.c) for different platforms. We even force > > > > "hacked-memalign" on OSX. > > > > > > > > Instead of backing this with uint8 above, we can just use the > DecoderBuffer > > > > class I'm introducing here: > > > > > > > > http://codereview.chromium.org/10447035/ > > > > > > > > Then you don't need to worry about alignment, etc. > > > > > > Now that this has landed... DecoderBuffer is for compressed bitstream data > > (e.g. > > > from protocol to demuxer, or demuxer to decoder), not for raw image data > > > (decoder output), isn't it? I can presumably just do the same you're doing > > > there, i.e. use av_malloc() except on Android. > > > > Yeah if you follow the discussion we had in > > https://chromiumcodereview.appspot.com/10533004/ it looks like we need a > simple > > wrapper for av_malloc/free that the rest of media code can use and gets > #ifdef'd > > for android > > Shall I mark that with a "TODO: dalecurtis refactor DecoderBuffer into > AlignedData" as suggested in that thread? I do somehow feel that the "our > classes multiply like rabbits as wrappers for one-line things" applies here, > too... They're wrappers because not every port of Chromium includes ffmpeg meaning we isolate the spread of #ifdef code at the expense of wrapping the code. Anyway this isn't something that needs to be addressed immediately so TODO'ing or dup'ing the #ifdef code is fine for now.
On 2012/05/26 22:52:34, Ami Fischman wrote: > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > media/base/video_frame.cc:136: extern "C" { > this should not be necessary Removed (and switched to using av_malloc(), like DecoderBuffer). > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > media/base/video_frame.cc:149: } else if (format_ == VideoFrame::YV12 || format_ > == VideoFrame::YV16) { > Other than the memalign's this function is a dup of AllocateYUV / AllocateRGB > above. Hopefully you can drop one of the copies (and assume always-aligned in > the non-NativeTexture case). Indeed, and so several of the other comments I skipped because they no longer apply. There's now only one allocated (well, one for RGB and one for YUV, just like it was before my patch) and it's now always aligned, except on Android. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > media/base/video_frame.cc:157: if (posix_memalign((void **) &data, 32, y_bytes + > (uv_bytes * 2) + kFramePadBytes)) { > 80-col limit, here and elsewhere Fixed. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/base/video_frame... > media/base/video_frame.h:127: int AllocateAlignedWithStrides(int strides[], int > aligned_h); > This change (ctor public, add public allocate method) seems strange. > Why wouldn't you bury the alignment details inside the CreateFrame public entry > point? > For that matter, what code path wants to allocate *unaligned* memory? I suspect > you can drop all changes to this file from this CL (restricting your change to > the .cc file). So, I do need small changes; the problem is that FFmpeg has (in some decoders) particularly strict requirements on relationship between strides of different planes, so the caller needs to define the stride, not VideoFrame itself (since it doesn't know the AVCodecContext). Thus, I've added arguments for that, and allow the function to be called without them. I hope I did that in the right C++y way. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/ffmpeg/ffmpeg_co... > media/ffmpeg/ffmpeg_common.h:31: #include <libavutil/imgutils.h> > why? av_image_check_size() and av_image_fill_linesizes(). > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:62: int > FFmpegVideoDecoder::AllocBuffer(VideoFrame **ptr) > no doco, no unittest, single call-site ==> inline it into the caller. Done. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:91: buf->AddRef(); > AddRef() is almost never the right thing to call. > AllocBuffer should store the result of its "new VideoFrame" in a > scoped_refptr<VideoFrame>, and return that type. > > When you need to pass it to C, call .release() on the scoped_refptr. Changes as suggested. I think it works correctly, i.e. memory usage stays stable during playback. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:92: frame->opaque = buf; > This sort of ascii-art alignment isn't adding anything, IMO. Removed. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:99: for (int i = 0; i < (buf->format() == > media::VideoFrame::RGB32 ? 1 : 3); i++) { > I think you're assuming either RGB32 or YV12/YV16. In that case precede this > for loop with > DCHECK(buf->format() == VideoFrame::YV12 || buf->format() == VideoFrame::YV16 || > buf->format() == VideoFrame::RGB32) << buf->format(); Done. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:100: frame->base[i] = > Un-style-ish. Repeat buf->data(i) Done. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:108: static int > c_GetVideoBuffer(AVCodecContext *s, AVFrame *frame) > the "c_" prefix is un-style-ish. Oh I think I forgot to do this let me do this right after sending this email. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:109: { > opening braces go on previous line (here and elsewhere) Fixed. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:110: FFmpegVideoDecoder *vd = > (FFmpegVideoDecoder *) s->opaque; > use static_cast<>() instead of c-style casts > (here and elsewhere) Fixed. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:111: if (!(s->flags & > CODEC_FLAG_EMU_EDGE)) { > When will this be true? > (can you drop the "then" clause? ditto for l.121 below). Yes, fixed. A future decoder could potentially enforce not using this flag (VP8 used to do that), but that's not the case right now. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:125: memset(frame->data, 0, > sizeof(frame->data)); > why? (also, do you really mean to only set the first 4/8 bytes to 0?) FFmpeg requires that release_buffer() sets data pointers to zero. It's an API thing, I don't zero the data, I zero the pointers. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:175: codec_context_->get_buffer = > c_GetVideoBuffer; > Depending on ffmpeg dispatches these callbacks, it may be possible to assign to > them the result of a base::Bind() which would allow you to drop the file-static > trampolines. base::bind() doesn't appear to work, I still need some way to trampoline back into the class so I can access codec_context_. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.cc:431: if (!(codec_context_->flags & > CODEC_FLAG_EMU_EDGE)) { > Drop this test here and elsewhere? Done. > https://chromiumcodereview.appspot.com/10451051/diff/1/media/filters/ffmpeg_v... > media/filters/ffmpeg_video_decoder.h:37: // called from within ffmpeg > Comments need to be complete English sentences, starting w/ capital, ending with > period. > More importantly, this should document what the return value means, and what the > method does. > I suspect a pointer to the interface this satisfies is in order. Done.
Holding off the rest of the review given my question below. http://codereview.chromium.org/10451051/diff/10001/media/base/video_frame.h File media/base/video_frame.h (right): http://codereview.chromium.org/10451051/diff/10001/media/base/video_frame.h#n... media/base/video_frame.h:49: size_t bytes_per_uv_row, These two args don't make sense to me, because they overlap semantically with the information content of width+format. Can this be reduced to an alignment/padding requirement instead? I think that would make for a much saner API.
On 2012/06/08 19:11:04, Ami Fischman wrote: > Holding off the rest of the review given my question below. > > http://codereview.chromium.org/10451051/diff/10001/media/base/video_frame.h > File media/base/video_frame.h (right): > > http://codereview.chromium.org/10451051/diff/10001/media/base/video_frame.h#n... > media/base/video_frame.h:49: size_t bytes_per_uv_row, > These two args don't make sense to me, because they overlap semantically with > the information content of width+format. > > Can this be reduced to an alignment/padding requirement instead? I think that > would make for a much saner API. It's a good point, I did spend considerable ... time ... doing absolutely nothing while going over that particular code. I kept changing it and not being happy with it, then changing it to something else and back and so on and I'm still unhappy with it. But ... The main problem is that FFmpeg "dictates" the strides to us. This is essentially a theoretical concern, but the API docs say that we have to conform to this and that not only in theory, but actually in practice, the stride cannot trivially be calculated from the width of a single plane plus some alignment. For example, the comments say that the MPEG-1, 2 or 4 decoder (as used by CrOS) uses y_stride>>1 to get uv_stride, so not only do they both need to be 16-byte aligned for SIMD (soon to be 32), but also do they need to relate to each other, even when that means wasting another extra 16 bytes on y_stride (which is thus not 16-, but 32-byte aligned). So to fit in with the FFmpeg API and limitations, I chose this API. A simple alignment/padding setting is unfortunately not enough to give FFmpeg what it needs in all cases, according to the API docs and comments...
Can you send pointers to the ffmpeg api docs and comments in question?
On 2012/06/08 19:36:30, Ami Fischman wrote: > Can you send pointers to the ffmpeg api docs and comments in question? http://git.libav.org/?p=libav.git;a=blob;f=libavcodec/utils.c;h=d2ee9f893b5d5... " 423 // NOTE: do not align linesizes individually, this breaks e.g. assumptions 424 // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2" I misremember, it's encoder, not decoder. I'm not sure if any decoders have such assumptions... If not, maybe I should redo it and make it simpler. Is a simple alignment_pow2 (which would typically be 4 for 16, 5 for 32 or 0 for I don't care) OK from an API point of view?
I think so. Esp. if it can be the same for y and uv.
On 2012/06/08 19:56:39, Ami Fischman wrote: > I think so. Esp. if it can be the same for y and uv. Depends whether we care about memory usage or speed. We can make it 16 for Y and 8 for UV which works for all decoders right now, but has the downside that then the UV are not 16-byte aligned and thus things like software rescaling may be slower. Or we can make both 16 in which case we use slightly more mem but sw rescaling would be faster. Either way it doesn't make much of a difference and is simpler, so I'll revise the patch like that soon.
http://codereview.chromium.org/10451051/diff/10001/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/10451051/diff/10001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder.cc:157: codec_context_->get_buffer = callbackGetVideoBuffer; I still think it should be possible to use base::Bind() to avoid the explicit trampoline static function. Bind()'ing to a member function of |*this| is the most common use of Bind().
On 2012/06/08 19:11:04, Ami Fischman wrote: > Holding off the rest of the review given my question below. > > http://codereview.chromium.org/10451051/diff/10001/media/base/video_frame.h > File media/base/video_frame.h (right): > > http://codereview.chromium.org/10451051/diff/10001/media/base/video_frame.h#n... > media/base/video_frame.h:49: size_t bytes_per_uv_row, > These two args don't make sense to me, because they overlap semantically with > the information content of width+format. > > Can this be reduced to an alignment/padding requirement instead? I think that > would make for a much saner API. Done as suggested.
On 2012/06/09 02:51:23, Ami Fischman wrote: > http://codereview.chromium.org/10451051/diff/10001/media/filters/ffmpeg_video... > File media/filters/ffmpeg_video_decoder.cc (right): > > http://codereview.chromium.org/10451051/diff/10001/media/filters/ffmpeg_video... > media/filters/ffmpeg_video_decoder.cc:157: codec_context_->get_buffer = > callbackGetVideoBuffer; > I still think it should be possible to use base::Bind() to avoid the explicit > trampoline static function. > Bind()'ing to a member function of |*this| is the most common use of Bind(). So this doesn't work because it's a C pointer. I got as far as creating a c++ function with the prototype int FFmpegVideoDecoder::GetVideoBuffer(AVCodecContext *ctx, AVFrame *f); and binding it using base::Bind(&class::func, this); and then assigning the callback.Run to the function pointer in the AVCodecContext, but that fails to compile with: ../../media/filters/ffmpeg_video_decoder.cc:150:32: error: cannot create a non-constant pointer to member function codec_context_->get_buffer = &GetBufferCb.Run; Ami told me that's hard to overcome and so it might be easier to keep it as is for now (at least until we figure out how this can be worked around). So this issue is not done.
nice! mostly style nits https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:30: frame->AllocateRGB(4u, alignment); fix indent here https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:34: frame->AllocateYUV(alignment); fix indent here https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:52: width * height <= limits::kMaxCanvas && alignment > 0); shouldn't alignment be checked for power of 2? check out the neat bitwise trick in RoundUp() https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:86: VideoFrame::CreateFrame(VideoFrame::YV12, width, height, revert change here https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:102: static inline size_t Max(size_t a, size_t b) { use std::max() instead? https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:113: // TODO use DataAligned or so, so this #ifdef hackery doesn't need to be s/TODO/TODO(dalecurtis):/ https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:115: data_[VideoFrame::kRGBPlane] = reinterpret_cast<uint8 *>( pointers go w/ types good: uint8* var bad: uint8 *var https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:116: av_malloc(bytes_per_row * aligned_height)); 4-space indent https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:136: // a full two rows of Y. comment needs updating to say this does a minimum of 4-byte alignment, etc... https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:147: // TODO use DataAligned or so, so this #ifdef hackery doesn't need to be ditto https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.cc:149: uint8* data = reinterpret_cast<uint8 *>( ditto for pointers https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.h:63: static bool IsValidConfig( I actually don't see any public users of this function! what happens if you get rid of the one w/o alignment? https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:64: int FFmpegVideoDecoder::GetVideoBuffer(AVFrame *frame) { pointer w/ type https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:70: int w = codec_context_->width, h = codec_context_->height, ret; split each initializer onto each line s/w/width/ s/h/height/ https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:74: scoped_refptr<VideoFrame> buf = VideoFrame::CreateFrame(format, drop this to next line + 4-space indent https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:74: scoped_refptr<VideoFrame> buf = VideoFrame::CreateFrame(format, nit: s/buf/video_frame/ rest of this file uses said convention https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:85: frame->pkt_pts = codec_context_->pkt ? codec_context_->pkt->pts : AV_NOPTS_VALUE; fix >80 chars https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:93: static int callbackGetVideoBuffer(AVCodecContext *s, AVFrame *frame) { pointer w/ type here + below https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:93: static int callbackGetVideoBuffer(AVCodecContext *s, AVFrame *frame) { we don't do camelCase code around here but AllCaps! I'd drop the "callback" prefix anyway as the name isn't clashing w/ anything same w/ ReleaseVideoBuffer below https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:98: static void callbackReleaseVideoBuffer(AVCodecContext *s, AVFrame *frame) { pointer w/ type here + below https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:101: // intentional. Would be nice if scoped_refptr::adopt existed. I'd drop the "Would be nice.." part -- it ain't gonna happen anytime soon :) https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:102: scoped_refptr<VideoFrame> buf = static_cast<VideoFrame *>(frame->opaque); s/buf/video_frame https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... File media/filters/ffmpeg_video_decoder.h (right): https://chromiumcodereview.appspot.com/10451051/diff/18001/media/filters/ffmp... media/filters/ffmpeg_video_decoder.h:43: int GetVideoBuffer(AVFrame *frame); pointer w/ type
I think I've made all changes as suggested, except 1, see below. https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/10451051/diff/18001/media/base/video_f... media/base/video_frame.h:63: static bool IsValidConfig( On 2012/06/14 02:21:33, scherkus wrote: > I actually don't see any public users of this function! > > what happens if you get rid of the one w/o alignment? See video_decoder_config.cc
https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.cc:119: av_malloc(bytes_per_row * aligned_height)); sorry -- "4 space indent" meant from the existing indentation for a total indent of 6 spaces https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.cc:144: size_t y_height = RoundUp(height_, std::max(alignment, does the height need to be an even number or rounded up? for example you pass in 16 here -- does that mean a height of 19 would be 32 instead of 20? https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.cc:154: av_malloc(y_bytes + (uv_bytes * 2) + kFramePadBytes)); indent here https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.h:57: return CreateFrame(format, width, height, 1, nit: this can fit on one line https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.h:63: static bool IsValidConfig( hrm this was added in r122886 and seems to be used by VideoDecoderConfig out of convenience -- can you add a "TODO(scherkus): VideoDecoderConfig shouldn't call this method" https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... media/filters/ffmpeg_video_decoder.cc:77: VideoFrame::CreateFrame(format, width, height, 16u, fix indent (4 spaces from current indent level = 6 spaces total) https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... media/filters/ffmpeg_video_decoder.cc:102: static void ReleaseVideoBuffer(AVCodecContext* s, AVFrame* frame) { nit: append Impl to fn name for parity https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... media/filters/ffmpeg_video_decoder.cc:107: static_cast<VideoFrame*>(frame->opaque); fix indent here (4 spaces from current indent level = 6 spaceS)
https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.cc:119: av_malloc(bytes_per_row * aligned_height)); On 2012/06/14 19:21:20, scherkus wrote: > sorry -- "4 space indent" meant from the existing indentation for a total indent > of 6 spaces Done. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.cc:144: size_t y_height = RoundUp(height_, std::max(alignment, On 2012/06/14 19:21:20, scherkus wrote: > does the height need to be an even number or rounded up? > > for example you pass in 16 here -- does that mean a height of 19 would be 32 > instead of 20? FFmpeg requires a multiple of 16, since each is decoded as a complete 16x16 macroblock. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.cc:154: av_malloc(y_bytes + (uv_bytes * 2) + kFramePadBytes)); On 2012/06/14 19:21:20, scherkus wrote: > indent here Done. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.h:57: return CreateFrame(format, width, height, 1, On 2012/06/14 19:21:20, scherkus wrote: > nit: this can fit on one line Done. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/base/video_fr... media/base/video_frame.h:63: static bool IsValidConfig( On 2012/06/14 19:21:20, scherkus wrote: > hrm this was added in r122886 and seems to be used by VideoDecoderConfig out of > convenience -- can you add a "TODO(scherkus): VideoDecoderConfig shouldn't call > this method" Done. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... media/filters/ffmpeg_video_decoder.cc:77: VideoFrame::CreateFrame(format, width, height, 16u, On 2012/06/14 19:21:20, scherkus wrote: > fix indent (4 spaces from current indent level = 6 spaces total) Done. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... media/filters/ffmpeg_video_decoder.cc:102: static void ReleaseVideoBuffer(AVCodecContext* s, AVFrame* frame) { On 2012/06/14 19:21:20, scherkus wrote: > nit: append Impl to fn name for parity Done. https://chromiumcodereview.appspot.com/10451051/diff/7010/media/filters/ffmpe... media/filters/ffmpeg_video_decoder.cc:107: static_cast<VideoFrame*>(frame->opaque); On 2012/06/14 19:21:20, scherkus wrote: > fix indent here (4 spaces from current indent level = 6 spaceS) Done.
Lgtm! Dale/Ami do you have any additional feedback?
lgtm % nits & qs https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.cc:24: size_t alignment, Do you think making this configurable is worth it? Why not just fix alignment at 16 or 32 bytes via an internal VideoFrame::kFrameAlignment constant and call it a day? At worst we'd be allocating (alignment-1)*3 extra bytes == 45 bytes or 93 bytes respectively. https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:77: VideoFrame::CreateFrame(format, width, height, 16u, Will this need to change to 32-bytes with AVX instructions? https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:80: for (int i = 0; i < 3; i++) { s/3/VideoFrame::kMaxPlanes/ ? https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:88: frame->pkt_pts = codec_context_->pkt ? codec_context_->pkt->pts : Alignment seems funky, consider just using a 4-sp indent on the next line. Up to you.
https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:77: VideoFrame::CreateFrame(format, width, height, 16u, On 2012/06/14 23:35:37, DaleCurtis wrote: > Will this need to change to 32-bytes with AVX instructions? Yes and no. So the alignment here serves two purposes: consider something like VP8 or H264, where the video is encoded in 16x16 macroblocks. If the video has a non-16x16 size, we need to round it up to 16x16 sizes for video coding purposes, and ignore the data outside the visible area. More concretely, if the video has a size of 1x1 pixels, then we still decode 1x1 macroblocks (16x16 pixels) and simply don't display everything outside the initial 1x1 pixels. But the extra data needs to be allocated and is written to. In addition to that, there's alignment and so on, which is only in horizontal dimensions. That's currently also 16 (for SSE2), which is very convenient. For AVX2, that will change to 32 which will make all of this a massive pain, since then vertical alignment needs to be 16 but horizontal 32. Let's ignore that until it comes up.
https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.cc:24: size_t alignment, On 2012/06/14 23:35:37, DaleCurtis wrote: > Do you think making this configurable is worth it? Why not just fix alignment at > 16 or 32 bytes via an internal VideoFrame::kFrameAlignment constant and call it > a day? At worst we'd be allocating (alignment-1)*3 extra bytes == 45 bytes or 93 > bytes respectively. That's horizontal, for vertical you're allocating a lot more. Also, it's sort of an implementation detail, when formats like HEVC/VP8+1 or optimizations like AVX2 come out, this will change to 32. That's an FFmpeg implementation detail, so having it configurable might make that a little more clear. But up yo you, if you prefer (Ami also brought this up earlier, did he?), I can simply make this static and always align by 16 in both dimensions.
https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.cc:24: size_t alignment, On 2012/06/14 23:35:37, DaleCurtis wrote: > Do you think making this configurable is worth it? Why not just fix alignment at > 16 or 32 bytes via an internal VideoFrame::kFrameAlignment constant and call it > a day? At worst we'd be allocating (alignment-1)*3 extra bytes == 45 bytes or 93 > bytes respectively. Don't forget it's per-line but agreed still nothing worth sweating about especially if in the end we eliminate ~3 VideoFrame allocations (for a 720p video that's ~4MB!) I'd be fine making 16-bytes the default w/ a constant and all that jazz.
I think Dale & Andrew have this review well in hand so removing myself from reviewers list :) https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.cc:24: size_t alignment, On 2012/06/14 23:47:50, scherkus wrote: > On 2012/06/14 23:35:37, DaleCurtis wrote: > > Do you think making this configurable is worth it? Why not just fix alignment > at > > 16 or 32 bytes via an internal VideoFrame::kFrameAlignment constant and call > it > > a day? At worst we'd be allocating (alignment-1)*3 extra bytes == 45 bytes or > 93 > > bytes respectively. > > Don't forget it's per-line but agreed still nothing worth sweating about > especially if in the end we eliminate ~3 VideoFrame allocations (for a 720p > video that's ~4MB!) > > I'd be fine making 16-bytes the default w/ a constant and all that jazz. Yep - agreed. Burn the bytes and simplify the API. https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.cc:44: static inline bool IsPowerOfTwo(int alignment) { I thought static & inline were incompatible for at least some of our compilers. Just sayin'. https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.h:51: static scoped_refptr<VideoFrame> CreateFrame( Chromium (and google) style guides prohibit function overloading like this. You really do have to go around and update all callers (which shouldn't be very many at all for this and the next functions). https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:103: // We're releasing the refenence to the buffer allocated in typo https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:109: memset(frame->data, 0, sizeof(frame->data)); unnecessary? (only do #if !NDEBUG ?)
https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... media/base/video_frame.cc:44: static inline bool IsPowerOfTwo(int alignment) { On 2012/06/15 06:53:30, Ami Fischman wrote: > I thought static & inline were incompatible for at least some of our compilers. > Just sayin'. RoundUp uses it also, so should be OK. extern inline is probably what you're referring to.
https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... File media/filters/ffmpeg_video_decoder.cc (right): https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... media/filters/ffmpeg_video_decoder.cc:109: memset(frame->data, 0, sizeof(frame->data)); On 2012/06/15 06:53:30, Ami Fischman wrote: > unnecessary? (only do #if !NDEBUG ?) We discussed this earlier, it's a FFmpeg API requirement in the callback. I'll add a comment in the next revision.
On 2012/06/15 19:10:27, rbultje1 wrote: > https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... > File media/filters/ffmpeg_video_decoder.cc (right): > > https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... > media/filters/ffmpeg_video_decoder.cc:109: memset(frame->data, 0, > sizeof(frame->data)); > On 2012/06/15 06:53:30, Ami Fischman wrote: > > unnecessary? (only do #if !NDEBUG ?) > > We discussed this earlier, it's a FFmpeg API requirement in the callback. I'll > add a comment in the next revision. Done.
On 2012/06/15 06:53:30, Ami Fischman wrote: > https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... > media/base/video_frame.h:51: static scoped_refptr<VideoFrame> CreateFrame( > Chromium (and google) style guides prohibit function overloading like this. You > really do have to go around and update all callers (which shouldn't be very many > at all for this and the next functions). Removed. > https://chromiumcodereview.appspot.com/10451051/diff/16003/media/filters/ffmp... > media/filters/ffmpeg_video_decoder.cc:103: // We're releasing the refenence to > the buffer allocated in > typo Fixed.
On 2012/06/14 23:47:50, scherkus wrote: > https://chromiumcodereview.appspot.com/10451051/diff/16003/media/base/video_f... > media/base/video_frame.cc:24: size_t alignment, > On 2012/06/14 23:35:37, DaleCurtis wrote: > > Do you think making this configurable is worth it? Why not just fix alignment > at > > 16 or 32 bytes via an internal VideoFrame::kFrameAlignment constant and call > it > > a day? At worst we'd be allocating (alignment-1)*3 extra bytes == 45 bytes or > 93 > > bytes respectively. > > Don't forget it's per-line but agreed still nothing worth sweating about > especially if in the end we eliminate ~3 VideoFrame allocations (for a 720p > video that's ~4MB!) > > I'd be fine making 16-bytes the default w/ a constant and all that jazz. OK, changed to 16 by default.
Will take a look tomorrow. http://codereview.chromium.org/10451051/diff/24002/media/base/video_frame.cc File media/base/video_frame.cc (right): http://codereview.chromium.org/10451051/diff/24002/media/base/video_frame.cc#... media/base/video_frame.cc:104: size_t bytes_per_row = RoundUp(width_, 16) * bytes_per_pixel; Don't have time to do a full review this second, but this 16 should be a constant. static const int kFrameAlignment maybe?
On 2012/06/16 04:23:10, DaleCurtis wrote: > http://codereview.chromium.org/10451051/diff/24002/media/base/video_frame.cc#... > media/base/video_frame.cc:104: size_t bytes_per_row = RoundUp(width_, 16) * > bytes_per_pixel; > Don't have time to do a full review this second, but this 16 should be a > constant. static const int kFrameAlignment maybe? OK, added something like that.
lgtm % nits & qs. http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc File media/base/video_frame.cc (right): http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:104: // Round up to align at least at a 64-bit (8 byte) boundary for each row. s/8/16/ http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:117: DCHECK(!(reinterpret_cast<intptr_t>(data_[VideoFrame::kRGBPlane]) & 7)); Looking at this, I'm surprised it actually passes on Android. They must have an aligned allocator turned on by default. scherkus? http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:139: size_t uv_height = format_ == VideoFrame::YV12 ? y_height / 2 : y_height; Why move this logic out of rows() ? http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:147: av_malloc(y_bytes + (uv_bytes * 2) + kFramePadBytes)); I wonder if kFramePadBytes is still necessary now that we're aligned on 16byte boundaries. http://codereview.chromium.org/10451051/diff/31001/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder.cc (left): http://codereview.chromium.org/10451051/diff/31001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder.cc:398: Can we delete these methods + file from the code base?
http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc File media/base/video_frame.cc (right): http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:104: // Round up to align at least at a 64-bit (8 byte) boundary for each row. On 2012/06/16 17:34:20, DaleCurtis wrote: > s/8/16/ Done. http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:139: size_t uv_height = format_ == VideoFrame::YV12 ? y_height / 2 : y_height; On 2012/06/16 17:34:20, DaleCurtis wrote: > Why move this logic out of rows() ? This is block(16)-aligned height for luma and counterpart for chroma. rows() returns actual height. http://codereview.chromium.org/10451051/diff/31001/media/base/video_frame.cc#... media/base/video_frame.cc:147: av_malloc(y_bytes + (uv_bytes * 2) + kFramePadBytes)); On 2012/06/16 17:34:20, DaleCurtis wrote: > I wonder if kFramePadBytes is still necessary now that we're aligned on 16byte > boundaries. Probably not, I can remove it. http://codereview.chromium.org/10451051/diff/31001/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder.cc (left): http://codereview.chromium.org/10451051/diff/31001/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder.cc:398: On 2012/06/16 17:34:20, DaleCurtis wrote: > Can we delete these methods + file from the code base? No, they're used by webrtc.
LGTM w/ 2 nits nice work! http://codereview.chromium.org/10451051/diff/29003/media/base/video_frame.cc File media/base/video_frame.cc (right): http://codereview.chromium.org/10451051/diff/29003/media/base/video_frame.cc#... media/base/video_frame.cc:104: // Round up to align at least at a 64-bit (16 byte) boundary for each row. 64-bit != 16 bytes :) Just say "align at 16 byte boundary" http://codereview.chromium.org/10451051/diff/29003/media/filters/ffmpeg_video... File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/10451051/diff/29003/media/filters/ffmpeg_video... media/filters/ffmpeg_video_decoder.cc:432: *video_frame = static_cast<VideoFrame *>(av_frame_->opaque); pointer w/ type: VideoFrame*
On 2012/06/18 20:27:38, scherkus wrote: > http://codereview.chromium.org/10451051/diff/29003/media/base/video_frame.cc#... > media/base/video_frame.cc:104: // Round up to align at least at a 64-bit (16 > byte) boundary for each row. > 64-bit != 16 bytes :) > > Just say "align at 16 byte boundary" Done > http://codereview.chromium.org/10451051/diff/29003/media/filters/ffmpeg_video... > media/filters/ffmpeg_video_decoder.cc:432: *video_frame = static_cast<VideoFrame > *>(av_frame_->opaque); > pointer w/ type: VideoFrame* Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbultje@chromium.org/10451051/36001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbultje@chromium.org/10451051/36001
Try job failure for 10451051-36001 (retry) on linux_rel for steps "media_unittests, content_unittests". It's a second try, previously, steps "media_unittests, content_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
content lgtm
LGTM++
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rbultje@chromium.org/10451051/35004
Change committed as 143115 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
