|
|
DescriptionAdding test for SingleNalUnit mode
Test enables single-nalu mode, sets limit for nalu lenght and verifies
that encoder follows that limit.
I found that QP jumps significantly when the mode is enabled. In result
encoder might produce 4kbyte and 0.4kbyte frames back-to-back. But it
seems that happens only to couple of frames in the beginning. This
caused test to fail with default RC thresholds. To bypass this I
increased frame size mismatch threshold from 20 to 30%. This should be
Ok considering single-nalu mode is rare.
BUG=webrtc:8070
Review-Url: https://codereview.webrtc.org/3014623002
Cr-Commit-Position: refs/heads/master@{#20023}
Committed: https://webrtc.googlesource.com/src/+/612f858ba0aa35c2993c4510b8ec5d3f8a5b70bf
Patch Set 1 #Patch Set 2 : addressing cl reviewers comments #
Total comments: 31
Patch Set 3 : Addressed review notes on coding style #Patch Set 4 : Addressed review notes on coding style #
Total comments: 6
Patch Set 5 : Addressed review notes #
Total comments: 5
Patch Set 6 : added check for empty nalu_indices #Patch Set 7 : Added codec_globals_headers to deps of video_codecs_test_framework #Messages
Total messages: 45 (20 generated)
The CQ bit was checked by ssilkin@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/28396)
copied from https://codereview.webrtc.org/3011373002/#msg28 On 2017/09/25 11:02:35, hbos wrote: > Sorry for the delayed review. > > Yes we should update chromium to, and we should try to land both changes at the > same time, we don't want two different versions. The chromium CL needs to update > https://cs.chromium.org/chromium/src/third_party/openh264/README.chromium to > reflect the new version number. > > Might I suggest splitting up the CL for adding the NALU test and updating the > version into two CLs? It changes more than just unittest files. done > If the test fails before the version is updated you can expect it to fail and > have a TODO saying when we use v.1.7 the test will pass. > > https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > File modules/video_coding/codecs/test/videoprocessor.cc (right): > > https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor.cc:115: return 0; > Does this mean that if HW is used we don't know the max NALU length? Prefer > rtc::Optional<size_t> for N/A rather than 0. done https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): > > https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:345: > codec.SetParam(cricket::kH264FmtpPacketizationMode, "0"); > nit: RTC_DCHECK_EQ(config_packetization_mode, > H264PacketizationMode::SingleNalUnit); not sure is need such check in this place. there are unit tests CanInitializeWithSingleNalUnitModeExplicitly/CanInitializeWithSingleNalUnitModeExplicitly which verify setting value through cricket::VideoCoder.
copied from https://codereview.webrtc.org/3011373002/#msg28 On 2017/09/25 11:02:35, hbos wrote: > Sorry for the delayed review. > > Yes we should update chromium to, and we should try to land both changes at the > same time, we don't want two different versions. The chromium CL needs to update > https://cs.chromium.org/chromium/src/third_party/openh264/README.chromium to > reflect the new version number. > > Might I suggest splitting up the CL for adding the NALU test and updating the > version into two CLs? It changes more than just unittest files. done > If the test fails before the version is updated you can expect it to fail and > have a TODO saying when we use v.1.7 the test will pass. > > https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > File modules/video_coding/codecs/test/videoprocessor.cc (right): > > https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor.cc:115: return 0; > Does this mean that if HW is used we don't know the max NALU length? Prefer > rtc::Optional<size_t> for N/A rather than 0. done https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): > > https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:345: > codec.SetParam(cricket::kH264FmtpPacketizationMode, "0"); > nit: RTC_DCHECK_EQ(config_packetization_mode, > H264PacketizationMode::SingleNalUnit); not sure is need such check in this place. there are unit tests CanInitializeWithSingleNalUnitModeExplicitly/CanInitializeWithSingleNalUnitModeExplicitly which verify setting value through cricket::VideoCoder.
ssilkin@webrtc.org changed reviewers: + brandtr@webrtc.org, hbos@webrtc.org, kjellander@webrtc.org
Nice. Please update the CL description (and title), I wrote some guidelines in the other CL. Can this land independently of the other CL or does the test break because of v.1.6? https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:345: codec.SetParam(cricket::kH264FmtpPacketizationMode, "0"); If there is an assumption about the enum only having two values and "else" being a particular value I would still suggest RTC_DCHECK_EQ as a preemptive measure in case more values are added to the enum in the future and the assumption no longer holds. If that isn't the case you don't need to DCHECK. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:575: const BitstreamThresholds *bs_thresholds) { nit: const BitstreamThresholds* bs_thresholds https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:202: void VerifyBitstream(int frame_number, Does it make more sense for frame_number to be size_t? It's used as an index and should never be negative, right? https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:203: const BitstreamThresholds *bs_thresholds); nit: const BitstreamThresholds* bs_thresholds (* is part of the type). And the indentation is wrong, it should be: void VerifyBitstream(int frame_number, const BitstreamThresholds* bs_thresholds); See git cl format . https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:60: &quality_thresholds, nullptr, kNoVisualizationParams); nit: There should be 2 more spaces in the indentation. You can run git cl format . And it will fix indentations etc to match the style guide. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc:96: &quality_thresholds, &bs_thresholds, kNoVisualizationParams); nit: 2 more spaces to the indentation here, git cl format .
The CQ bit was checked by hbos@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
Description was changed from ========== Splitting cl https://codereview.webrtc.org/3011373002 into two: updating v1.7 and adding unit tests. This one is about unit test for single-nalu mode: test enables the mode, sets limit for nalu lenght and verifies that encoder follows that limit. I found that QP jumps significantly when single-nalu mode is enabled. In result encoder produces 4kbyte and 0.4kbyte frames back-to-back. But it seems that happens only to couple of frames in the beginning. This caused test to fail with default RC thresholds. To bypass this I increased frame size mismatch threshold from 20 to 30%. This should be Ok considering single-nalu mode is rare. BUG=webrtc:8070 ========== to ========== Adding test for SingleNalUnit mode Test enables single-nalu mode, sets limit for nalu lenght and verifies that encoder follows that limit. I found that QP jumps significantly when the mode is enabled. In result encoder might produce 4kbyte and 0.4kbyte frames back-to-back. But it seems that happens only to couple of frames in the beginning. This caused test to fail with default RC thresholds. To bypass this I increased frame size mismatch threshold from 20 to 30%. This should be Ok considering single-nalu mode is rare. BUG=webrtc:8070 ==========
Test fails because of v1.6.
https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:345: codec.SetParam(cricket::kH264FmtpPacketizationMode, "0"); On 2017/09/25 16:13:41, hbos wrote: > If there is an assumption about the enum only having two values and "else" being > a particular value I would still suggest RTC_DCHECK_EQ as a preemptive measure > in case more values are added to the enum in the future and the assumption no > longer holds. If that isn't the case you don't need to DCHECK. Sorry, i didn't get you previous comment on this right. I agree that should be added. Added. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:575: const BitstreamThresholds *bs_thresholds) { On 2017/09/25 16:13:41, hbos wrote: > nit: const BitstreamThresholds* bs_thresholds Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:202: void VerifyBitstream(int frame_number, On 2017/09/25 16:13:41, hbos wrote: > Does it make more sense for frame_number to be size_t? It's used as an index and > should never be negative, right? Agree. There are tons of ints in videoprocessor which only take positive values and should be size_t. However, I would suggest to keep it as it as for now and do refactoring separately. It is just too big change for this little CL. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:203: const BitstreamThresholds *bs_thresholds); On 2017/09/25 16:13:41, hbos wrote: > nit: const BitstreamThresholds* bs_thresholds (* is part of the type). > > And the indentation is wrong, it should be: > void VerifyBitstream(int frame_number, > const BitstreamThresholds* bs_thresholds); > See git cl format . Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:60: &quality_thresholds, nullptr, kNoVisualizationParams); On 2017/09/25 16:13:41, hbos wrote: > nit: There should be 2 more spaces in the indentation. You can run > git cl format . > And it will fix indentations etc to match the style guide. Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc:96: &quality_thresholds, &bs_thresholds, kNoVisualizationParams); On 2017/09/25 16:13:41, hbos wrote: > nit: 2 more spaces to the indentation here, git cl format . Done.
Looks good! I added some comments. It's a useful test to have, and it would have caught the video quality problems in the bug. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/stats.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/stats.h:34: rtc::Optional<size_t> max_nalu_length; Maybe add a comment that this is only relevant for H.264? (Hence the rtc::Optional.) All other members are codec-agnostic. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:19: #include <algorithm> Also sort the include order here. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:113: const TestConfig& config) { To fix the indentation, you can run 'git cl format' in the checkout directory. That formats all the code that you have touched according to the Google C++ style guide. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:121: size_t maxLength = 0; We typically use snake_case for naming local variables. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.h:21: #include "common_video/h264/h264_common.h" Reorder with line 20, to have the includes in alphabetical order. I think the presubmit will complain otherwise. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.h:107: H264PacketizationMode packetization_mode = Add explicit include to webrtc/modules/video_coding/codecs/h264/include/h264_globals.h https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:75: explicit BitstreamThresholds(size_t max_nalu_length) Not sure if the ctor serves a purpose together with the rtc::Optional? If an object of this type is instantiated, it must necessarily have |max_nalu_length| set? https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:202: void VerifyBitstream(int frame_number, On 2017/09/25 16:13:41, hbos wrote: > Does it make more sense for frame_number to be size_t? It's used as an index and > should never be negative, right? You are right, but the frame numbers are ints in the rest of the test. Could be a followup to fix though. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc:56: TEST_F(VideoProcessorIntegrationTestOpenH264, Process0PercentPacketLossH264) { Not your change, but you could take the opportunity to remove 'H264' from the test name, since it's part of the test fixture name ('OpenH264'). https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc:76: TEST_F(VideoProcessorIntegrationTestOpenH264, ProcessNoLossSingleNalUnitH264) { 'ProcessNoLossSingleNalUnit' (see above).
https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/stats.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/stats.h:34: rtc::Optional<size_t> max_nalu_length; On 2017/09/26 09:19:13, brandtr wrote: > Maybe add a comment that this is only relevant for H.264? (Hence the > rtc::Optional.) All other members are codec-agnostic. Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:19: #include <algorithm> On 2017/09/26 09:19:13, brandtr wrote: > Also sort the include order here. Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:113: const TestConfig& config) { On 2017/09/26 09:19:13, brandtr wrote: > To fix the indentation, you can run 'git cl format' in the checkout directory. > That formats all the code that you have touched according to the Google C++ > style guide. Great. I didn't know about that command. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:121: size_t maxLength = 0; On 2017/09/26 09:19:13, brandtr wrote: > We typically use snake_case for naming local variables. Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.h:21: #include "common_video/h264/h264_common.h" On 2017/09/26 09:19:13, brandtr wrote: > Reorder with line 20, to have the includes in alphabetical order. I think the > presubmit will complain otherwise. Moved h264_common.h to videoprocessor.cc since h264_globals.h has been added here. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.h:107: H264PacketizationMode packetization_mode = On 2017/09/26 09:19:13, brandtr wrote: > Add explicit include to > webrtc/modules/video_coding/codecs/h264/include/h264_globals.h Done. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.h:75: explicit BitstreamThresholds(size_t max_nalu_length) On 2017/09/26 09:19:13, brandtr wrote: > Not sure if the ctor serves a purpose together with the rtc::Optional? If an > object of this type is instantiated, it must necessarily have |max_nalu_length| > set? Agree. No need in rtc::Otional here. Changed to size_t. https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc:56: TEST_F(VideoProcessorIntegrationTestOpenH264, Process0PercentPacketLossH264) { On 2017/09/26 09:19:13, brandtr wrote: > Not your change, but you could take the opportunity to remove 'H264' from the > test name, since it's part of the test fixture name ('OpenH264'). Removed https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest_openh264.cc:76: TEST_F(VideoProcessorIntegrationTestOpenH264, ProcessNoLossSingleNalUnitH264) { On 2017/09/26 09:19:13, brandtr wrote: > 'ProcessNoLossSingleNalUnit' (see above). Done.
Nice! Added one more comment. https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:124: max_length = std::max(max_length, index.payload_size); This indentation doesn't look right. https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:577: const BitstreamThresholds* bs_thresholds) { Since this method is always called with a non-null |bs_thresholds|, you could change it to take a const ref instead. https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:580: EXPECT_LE(*(frame_stat->max_nalu_length), bs_thresholds->max_nalu_length); This is a bit dangerous: whenever |bs_thresholds| is non-null, we expect that |max_nalu_length| is set. But this will not be the case for non-H264 encoders right now. Maybe add a RTC_DCHECK(frame_stat->max_nalu_length) above to make this assumption clear here. Alternatively, gate the comparison using a conditional on the codec_type.
On 2017/09/26 11:20:56, brandtr wrote: > Nice! Added one more comment. > > https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... > File modules/video_coding/codecs/test/videoprocessor.cc (right): > > https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor.cc:124: max_length = > std::max(max_length, index.payload_size); > This indentation doesn't look right. > > https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... > File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): > > https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:577: const > BitstreamThresholds* bs_thresholds) { > Since this method is always called with a non-null |bs_thresholds|, you could > change it to take a const ref instead. > > https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... > modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:580: > EXPECT_LE(*(frame_stat->max_nalu_length), bs_thresholds->max_nalu_length); > This is a bit dangerous: whenever |bs_thresholds| is non-null, we expect that > |max_nalu_length| is set. But this will not be the case for non-H264 encoders > right now. Maybe add a RTC_DCHECK(frame_stat->max_nalu_length) above to make > this assumption clear here. Alternatively, gate the comparison using a > conditional on the codec_type. lgtm after offline discussion about RTC_DCHECK being used inside the optional implementation.
https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:124: max_length = std::max(max_length, index.payload_size); On 2017/09/26 11:20:55, brandtr wrote: > This indentation doesn't look right. Fixed https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:577: const BitstreamThresholds* bs_thresholds) { On 2017/09/26 11:20:55, brandtr wrote: > Since this method is always called with a non-null |bs_thresholds|, you could > change it to take a const ref instead. Right. Done. https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:580: EXPECT_LE(*(frame_stat->max_nalu_length), bs_thresholds->max_nalu_length); On 2017/09/26 11:20:55, brandtr wrote: > This is a bit dangerous: whenever |bs_thresholds| is non-null, we expect that > |max_nalu_length| is set. But this will not be the case for non-H264 encoders > right now. Maybe add a RTC_DCHECK(frame_stat->max_nalu_length) above to make > this assumption clear here. Alternatively, gate the comparison using a > conditional on the codec_type. Such check is done within rtc:Optional
PTAL
lgtm. Optional: if you want to land this before updating OpenH264 you can make it a DEATH test, see https://cs.chromium.org/chromium/src/third_party/webrtc/stats/rtcstats_unitte... that way you can expect it to crash in ProcessFramesAndMaybeVerify, and add a TODO saying it should not crash once OpenH264 is updated. That way you can't update OpenH264 without enabling the test. https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); Should we return empty optional if nalu_indices is empty or 0?
PTAL https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/27 15:56:42, hbos wrote: > Should we return empty optional if nalu_indices is empty or 0? Added RTC_CHECK for empty nalu_indices. It is expected that output buffer contains at least 1 NALU.
https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/28 09:53:09, ssilkin wrote: > On 2017/09/27 15:56:42, hbos wrote: > > Should we return empty optional if nalu_indices is empty or 0? > > Added RTC_CHECK for empty nalu_indices. It is expected that output buffer > contains at least 1 NALU. Is it safe to assume the encoded image is "correct" at this point? Imagine me trying to crash your endpoint by sending broken frames.
https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/28 12:08:34, hbos wrote: > On 2017/09/28 09:53:09, ssilkin wrote: > > On 2017/09/27 15:56:42, hbos wrote: > > > Should we return empty optional if nalu_indices is empty or 0? > > > > Added RTC_CHECK for empty nalu_indices. It is expected that output buffer > > contains at least 1 NALU. > > Is it safe to assume the encoded image is "correct" at this point? Imagine me > trying to crash your endpoint by sending broken frames. Or is this only on frames we've encoded?
https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/28 12:10:45, hbos wrote: > On 2017/09/28 12:08:34, hbos wrote: > > On 2017/09/28 09:53:09, ssilkin wrote: > > > On 2017/09/27 15:56:42, hbos wrote: > > > > Should we return empty optional if nalu_indices is empty or 0? > > > > > > Added RTC_CHECK for empty nalu_indices. It is expected that output buffer > > > contains at least 1 NALU. > > > > Is it safe to assume the encoded image is "correct" at this point? Imagine me > > trying to crash your endpoint by sending broken frames. > > Or is this only on frames we've encoded? This is just test code, so I think it's OK to be liberal about using CHECKs here. The reason that we have used CHECKs (instead of DCHECKs) in other parts of this class is that we generally want to run it in Release mode, for performance reasons. So to save on testing iterations (the Chromium test runner is a bit slow getting started sometimes), we have used CHECKs...
yes, we encode and analyze the output in this test. no broken frames expected.
The CQ bit was checked by ssilkin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, hbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/3014623002/#ps100001 (title: "added check for empty nalu_indices")
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: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/24399) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/24119) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/28762)
The CQ bit was checked by ssilkin@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/...
The CQ bit was unchecked by ssilkin@webrtc.org
PTAL, brandtr I updated deps for video_codecs_test_framework in BUILD.gn: added codec_globals_headers.
lgtm!
The CQ bit was checked by ssilkin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/3014623002/#ps120001 (title: "Added codec_globals_headers to deps of video_codecs_test_framework")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1506614233449710, "parent_rev": "0cbaf1a6f6ad13a25993f6ea3be931894a196834", "commit_rev": "612f858ba0aa35c2993c4510b8ec5d3f8a5b70bf"}
Message was sent while issue was closed.
Description was changed from ========== Adding test for SingleNalUnit mode Test enables single-nalu mode, sets limit for nalu lenght and verifies that encoder follows that limit. I found that QP jumps significantly when the mode is enabled. In result encoder might produce 4kbyte and 0.4kbyte frames back-to-back. But it seems that happens only to couple of frames in the beginning. This caused test to fail with default RC thresholds. To bypass this I increased frame size mismatch threshold from 20 to 30%. This should be Ok considering single-nalu mode is rare. BUG=webrtc:8070 ========== to ========== Adding test for SingleNalUnit mode Test enables single-nalu mode, sets limit for nalu lenght and verifies that encoder follows that limit. I found that QP jumps significantly when the mode is enabled. In result encoder might produce 4kbyte and 0.4kbyte frames back-to-back. But it seems that happens only to couple of frames in the beginning. This caused test to fail with default RC thresholds. To bypass this I increased frame size mismatch threshold from 20 to 30%. This should be Ok considering single-nalu mode is rare. BUG=webrtc:8070 Review-Url: https://codereview.webrtc.org/3014623002 Cr-Commit-Position: refs/heads/master@{#20023} Committed: https://webrtc.googlesource.com/src/+/612f858ba0aa35c2993c4510b8ec5d3f8a5b70bf ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://webrtc.googlesource.com/src/+/612f858ba0aa35c2993c4510b8ec5d3f8a5b70bf |