Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2638933002:
H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. (Closed)
DescriptionH264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header.
- Changed method name to clarify that entire Nalus are expected.
- Added unit test code.
- Adjusted InsetSpsPpsNalus() implementation to above requirement.
BUG=webrtc:5948
Review-Url: https://codereview.webrtc.org/2638933002
Cr-Commit-Position: refs/heads/master@{#16221}
Committed: https://chromium.googlesource.com/external/webrtc/+/f53d7374cfa59440f777729d3a0b7dd39830d6ec
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add failure test cases and cleanup. #
Total comments: 6
Patch Set 3 : Remove reundant log msg. #
Messages
Total messages: 26 (13 generated)
The CQ bit was checked by johan@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/...
johan@webrtc.org changed reviewers: + philipel@webrtc.org, sprang@google.com
philipel@ original author of H264SpsPpsTracker. sprang@ Owner
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... File webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc (right): https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:265: std::vector<uint8_t> data; Looks like you don't really need a vector here. If you use a plain array instead, eg const uint8_t[] kData = {1, 2, 3}; ...then you can clean up some code below and get rid of the delete. https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:271: const std::vector<uint8_t> pps({0x68, 0xeb, 0xe3, 0xcb, 0x22, 0xc0}); Is there any data baked in here that's worth checking if it was parsed correctly by the SpsPpsTracker, or is that already covered by other unit tests? https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:274: // Insert first packet of the IDR Nit: End with comments with period.
Description was changed from ========== H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. - Changed method name to clarify that entire Nalus are expected. - Added unit test code. - Adjusted InsetSpsPpsNalus() implementation to above requirement. BUG=webrtc:5948 ========== to ========== H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. - Changed method name to clarify that entire Nalus are expected. - Added unit test code. - Adjusted InsetSpsPpsNalus() implementation to above requirement. BUG=webrtc:5948 ==========
johan@webrtc.org changed reviewers: - sprang@google.com
https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... File webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc (right): https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:265: std::vector<uint8_t> data; On 2017/01/18 09:34:05, språng wrote: > Looks like you don't really need a vector here. If you use a plain array > instead, eg > > const uint8_t[] kData = {1, 2, 3}; > > ...then you can clean up some code below and get rid of the delete. Acknowledged. https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:271: const std::vector<uint8_t> pps({0x68, 0xeb, 0xe3, 0xcb, 0x22, 0xc0}); On 2017/01/18 09:34:05, språng wrote: > Is there any data baked in here that's worth checking if it was parsed correctly > by the SpsPpsTracker, or is that already covered by other unit tests? SpsPpsTracker itself checks only the nalu headers. For further parsing it relies on SpsParser and PpsParser. For that reason, data needs to represent valid sps/pps nalus. Unit test of actual parsing is covered by the respective parser unit tests. But, maybe adding a test case with invalid nalus and invalid nalu header could be a good idea. https://codereview.webrtc.org/2638933002/diff/1/webrtc/modules/video_coding/h... webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc:274: // Insert first packet of the IDR On 2017/01/18 09:34:05, språng wrote: > Nit: End with comments with period. Acknowledged.
lg https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:188: if (sps.size() < kNaluHeaderOffset) { Why do we perform these checks before parsing? For better logging? In that case don't you think it's better to add more logging into the Sps/PpsParser? https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:219: if (!parsed_pps || !parsed_sps) { Remove this if block.
https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:188: if (sps.size() < kNaluHeaderOffset) { On 2017/01/18 12:51:48, philipel wrote: > Why do we perform these checks before parsing? For better logging? In that case > don't you think it's better to add more logging into the Sps/PpsParser? The SpsPraser and PpsParser implementations handle only Nal-unit "payload" not the Nal-unit "headers". At least that's my conclusion from debugging unit tests and from comparing parser implementation to H264 spec, section 7.3.1.ff. So this function should do a plausibility check on the nalu headers before handing the nalu payloads to the actual SpsParser/PpsParser. https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:219: if (!parsed_pps || !parsed_sps) { On 2017/01/18 12:51:48, philipel wrote: > Remove this if block. Removing the log, sticking with the return.
lgtm https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right): https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:188: if (sps.size() < kNaluHeaderOffset) { On 2017/01/18 14:26:04, johan wrote: > On 2017/01/18 12:51:48, philipel wrote: > > Why do we perform these checks before parsing? For better logging? In that > case > > don't you think it's better to add more logging into the Sps/PpsParser? > > The SpsPraser and PpsParser implementations handle only Nal-unit "payload" not > the Nal-unit "headers". At least that's my conclusion from debugging unit tests > and from comparing parser implementation to H264 spec, section 7.3.1.ff. > > So this function should do a plausibility check on the nalu headers before > handing the nalu payloads to the actual SpsParser/PpsParser. > Acknowledged. https://codereview.webrtc.org/2638933002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/h264_sps_pps_tracker.cc:219: if (!parsed_pps || !parsed_sps) { On 2017/01/18 14:26:04, johan wrote: > On 2017/01/18 12:51:48, philipel wrote: > > Remove this if block. > > Removing the log, sticking with the return. Ah... ofc
@sprang PTAL
lgtm
The CQ bit was checked by johan@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: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17911) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22415) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
The CQ bit was checked by johan@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485191513105780,
"parent_rev": "e1405ad0d164230c77c4ac10efd92db3ef2f9030", "commit_rev":
"f53d7374cfa59440f777729d3a0b7dd39830d6ec"}
Message was sent while issue was closed.
Description was changed from ========== H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. - Changed method name to clarify that entire Nalus are expected. - Added unit test code. - Adjusted InsetSpsPpsNalus() implementation to above requirement. BUG=webrtc:5948 ========== to ========== H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. - Changed method name to clarify that entire Nalus are expected. - Added unit test code. - Adjusted InsetSpsPpsNalus() implementation to above requirement. BUG=webrtc:5948 Review-Url: https://codereview.webrtc.org/2638933002 Cr-Commit-Position: refs/heads/master@{#16221} Committed: https://chromium.googlesource.com/external/webrtc/+/f53d7374cfa59440f777729d3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/f53d7374cfa59440f777729d3...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2649113003/ by kjellander@webrtc.org. The reason for reverting is: Triggers leak on Linux memcheck (non-default trybot): ### BEGIN MEMORY TOOL REPORT (error hash=#0112A395AF2326BC#) Command: ../Release/./modules_unittests --isolated-script-test-output=/b/s/w/ioUlJCnu/output.json --isolated-script-test-chartjson-output=/b/s/w/ioUlJCnu/chartjson-output.json --gtest_filter=-CommonFormats/AudioProcessingTest* Leak_DefinitelyLost 45 bytes in 1 blocks are definitely lost in loss record 118 of 277 operator new[](unsigned long) (m_replacemalloc/vg_replace_malloc.c:363) webrtc::video_coding::H264SpsPpsTracker::CopyAndFixBitstream(webrtc::VCMPacket*) (/b/s/w/irJgAGsR/out/Release/modules_unittests) webrtc::video_coding::TestH264SpsPpsTracker_SpsPpsOutOfBand_Test::TestBody() (/b/s/w/irJgAGsR/out/Release/modules_unittests) Suppression (error hash=#0112A395AF2326BC#): For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem... { <insert_a_suppression_name_here> Memcheck:Leak fun:_Zna* fun:_ZN6webrtc12video_coding17H264SpsPpsTracker19CopyAndFixBitstreamEPNS_9VCMPacketE fun:_ZN6webrtc12video_coding42TestH264SpsPpsTracker_SpsPpsOutOfBand_Test8TestBodyEv } ### END MEMORY TOOL REPORT (error hash=#0112A395AF2326BC#) . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
