Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209143002:
Skip AUD while extracting SPS and PPS on iOS. (Closed)
|
Created:
4 years, 4 months ago by jianjunz Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSkip AUD while extracting SPS and PPS on iOS.
H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD.
BUG=webrtc:6173
Committed: https://crrev.com/e1b4d243e288b9517e9ea66596dae4e597a08f0a
Cr-Commit-Position: refs/heads/master@{#13765}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Feedback fixes #Patch Set 3 : Update unit test. #
Messages
Total messages: 34 (17 generated)
Description was changed from ========== Skip AUD while extracting SPS and PPS on iOS. H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD. BUG=webrtc:6173 ========== to ========== Skip AUD while extracting SPS and PPS on iOS. H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD. BUG=webrtc:6173 ==========
jianjun.zhu@intel.com changed reviewers: + tkchin@webrtc.org
PTAL. Thanks.
tkchin@webrtc.org changed reviewers: + noahric@chromium.org
noah can you help look as well? I haven't read the h264 spec in a long time. https://codereview.webrtc.org/2209143002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2209143002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:261: // start code + access unit delimiter + start code = 4 + 2 + 4 = 10 nit: caps and period // Start code .... = 10. Here and elsewhere. https://codereview.webrtc.org/2209143002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:283: if ((annexb_buffer[4] & 0x1f) == 0x9) { suggest having enum for types, and function that returns type given nalu start ptr will make this more readable, and can also be applied above
https://codereview.webrtc.org/2209143002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2209143002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:261: // start code + access unit delimiter + start code = 4 + 2 + 4 = 10 On 2016/08/04 16:49:05, tkchin_webrtc wrote: > nit: caps and period // Start code .... = 10. Here and elsewhere. Done. https://codereview.webrtc.org/2209143002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:283: if ((annexb_buffer[4] & 0x1f) == 0x9) { On 2016/08/04 16:49:05, tkchin_webrtc wrote: > suggest having enum for types, and function that returns type given nalu start > ptr > will make this more readable, and can also be applied above Included "webrtc/common_video/h264/h264_common.h" for NALU type definition and parsing.
tkchin@webrtc.org changed reviewers: + sprang@webrtc.org
lgtm sprang@ can you take a look as well?
lgtm
The CQ bit was checked by tkchin@webrtc.org
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: linux_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/103)
The CQ bit was checked by jianjun.zhu@intel.com
The CQ bit was unchecked by jianjun.zhu@intel.com
The CQ bit was checked by jianjun.zhu@intel.com
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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
sprang@google.com changed reviewers: + sprang@google.com
lgtm, but would have been nice with a unit test.
Yes, please add a unit test for this.
Thanks for your suggestion. Updated unit test.
The CQ bit was checked by jianjun.zhu@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@google.com, noahric@chromium.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2209143002/#ps40001 (title: "Update unit test.")
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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/9489)
The CQ bit was checked by jianjun.zhu@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Skip AUD while extracting SPS and PPS on iOS. H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD. BUG=webrtc:6173 ========== to ========== Skip AUD while extracting SPS and PPS on iOS. H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD. BUG=webrtc:6173 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Skip AUD while extracting SPS and PPS on iOS. H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD. BUG=webrtc:6173 ========== to ========== Skip AUD while extracting SPS and PPS on iOS. H.264 frames may have AUD before SPS. This change detects AUD and try to extract SPS and PPS behind AUD. BUG=webrtc:6173 Committed: https://crrev.com/e1b4d243e288b9517e9ea66596dae4e597a08f0a Cr-Commit-Position: refs/heads/master@{#13765} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e1b4d243e288b9517e9ea66596dae4e597a08f0a Cr-Commit-Position: refs/heads/master@{#13765} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
