Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710913004:
Step #3: Add flag for correctness mode in VideoProcessor integration tests. (Closed)
DescriptionAdd option to disable EXPECT_EQ's in VideoProcessor integration tests.
Since HW codecs are not as well-behaved as SW codecs, we need a
way to disable the EXPECT_EQ's in the VideoProcessor integration tests
for the former. This CL introduces such an ability.
BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/2710913004
Cr-Commit-Position: refs/heads/master@{#17166}
Committed: https://chromium.googlesource.com/external/webrtc/+/60dcda4943d4b20bd6f167468cbbf99135d6c17a
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Rebase. #Patch Set 4 : Rebase. #Patch Set 5 : Nit. #
Total comments: 3
Patch Set 6 : Rebase. #Patch Set 7 : asapersson comments 1. #
Total comments: 1
Patch Set 8 : Rebase. #
Total comments: 6
Patch Set 9 : sprang comments 1. #
Total comments: 3
Patch Set 10 : asapersson comments 2. #Patch Set 11 : Rebase. #Patch Set 12 : asapersson comments 3 (offline). #
Dependent Patchsets: Messages
Total messages: 31 (11 generated)
Description was changed from ========== Add flag for correctness mode in VideoProcessor integration tests. Since HW codecs are not as well-behaved as SW codecs, we need a way to disable the EXPECTs in the VideoProcessor integration tests for the former. This CL introduces such a flag. Correctness mode should only be disabled when doing manual experimentation on devices. It should NOT be disabled on the bots. BUG=webrtc:6634 ========== to ========== Add flag for correctness mode in VideoProcessor integration tests. Since HW codecs are not as well-behaved as SW codecs, we need a way to disable the EXPECTs in the VideoProcessor integration tests for the former. This CL introduces such a flag. Correctness mode should only be disabled when doing manual experimentation on devices. It should NOT be disabled on the bots. BUG=webrtc:6634 ==========
Rebase.
Rebase.
Rebase.
Nit.
brandtr@webrtc.org changed reviewers: + asapersson@webrtc.org, sprang@webrtc.org
https://codereview.webrtc.org/2710913004/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/2710913004/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:76: SetQualityThresholds(&quality_thresholds, 15.0, 10.0, 0.2, 0.1); Maybe the thresholds could be configured to some value that will always pass (e.g. zero for the quality thresholds)? The quality thresholds could maybe be initialized to 0 and the call to SetQualityThresholds removed.
https://codereview.webrtc.org/2710913004/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/2710913004/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:76: SetQualityThresholds(&quality_thresholds, 15.0, 10.0, 0.2, 0.1); On 2017/03/09 11:48:57, åsapersson wrote: > Maybe the thresholds could be configured to some value that will always pass > (e.g. zero for the quality thresholds)? The quality thresholds could maybe be > initialized to 0 and the call to SetQualityThresholds removed. Yes, I considered that. But I would still have to disable these EXPECT_EQ's: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... What I could do instead is to implicitly disable the EXPECT_GE/EXPECT_LE's by setting the thresholds appropriately low/high, and then explicitly disable the EXPECT_EQ's by setting the corresponding threshold to -1, and then just check that locally. That is probably a cleaner solution, what do you think?
https://codereview.webrtc.org/2710913004/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/2710913004/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:76: SetQualityThresholds(&quality_thresholds, 15.0, 10.0, 0.2, 0.1); On 2017/03/09 12:07:02, brandtr wrote: > On 2017/03/09 11:48:57, åsapersson wrote: > > Maybe the thresholds could be configured to some value that will always pass > > (e.g. zero for the quality thresholds)? The quality thresholds could maybe be > > initialized to 0 and the call to SetQualityThresholds removed. > > Yes, I considered that. But I would still have to disable these EXPECT_EQ's: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... > > What I could do instead is to implicitly disable the EXPECT_GE/EXPECT_LE's by > setting the thresholds appropriately low/high, and then explicitly disable the > EXPECT_EQ's by setting the corresponding threshold to -1, and then just check > that locally. That is probably a cleaner solution, what do you think? Sounds good to me.
Rebase.
Description was changed from ========== Add flag for correctness mode in VideoProcessor integration tests. Since HW codecs are not as well-behaved as SW codecs, we need a way to disable the EXPECTs in the VideoProcessor integration tests for the former. This CL introduces such a flag. Correctness mode should only be disabled when doing manual experimentation on devices. It should NOT be disabled on the bots. BUG=webrtc:6634 ========== to ========== Add option to disable EXPECT_EQ's in VideoProcessor integration tests. Since HW codecs are not as well-behaved as SW codecs, we need a way to disable the EXPECT_EQ's in the VideoProcessor integration tests for the former. This CL introduces such an ability. BUG=webrtc:6634 ==========
Changed implementation. https://codereview.webrtc.org/2710913004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/2710913004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:86: 1000, // max_key_frame_size_mismatch These values are arbitrarily selected. Typically for a Nexus 6, I see mismatches < 500%, so I just 2x'd that :)
Rebase.
lgtm https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:80: RateControlThresholds rc_thresholds[1]; nit: Not your change, but it looks like this doesn't need to be a vector? https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:85: kNumFramesLong + 1, // max_num_dropped_frames, nit: that comma bugs me :) https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:97: // Thresholds for the quality metrics. Defaults are maximally loose. Maximally minimal!
https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:80: RateControlThresholds rc_thresholds[1]; On 2017/03/10 09:25:10, språng wrote: > nit: Not your change, but it looks like this doesn't need to be a vector? It needs to be here: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... It would be nice to verify that the user of the test is sending something sensible though; I added a TODO for that. https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_videoprocessor_integrationtest.cc:85: kNumFramesLong + 1, // max_num_dropped_frames, On 2017/03/10 09:25:10, språng wrote: > nit: that comma bugs me :) Me too, now that I see it :) https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/2710913004/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:97: // Thresholds for the quality metrics. Defaults are maximally loose. On 2017/03/10 09:25:10, språng wrote: > Maximally minimal! :)
https://codereview.webrtc.org/2710913004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/2710913004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:478: if (rc_expected.num_spatial_resizes > 0) { >=?
https://codereview.webrtc.org/2710913004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/2710913004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:478: if (rc_expected.num_spatial_resizes > 0) { On 2017/03/10 11:12:34, åsapersson wrote: > >=? Yes, thanks. https://codereview.webrtc.org/2710913004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:481: if (rc_expected.num_key_frames > 0) { Changed here too, in case we don't expect a key frame for some rate update period.
Rebase.
Updated thresholds, as discussed offline.
lgtm
The CQ bit was checked by brandtr@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: This issue passed the CQ dry run.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2710913004/#ps220001 (title: "asapersson comments 3 (offline).")
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": 220001, "attempt_start_ts": 1489152718965470,
"parent_rev": "2ad5e38709bab33843635cc8f720a5b6ff71d52b", "commit_rev":
"60dcda4943d4b20bd6f167468cbbf99135d6c17a"}
Message was sent while issue was closed.
Description was changed from ========== Add option to disable EXPECT_EQ's in VideoProcessor integration tests. Since HW codecs are not as well-behaved as SW codecs, we need a way to disable the EXPECT_EQ's in the VideoProcessor integration tests for the former. This CL introduces such an ability. BUG=webrtc:6634 ========== to ========== Add option to disable EXPECT_EQ's in VideoProcessor integration tests. Since HW codecs are not as well-behaved as SW codecs, we need a way to disable the EXPECT_EQ's in the VideoProcessor integration tests for the former. This CL introduces such an ability. BUG=webrtc:6634 Review-Url: https://codereview.webrtc.org/2710913004 Cr-Commit-Position: refs/heads/master@{#17166} Committed: https://chromium.googlesource.com/external/webrtc/+/60dcda4943d4b20bd6f167468... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/60dcda4943d4b20bd6f167468... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
