Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(301)

Issue 3014623002: Adding test for SingleNalUnit mode (Closed)

Created:
3 years, 2 months ago by ssilkin
Modified:
3 years, 2 months ago
CC:
webrtc-reviews_webrtc.org
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

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

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)
ssilkin
copied from https://codereview.webrtc.org/3011373002/#msg28 On 2017/09/25 11:02:35, hbos wrote: > Sorry for the delayed review. > ...
3 years, 2 months ago (2017-09-25 15:06:48 UTC) #5
ssilkin
copied from https://codereview.webrtc.org/3011373002/#msg28 On 2017/09/25 11:02:35, hbos wrote: > Sorry for the delayed review. > ...
3 years, 2 months ago (2017-09-25 15:06:50 UTC) #6
ssilkin
3 years, 2 months ago (2017-09-25 15:07:34 UTC) #8
hbos
Nice. Please update the CL description (and title), I wrote some guidelines in the other ...
3 years, 2 months ago (2017-09-25 16:13:41 UTC) #9
ssilkin
Test fails because of v1.6.
3 years, 2 months ago (2017-09-26 09:17:12 UTC) #15
ssilkin
https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/codecs/test/videoprocessor_integrationtest.cc File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/codecs/test/videoprocessor_integrationtest.cc#newcode345 modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:345: codec.SetParam(cricket::kH264FmtpPacketizationMode, "0"); On 2017/09/25 16:13:41, hbos wrote: > If ...
3 years, 2 months ago (2017-09-26 09:17:52 UTC) #16
brandtr
Looks good! I added some comments. It's a useful test to have, and it would ...
3 years, 2 months ago (2017-09-26 09:19:13 UTC) #17
ssilkin
https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/codecs/test/stats.h File modules/video_coding/codecs/test/stats.h (right): https://codereview.webrtc.org/3014623002/diff/20001/modules/video_coding/codecs/test/stats.h#newcode34 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 ...
3 years, 2 months ago (2017-09-26 11:07:05 UTC) #18
brandtr
Nice! Added one more comment. https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/codecs/test/videoprocessor.cc File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/codecs/test/videoprocessor.cc#newcode124 modules/video_coding/codecs/test/videoprocessor.cc:124: max_length = std::max(max_length, index.payload_size); ...
3 years, 2 months ago (2017-09-26 11:20:56 UTC) #19
brandtr
On 2017/09/26 11:20:56, brandtr wrote: > Nice! Added one more comment. > > https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/codecs/test/videoprocessor.cc > ...
3 years, 2 months ago (2017-09-26 11:52:09 UTC) #20
ssilkin
https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/codecs/test/videoprocessor.cc File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/60001/modules/video_coding/codecs/test/videoprocessor.cc#newcode124 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: ...
3 years, 2 months ago (2017-09-26 12:05:33 UTC) #21
ssilkin
PTAL
3 years, 2 months ago (2017-09-27 14:40:30 UTC) #22
hbos
lgtm. Optional: if you want to land this before updating OpenH264 you can make it ...
3 years, 2 months ago (2017-09-27 15:56:43 UTC) #23
ssilkin
PTAL https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc#newcode120 modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/27 15:56:42, hbos wrote: > Should ...
3 years, 2 months ago (2017-09-28 09:53:09 UTC) #24
hbos
https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc#newcode120 modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/28 09:53:09, ssilkin wrote: > On 2017/09/27 ...
3 years, 2 months ago (2017-09-28 12:08:34 UTC) #25
hbos
https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc#newcode120 modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/28 12:08:34, hbos wrote: > On 2017/09/28 ...
3 years, 2 months ago (2017-09-28 12:10:45 UTC) #26
brandtr
https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3014623002/diff/80001/modules/video_coding/codecs/test/videoprocessor.cc#newcode120 modules/video_coding/codecs/test/videoprocessor.cc:120: encoded_frame._length); On 2017/09/28 12:10:45, hbos wrote: > On 2017/09/28 ...
3 years, 2 months ago (2017-09-28 13:39:26 UTC) #27
ssilkin
yes, we encode and analyze the output in this test. no broken frames expected.
3 years, 2 months ago (2017-09-28 13:45:30 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3014623002/100001
3 years, 2 months ago (2017-09-28 15:12:51 UTC) #31
commit-bot: I haz the power
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, ...
3 years, 2 months ago (2017-09-28 15:14:42 UTC) #33
ssilkin
PTAL, brandtr I updated deps for video_codecs_test_framework in BUILD.gn: added codec_globals_headers.
3 years, 2 months ago (2017-09-28 15:39:54 UTC) #37
brandtr
lgtm!
3 years, 2 months ago (2017-09-28 15:43:19 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3014623002/120001
3 years, 2 months ago (2017-09-28 15:57:21 UTC) #41
hbos
lgtm
3 years, 2 months ago (2017-09-28 16:22:39 UTC) #42
commit-bot: I haz the power
3 years, 2 months ago (2017-09-28 16:23:27 UTC) #45
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://webrtc.googlesource.com/src/+/612f858ba0aa35c2993c4510b8ec5d3f8a5b70bf

Powered by Google App Engine
This is Rietveld 408576698