Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1267003004:
Revert to zero-initializing buffers for FFmpegVideoDecoder (Closed)
|
Created:
5 years, 4 months ago by emircan Modified:
5 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRevert to zero-initializing buffers for FFmpegVideoDecoder
Following the regression on , I reverted back to PatchSet#5 from [1]
[0] https://code.google.com/p/chromium/issues/detail?id=514759
[1] https://codereview.chromium.org/1227383003
- Modified VideoFramePool::CreateFrame() to zero-initialize newly allocated frames.
BUG=514759
Committed: https://crrev.com/3ea546df82371eeb7b62f2de25416d4bd21ce745
Cr-Commit-Position: refs/heads/master@{#343289}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Changed method name. #
Total comments: 4
Patch Set 3 : Returning 0 for default. #
Total comments: 7
Patch Set 4 : Zero initialize in VideoFrame::CreateFrame() #Patch Set 5 : Zero initialize only the planes. #
Messages
Total messages: 27 (5 generated)
Patchset #1 (id:1) has been deleted
emircan@chromium.org changed reviewers: + dalecurtis@google.com, miu@chromium.org
Please take a look.
emircan@chromium.org changed reviewers: + dalecurtis@chromium.org - dalecurtis@google.com
https://codereview.chromium.org/1267003004/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1267003004/diff/20001/media/base/video_frame.... media/base/video_frame.h:253: // Returns the actual allocation size for a YUV frame that is created by Why do we need these two methods again? Can we just make AllocationSize return the right information?
https://codereview.chromium.org/1267003004/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1267003004/diff/20001/media/base/video_frame.... media/base/video_frame.h:253: // Returns the actual allocation size for a YUV frame that is created by On 2015/08/03 21:23:25, DaleCurtis wrote: > Why do we need these two methods again? Can we just make AllocationSize return > the right information? Only VideoFrame::CreateFrame() uses VideoFrame::AllocateYUV() to allocate buffers and that is not equal to AllocationSize() as some alignments and padding takes space -exceptional case-. FFmpegVideoDecoder allocates via VideoFrame::CreateFrame() above. It sets the pointers to the start of the planes according to the aligned allocation[0]. However, when it calls av_buffer_create, it uses AllocationSize() which is the packed size [1]. [0] https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... I tried changing av_buffer_create() parameter in [2] PatchSet#4 but the bots didn't like it. I am also not clear why this inconsistency is is there. But having AlignedAllocationSize() helps me to make sure to zero the whole allocated buffer. I can use AllocationSize() instead and only zero the packed size as well. WDYT? [2] https://codereview.chromium.org/1227383003
Oh yeah... Hmm, I feel these names are confusing, but don't have a better suggestion. Maybe the first shouldn't be called allocation size, but instead something like PackedSize() ?
Can't access crbug.com/514759. Why was the change reverted? MSAN issues?
Patchset #2 (id:40001) has been deleted
On 2015/08/04 04:38:43, miu wrote: > Can't access crbug.com/514759. Why was the change reverted? MSAN issues? You are cc'ed now. You should be able to see. I uploaded a new patch by changing the method to a member rather than static. PTAL.
Yeah, the gist is ffmpeg is using these values to calculate other values and then reading from those addresses. In most cases it pre-zeros this data section, but it looks like it doesn't always do this with corrupted content, and debugging this isn't worth the trouble, so we should just zero initialize. https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.... media/base/video_frame.cc:634: if (allocated_size_) Just default to zero in all cases except the AllocateYUV case, then this can be a simple accessor. https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.... media/base/video_frame.h:301: size_t allocated_size() const; Return inline.
https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.... media/base/video_frame.cc:634: if (allocated_size_) On 2015/08/04 19:07:44, DaleCurtis wrote: > Just default to zero in all cases except the AllocateYUV case, then this can be > a simple accessor. Done. https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1267003004/diff/60001/media/base/video_frame.... media/base/video_frame.h:301: size_t allocated_size() const; On 2015/08/04 19:07:44, DaleCurtis wrote: > Return inline. Done.
https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame.... media/base/video_frame.h:452: size_t allocated_size_; This isn't needed. See comments below. https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... media/base/video_frame_pool.cc:87: memset(frame->data(0), 0, frame->allocated_size()); You have enough information to memset() all the memory without having this extra allocated_size() property. Meaning: if (zero_new_frames) { const size_t num_planes = VideoFrame::NumPlanes(frame->format()); for (size_t i = 0; i < num_planes; ++i) { memset(frame->data(i), 0, VideoFrame::PlaneSize(frame->format(), i, frame->coded_size()).GetArea())); } } https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:121: format, coded_size, gfx::Rect(size), natural_size, kNoTimestamp(), true); Instead of adding this extra bool parameter to VideoFramePool::CreateFrame(), you should just memset the memory here in ffmpeg_video_decoder.cc. This seems to be the only place where the memory is required to be initialized.
https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... media/base/video_frame_pool.cc:87: memset(frame->data(0), 0, frame->allocated_size()); On 2015/08/04 20:16:22, miu wrote: > You have enough information to memset() all the memory without having this extra > allocated_size() property. Meaning: > > if (zero_new_frames) { > const size_t num_planes = VideoFrame::NumPlanes(frame->format()); > for (size_t i = 0; i < num_planes; ++i) { > memset(frame->data(i), > 0, > VideoFrame::PlaneSize(frame->format(), > i, > frame->coded_size()).GetArea())); > } > } > This doesn't include the padding around the frame or the extra U plane allocation. See l.889
On 2015/08/04 20:16:22, miu wrote: > https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... > media/base/video_frame_pool.cc:87: memset(frame->data(0), 0, > frame->allocated_size()); > You have enough information to memset() all the memory without having this extra > allocated_size() property. Meaning: > > if (zero_new_frames) { > const size_t num_planes = VideoFrame::NumPlanes(frame->format()); > for (size_t i = 0; i < num_planes; ++i) { > memset(frame->data(i), > 0, > VideoFrame::PlaneSize(frame->format(), > i, > frame->coded_size()).GetArea())); > } > } VideoFrame::PlaneSize() only rounds up to 2 the given dimensions. AllocateYUV() uses kFrameSizeAlignment, which might end up in different results. Also, as dalecurtis@ pointed out this doesn't include the padding. > https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... > media/filters/ffmpeg_video_decoder.cc:121: format, coded_size, gfx::Rect(size), > natural_size, kNoTimestamp(), true); > Instead of adding this extra bool parameter to VideoFramePool::CreateFrame(), > you should just memset the memory here in ffmpeg_video_decoder.cc. This seems > to be the only place where the memory is required to be initialized. That would end up zeroing each buffer usage, rather than only zeroing during initialization. I think we want to only zero during allocation as in the original usage. WDYT?
https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1267003004/diff/80001/media/base/video_frame_... media/base/video_frame_pool.cc:87: memset(frame->data(0), 0, frame->allocated_size()); On 2015/08/04 21:20:52, DaleCurtis wrote: > On 2015/08/04 20:16:22, miu wrote: > > You have enough information to memset() all the memory without having this > extra > > allocated_size() property. Meaning: > > > > if (zero_new_frames) { > > const size_t num_planes = VideoFrame::NumPlanes(frame->format()); > > for (size_t i = 0; i < num_planes; ++i) { > > memset(frame->data(i), > > 0, > > VideoFrame::PlaneSize(frame->format(), > > i, > > frame->coded_size()).GetArea())); > > } > > } > > > > This doesn't include the padding around the frame or the extra U plane > allocation. See l.889 Okay, but is it enough to ensure FFMPEG isn't reading uninitialized data? Basically, I'm opposed to adding |allocated_size| to VideoFrames just for this one edge case. Let's be sure we absolutely have to do this. BTW--If we "absolutely have to do this," we should just have VideoFrame::CreateFrame() do it (e.g., in AllocateYUV() or nearby), rather than expose this as an externally-handled thing. https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:121: format, coded_size, gfx::Rect(size), natural_size, kNoTimestamp(), true); On 2015/08/04 20:16:22, miu wrote: > Instead of adding this extra bool parameter to VideoFramePool::CreateFrame(), > you should just memset the memory here in ffmpeg_video_decoder.cc. This seems > to be the only place where the memory is required to be initialized. OIC. Hmm...now I'm thinking we ought to just always zero-out the VideoFrames the first time they're created in the pool. This is only a one-time cost, so it shouldn't be a huge performance problem for the non-FFMPEG modules that do not need the zero-initialization. WDYT?
https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1267003004/diff/80001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:121: format, coded_size, gfx::Rect(size), natural_size, kNoTimestamp(), true); On 2015/08/06 21:23:30, miu wrote: > On 2015/08/04 20:16:22, miu wrote: > > Instead of adding this extra bool parameter to VideoFramePool::CreateFrame(), > > you should just memset the memory here in ffmpeg_video_decoder.cc. This seems > > to be the only place where the memory is required to be initialized. > > OIC. Hmm...now I'm thinking we ought to just always zero-out the VideoFrames > the first time they're created in the pool. This is only a one-time cost, so it > shouldn't be a huge performance problem for the non-FFMPEG modules that do not > need the zero-initialization. WDYT? That's fine with me, in a pool the one-time cost should be low.
On 2015/08/06 21:23:30, miu wrote: > Okay, but is it enough to ensure FFMPEG isn't reading uninitialized data? > Basically, I'm opposed to adding |allocated_size| to VideoFrames just for this > one edge case. Let's be sure we absolutely have to do this. > > BTW--If we "absolutely have to do this," we should just have > VideoFrame::CreateFrame() do it (e.g., in AllocateYUV() or nearby), rather than > expose this as an externally-handled thing. Last time bots were happy but eventually it bounced back. So you want me to zero-initialize the planes as you described and try sending it to bots? Or add a bool |zero_new_frame| to create frames as in the patch I uploadad above? If you are OK with the new signature, I can go ahead and chnage the 39 references to CreateFrame()? > OIC. Hmm...now I'm thinking we ought to just always zero-out the VideoFrames > the first time they're created in the pool. This is only a one-time cost, so it > shouldn't be a huge performance problem for the non-FFMPEG modules that do not > need the zero-initialization. WDYT? I agree, it is reasonable to initialize for every user of the pool
On 2015/08/07 17:07:45, emircan wrote: > On 2015/08/06 21:23:30, miu wrote: > > Okay, but is it enough to ensure FFMPEG isn't reading uninitialized data? > > Basically, I'm opposed to adding |allocated_size| to VideoFrames just for this > > one edge case. Let's be sure we absolutely have to do this. > > > > BTW--If we "absolutely have to do this," we should just have > > VideoFrame::CreateFrame() do it (e.g., in AllocateYUV() or nearby), rather > than > > expose this as an externally-handled thing. > > Last time bots were happy but eventually it bounced back. So you want me to > zero-initialize the planes as you described and try sending it to bots? I would try this first. Because the alternative is lots of work for you. ;-) > Or add a bool |zero_new_frame| to create frames as in the patch I uploadad > above? If you are OK with the new signature, I can go ahead and chnage the 39 > references to CreateFrame()? This would be the "lots of work for you" alternative. Also, if we're honest, the real root problem is explained in my comments on this other code review: https://codereview.chromium.org/1286623002/#msg8 Basically, the root problem is that VideoFrame::CreateFrame() can ignore the coded_size it is given and round-up. That's bad for what you need to do for "FFMPEG zero init" here, because you don't know how much memory to memset(). It's also bad for all the other callers of VideoFrame::CreateFrame() that require an exact coded_size without any alignment adjustments (e.g., HW encoders have specific, and sometimes strange, alignment requirements).
On 2015/08/11 21:06:31, miu wrote: > On 2015/08/07 17:07:45, emircan wrote: > > On 2015/08/06 21:23:30, miu wrote: > > > Okay, but is it enough to ensure FFMPEG isn't reading uninitialized data? > > > Basically, I'm opposed to adding |allocated_size| to VideoFrames just for > this > > > one edge case. Let's be sure we absolutely have to do this. > > > > > > BTW--If we "absolutely have to do this," we should just have > > > VideoFrame::CreateFrame() do it (e.g., in AllocateYUV() or nearby), rather > > than > > > expose this as an externally-handled thing. > > > > Last time bots were happy but eventually it bounced back. So you want me to > > zero-initialize the planes as you described and try sending it to bots? > > I would try this first. Because the alternative is lots of work for you. ;-) I uploaded a patch with this and sent it to the bots. PTAL.
lgtm, assuming it runs on the msan bots (there was a "patch step" failure as of this writing).
On 2015/08/13 19:34:24, miu wrote: > lgtm, assuming it runs on the msan bots (there was a "patch step" failure as of > this writing). Thanks. Those two are webrtc and v8 bots, so it makes sense why they wouldn't be able to patch. I shouldn't have run them in the first place. dalecurtis@, I still need owners review. PTAL.
lgtm
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267003004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267003004/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3ea546df82371eeb7b62f2de25416d4bd21ce745 Cr-Commit-Position: refs/heads/master@{#343289} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
