Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2356793002:
Add support for 3-byte headers in VideoToolbox NALU parser. (Closed)
|
Created:
4 years, 3 months ago by kthelgason Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman, sprang_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd support for 3-byte headers in VideoToolbox NALU parser.
BUG=webrtc:6278
R=tkchin@webrtc.org, tommi@webrtc.org
Committed: https://crrev.com/cb18ee61270694e743970100bf2871bd67a44906
Cr-Commit-Position: refs/heads/master@{#14889}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Code review #
Total comments: 9
Patch Set 3 : Alphabetize using statements #Patch Set 4 : Add back check #Patch Set 5 : rebase #
Messages
Total messages: 22 (10 generated)
kthelgason@webrtc.org changed reviewers: + tkchin@webrtc.org, tommi@webrtc.org
There may be some work left to fully support this. I'm not sure how to test.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm https://codereview.webrtc.org/2356793002/diff/1/webrtc/common_video/h264/h264... File webrtc/common_video/h264/h264_common.cc (right): https://codereview.webrtc.org/2356793002/diff/1/webrtc/common_video/h264/h264... webrtc/common_video/h264/h264_common.cc:25: if (buffer_size < kNaluShortStartSequenceSize) { nit: no {} (see the convention below for 2-line conditionals) https://codereview.webrtc.org/2356793002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2356793002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:331: offset_++; nit: ++foo
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (left): https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:61: RTC_DCHECK_EQ(nalu_header_size, 4); why is this removed? Perhaps should be checked that it is equal to kAvccHeaderByteSize instead? https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:29: using H264::NaluIndex; nit: consider alphabetizing https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:141: size_t bytes_written = packet_size + sizeof(kAnnexBHeaderBytes); good catch https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:315: offsets_ = H264::FindNaluIndices(annexb_buffer, length); How large do we expect these buffers to be? Is it safe to put potentially heavy work into the ctor like this?
https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (left): https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:61: RTC_DCHECK_EQ(nalu_header_size, 4); On 2016/09/27 11:29:35, tkchin_webrtc wrote: > why is this removed? Perhaps should be checked that it is equal to > kAvccHeaderByteSize instead? That doesn't make sense to me, won't kAvccHeaderByteSize always be 4? nalu_header_size is supposed to be allowed to be either 3 or 4, right? https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:29: using H264::NaluIndex; On 2016/09/27 11:29:35, tkchin_webrtc wrote: > nit: consider alphabetizing Done. https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:315: offsets_ = H264::FindNaluIndices(annexb_buffer, length); On 2016/09/27 11:29:34, tkchin_webrtc wrote: > How large do we expect these buffers to be? Is it safe to put potentially heavy > work into the ctor like this? I have no idea, didn't consider that. In what context would it represent an issue? The work has to be done at some point.
https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (left): https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:61: RTC_DCHECK_EQ(nalu_header_size, 4); On 2016/09/28 08:48:27, kthelgason wrote: > On 2016/09/27 11:29:35, tkchin_webrtc wrote: > > why is this removed? Perhaps should be checked that it is equal to > > kAvccHeaderByteSize instead? > That doesn't make sense to me, won't kAvccHeaderByteSize always be 4? > nalu_header_size is supposed to be allowed to be either 3 or 4, right? iiuc nalu_header_size here is not the 0001/001. It is the length byte for avcc format. We expect it to only 4 bytes (the size of a 32-bit int) but who knows if what's returned by the CMSampleBuffer changes. https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2356793002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:315: offsets_ = H264::FindNaluIndices(annexb_buffer, length); On 2016/09/28 08:48:27, kthelgason wrote: > On 2016/09/27 11:29:34, tkchin_webrtc wrote: > > How large do we expect these buffers to be? Is it safe to put potentially > heavy > > work into the ctor like this? > > I have no idea, didn't consider that. In what context would it represent an > issue? The work has to be done at some point. It probably doesn't matter in this particular case. It would only be an issue if the buffer was extremely large. Just following the general paradigm of not putting heavy work into ctors.
> iiuc nalu_header_size here is not the 0001/001. It is the length byte for avcc > format. We expect it to only 4 bytes (the size of a 32-bit int) but who knows if > what's returned by the CMSampleBuffer changes. Ok. It's pretty hard to find any documentation on these methods. I'm not convinced that this CHECK actually does anything useful for us though. > It probably doesn't matter in this particular case. It would only be an issue if > the buffer was extremely large. Just following the general paradigm of not > putting heavy work into ctors. Noted. I can't think of a simple change to make here that will place this somewhere else so I'm inclined to leave it like this, but definitely something to keep in mind.
On 2016/09/28 11:40:35, kthelgason wrote: > > iiuc nalu_header_size here is not the 0001/001. It is the length byte for avcc > > format. We expect it to only 4 bytes (the size of a 32-bit int) but who knows > if > > what's returned by the CMSampleBuffer changes. > > Ok. It's pretty hard to find any documentation on these methods. I'm not > convinced that this CHECK actually does anything useful for us though. I think it is useful to even CHECK instead of DCHECK it. It's not a situation we can handle right now because we are casting the bits directly to uint32_t later and expect precisely 4 bytes.
Care to take one last look Zeke?
On 2016/10/18 08:18:12, kthelgason wrote: > Care to take one last look Zeke? lgtm
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2356793002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Add support for 3-byte headers in VideoToolbox NALU parser. BUG=webrtc:6278 ========== to ========== Add support for 3-byte headers in VideoToolbox NALU parser. BUG=webrtc:6278 R=tkchin@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/cb18ee61270694e743970100b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as cb18ee61270694e743970100bf2871bd67a44906 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Add support for 3-byte headers in VideoToolbox NALU parser. BUG=webrtc:6278 R=tkchin@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/cb18ee61270694e743970100b... ========== to ========== Add support for 3-byte headers in VideoToolbox NALU parser. BUG=webrtc:6278 R=tkchin@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/cb18ee61270694e743970100bf2871bd67a44906 Cr-Commit-Position: refs/heads/master@{#14889} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
