Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1602523004:
Added EncodedImage::GetBufferPaddingBytes. (Closed)
|
Created:
4 years, 11 months ago by hbos Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded EncodedImage::GetBufferPaddingBytes.
The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms.
We plan to use FFmpeg for a soon-to-land H.264 enc/dec.
This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding.
All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change.
Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing.
BUG=chromium:468365
BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424
NOTRY=True
Committed: https://crrev.com/d664836efa2f51edc6d7e277f007d47ae97ba7ff
Cr-Commit-Position: refs/heads/master@{#11337}
Patch Set 1 : 8 additional bytes regardless of codec, for testing #Patch Set 2 : 0 additional bytes, make ios compile #
Total comments: 9
Patch Set 3 : Moved constant, reverted dependency, function name indicating unit is bytes #
Total comments: 1
Patch Set 4 : Constant inside EncodedImage instead #
Messages
Total messages: 27 (14 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be added to the end of the encoded image buffers it reads due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. BUG=468365 ========== to ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. BUG=468365 ==========
Description was changed from ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. BUG=468365 ========== to ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. BUG=468365 ==========
Description was changed from ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. BUG=468365 ========== to ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=468365 ==========
hbos@webrtc.org changed reviewers: + mflodman@webrtc.org, stefan@webrtc.org
Please take a look, stefan and mflodman. https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h#newc... webrtc/video_frame.h:178: static size_t GetBufferPadding(VideoCodecType codec_type); (I originally wanted to place this in VideoDecoder but experienced circular dependencies. This place makes sense too though.)
On 2016/01/19 14:22:18, hbos wrote: > Please take a look, stefan and mflodman. > > https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h > File webrtc/video_frame.h (right): > > https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h#newc... > webrtc/video_frame.h:178: static size_t GetBufferPadding(VideoCodecType > codec_type); > (I originally wanted to place this in VideoDecoder but experienced circular > dependencies. This place makes sense too though.) Change to BUG=chromium:468365
We also talked about DCHECKing in the decoder to make sure there actually is exactly 8 extra bytes added. Could you add that too? https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264.cc:24: // when the FFmpeg decoder is added. Should we also file a bug to the ffmpeg project? Seems like something they should be able to fix. https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h#newc... webrtc/video_frame.h:178: static size_t GetBufferPadding(VideoCodecType codec_type); On 2016/01/19 14:22:18, hbos wrote: > (I originally wanted to place this in VideoDecoder but experienced circular > dependencies. This place makes sense too though.) Somewhat... VideoDecoder would probably have been slightly nicer, but if the dependencies are weird maybe this is good enough.
Description was changed from ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=468365 ========== to ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=468365, https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 ==========
Description was changed from ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=468365, https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 ========== to ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 ==========
PTAL stefan (and mflodman), replied to comments. On 2016/01/19 15:06:20, stefan-webrtc (holmer) wrote: > We also talked about DCHECKing in the decoder to make sure there actually is > exactly 8 extra bytes added. Could you add that too? Yes definitely, but the decoder is not in this CL (and has not landed). I'll do that there. Did this in a separate CL to ease review process. https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264.cc:24: // when the FFmpeg decoder is added. On 2016/01/19 15:06:20, stefan-webrtc (holmer) wrote: > Should we also file a bug to the ffmpeg project? Seems like something they > should be able to fix. It is documented in their source code (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...) so they already know about it. Think I should create a bug anyway? https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/video_frame.h#newc... webrtc/video_frame.h:178: static size_t GetBufferPadding(VideoCodecType codec_type); On 2016/01/19 15:06:20, stefan-webrtc (holmer) wrote: > On 2016/01/19 14:22:18, hbos wrote: > > (I originally wanted to place this in VideoDecoder but experienced circular > > dependencies. This place makes sense too though.) > > Somewhat... VideoDecoder would probably have been slightly nicer, but if the > dependencies are weird maybe this is good enough. They look weird to me.
Bump stefan, mflodman
lgtm from me but I'd like to get Magnus opinion on the dependencies https://codereview.webrtc.org/1602523004/diff/40001/webrtc/common_video/commo... File webrtc/common_video/common_video.gyp (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/common_video/commo... webrtc/common_video/common_video.gyp:23: '<(webrtc_root)/modules/modules.gyp:webrtc_h264', Not an awesome dependency to have for a constant... Magnus, what do you say? Should we add this dependency or move the constant to EncodedImage? https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264.cc:24: // when the FFmpeg decoder is added. On 2016/01/19 16:50:34, hbos wrote: > On 2016/01/19 15:06:20, stefan-webrtc (holmer) wrote: > > Should we also file a bug to the ffmpeg project? Seems like something they > > should be able to fix. > > It is documented in their source code > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...) > so they already know about it. Think I should create a bug anyway? I think we should at least ask whether they would consider fixing it. I don't see any reason why it should behave in this way, as they can read the whole frame except the end as chunks of 64 bits. I'd be surprised if that would end up being a performance issue.
https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264.cc:24: // when the FFmpeg decoder is added. On 2016/01/21 10:34:47, stefan-webrtc (holmer) wrote: > On 2016/01/19 16:50:34, hbos wrote: > > On 2016/01/19 15:06:20, stefan-webrtc (holmer) wrote: > > > Should we also file a bug to the ffmpeg project? Seems like something they > > > should be able to fix. > > > > It is documented in their source code > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...) > > so they already know about it. Think I should create a bug anyway? > > I think we should at least ask whether they would consider fixing it. I don't > see any reason why it should behave in this way, as they can read the whole > frame except the end as chunks of 64 bits. I'd be surprised if that would end up > being a performance issue. Agreed, I created a ticket (https://trac.ffmpeg.org/ticket/5176). Got a reply: "[...] if we wouldn't have padding we would have to check every time if we're at the end of the buffer [...] Don't expect any changes, this is a clear design decision."
https://codereview.webrtc.org/1602523004/diff/40001/webrtc/common_video/commo... File webrtc/common_video/common_video.gyp (right): https://codereview.webrtc.org/1602523004/diff/40001/webrtc/common_video/commo... webrtc/common_video/common_video.gyp:23: '<(webrtc_root)/modules/modules.gyp:webrtc_h264', On 2016/01/21 10:34:47, stefan-webrtc (holmer) wrote: > Not an awesome dependency to have for a constant... > > Magnus, what do you say? Should we add this dependency or move the constant to > EncodedImage? I'd prefer to move the constant, different module target already depends on common_video and this will introduce a circular dependency.
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Added EncodedImage::GetBufferPadding. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 ========== to ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 ==========
PTAL mflodman
One minot comment, then LGTM. https://codereview.webrtc.org/1602523004/diff/80001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/1602523004/diff/80001/webrtc/video_frame.h#newc... webrtc/video_frame.h:22: extern const size_t kEncodedImagePaddingH264; Maybe this should be part of class EncodedImage, wdyt?
Description was changed from ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 ========== to ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 NOTRY=True ==========
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1602523004/#ps100001 (title: "Constant inside EncodedImage instead")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1602523004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1602523004/100001
Message was sent while issue was closed.
Description was changed from ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 NOTRY=True ========== to ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 NOTRY=True ========== to ========== Added EncodedImage::GetBufferPaddingBytes. The FFmpeg video decoder requires up to 8 additional bytes to be allocated for its encoded image buffer input, due to optimized byte readers over-reading on some platforms. We plan to use FFmpeg for a soon-to-land H.264 enc/dec. This CL adds support for padding encoded image buffers based on codec type, and makes sure calls to VCMEncodedFrame::VerifyAndAllocate use the padding. All padding constants are 0 but making H.264 pad with 8 bytes will be a one-line change. Also, added -framework CoreFoundation to webrtc_h264_video_toolbox which was missing. BUG=chromium:468365 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5424 NOTRY=True Committed: https://crrev.com/d664836efa2f51edc6d7e277f007d47ae97ba7ff Cr-Commit-Position: refs/heads/master@{#11337} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d664836efa2f51edc6d7e277f007d47ae97ba7ff Cr-Commit-Position: refs/heads/master@{#11337} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
