Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2730073002:
Added large room scenario to full-stack tests. Added thumbnail streams functionality to call test/v… (Closed)
DescriptionAdded large room scenario to full-stack tests. Added thumbnail streams functionality to video quality test.
Changed simulcast full-stack tests to be 30fps instead of 50 to better reflect real usecases (expect all kind of perf metrics to improve).
BUG=webrtc:7095
Review-Url: https://codereview.webrtc.org/2730073002
Cr-Commit-Position: refs/heads/master@{#17068}
Committed: https://chromium.googlesource.com/external/webrtc/+/d8bd1b1d82aa0c1ceec901e99e1f30f9ed58cb77
Patch Set 1 #Patch Set 2 : Fixing nits #Patch Set 3 : fixing win CE #Patch Set 4 : Moving all of thumbnails out of call test #
Total comments: 10
Patch Set 5 : Implemented Sprang@ comments. #
Total comments: 1
Patch Set 6 : Made thumbnail streams to be send in the opposite way. Cleaned up CallTest modifications as they ar… #Patch Set 7 : implementing Sprang@ comments. #Patch Set 8 : rebase #Patch Set 9 : disabling too huge test everythere except for linux. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 46 (24 generated)
The CQ bit was checked by ilnik@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 ilnik@webrtc.org
The CQ bit was checked by ilnik@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: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/6989)
Description was changed from ========== Added large room scenario to full-stack tests. Added thumbnail streams functionality to call test/video quality test. Changed simulcast full-stack tests to be 30fps instead of 50 (expect all kind of perf metrics to improve). BUG=webrtc:7095 ========== to ========== Added large room scenario to full-stack tests. Added thumbnail streams functionality to video quality test. Changed simulcast full-stack tests to be 30fps instead of 50 to better reflect real usecases (expect all kind of perf metrics to improve). BUG=webrtc:7095 ==========
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org
ilnik@webrtc.org changed reviewers: + kjellander@webrtc.org
The CQ bit was checked by ilnik@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.
https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.h (left): https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:82: // (*) Set to -1.1 if generating graph data for simulcast or SVC and the Why remove this? Is it not correct?
https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.h (left): https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:82: // (*) Set to -1.1 if generating graph data for simulcast or SVC and the On 2017/03/06 12:02:44, kjellander_webrtc wrote: > Why remove this? Is it not correct? Not relevant anymore since now upscaling is implemented for PSNR and SSIM computation.
lgtm, but I'm nor owner of knows this code very well.
On 2017/03/06 12:19:24, kjellander_webrtc wrote: > lgtm, but I'm nor owner of knows this code very well. of -> or
On 2017/03/06 12:19:48, kjellander_webrtc wrote: > On 2017/03/06 12:19:24, kjellander_webrtc wrote: > > lgtm, but I'm nor owner of knows this code very well. > > of -> or Thank you. You are an owner of webrtc/test/*. Which Erik is not. Your review is crucial.
Looks good! Could we make the first thumbnail stream larger though? To simulate the main receive-stream... Can use the same settings as the thumbnails in other regards though. https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1585: for (size_t i = 0; i < thumbnail_send_streams_.size(); ++i) nit: {} for multi-line statements https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1630: // thumbnails are not managed by CallTest. We need to destroy them manually. nit: capital T https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.h (right): https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:127: std::vector<std::unique_ptr<test::VideoCapturer> > thumbnail_capturers_; nit: Don't actually need the space in >> anymore https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:134: std::vector<VideoSendStream*> thumbnail_send_streams_; The vectors need to match, right? Can we use a single vector of struct for these instead?
https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.h (right): https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:127: std::vector<std::unique_ptr<test::VideoCapturer> > thumbnail_capturers_; On 2017/03/06 12:27:23, språng wrote: > nit: Don't actually need the space in >> anymore Done. https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:134: std::vector<VideoSendStream*> thumbnail_send_streams_; On 2017/03/06 12:27:23, språng wrote: > The vectors need to match, right? Can we use a single vector of struct for these > instead? They match by how they are created. I don't think merging these together will justify reduced readability.
On 2017/03/06 12:27:23, språng wrote: > Looks good! > Could we make the first thumbnail stream larger though? To simulate the main > receive-stream... Can use the same settings as the thumbnails in other regards > though. > What do you mean by "to simulate the main receive stream"? We already have a main receive stream at 720p@30fps.
https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1585: for (size_t i = 0; i < thumbnail_send_streams_.size(); ++i) On 2017/03/06 12:27:23, språng wrote: > nit: {} for multi-line statements Done. https://codereview.webrtc.org/2730073002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1630: // thumbnails are not managed by CallTest. We need to destroy them manually. On 2017/03/06 12:27:23, språng wrote: > nit: capital T Done.
https://codereview.webrtc.org/2730073002/diff/80001/webrtc/video/full_stack_t... File webrtc/video/full_stack_tests.cc (right): https://codereview.webrtc.org/2730073002/diff/80001/webrtc/video/full_stack_t... webrtc/video/full_stack_tests.cc:588: large_room.num_thumbnails = 20; Can we bump this to 50, so we _really_ stress things?
The CQ bit was checked by ilnik@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/...
On 2017/03/06 12:53:26, språng wrote: > https://codereview.webrtc.org/2730073002/diff/80001/webrtc/video/full_stack_t... > File webrtc/video/full_stack_tests.cc (right): > > https://codereview.webrtc.org/2730073002/diff/80001/webrtc/video/full_stack_t... > webrtc/video/full_stack_tests.cc:588: large_room.num_thumbnails = 20; > Can we bump this to 50, so we _really_ stress things? Now thumbnails are received by sender_call to better reflect real-life usage. Unfortunately, sending and receiving main feed at the same call is not easy to do. Simple way would affect all other full-stack tests. Alternative would be to separate large-room scenario from other full-stack tests, but it will create a third similar to RunWith... funcyion in VideoQualityTest, from which we, apparently, trying to move away. Made it 50 thumbnails. Somehow, it works. Although, consuming 536% of CPU.
On 2017/03/06 13:36:54, ilnik wrote: > On 2017/03/06 12:53:26, språng wrote: > > > https://codereview.webrtc.org/2730073002/diff/80001/webrtc/video/full_stack_t... > > File webrtc/video/full_stack_tests.cc (right): > > > > > https://codereview.webrtc.org/2730073002/diff/80001/webrtc/video/full_stack_t... > > webrtc/video/full_stack_tests.cc:588: large_room.num_thumbnails = 20; > > Can we bump this to 50, so we _really_ stress things? > > Now thumbnails are received by sender_call to better reflect real-life usage. > Unfortunately, sending and receiving main feed at the same call is not easy to > do. Simple way would affect all other full-stack tests. Alternative would be to > separate large-room scenario from other full-stack tests, but it will create a > third similar to RunWith... funcyion in VideoQualityTest, from which we, > apparently, trying to move away. Fair enough, let's go with this setup. > Made it 50 thumbnails. Somehow, it works. Although, consuming 536% of CPU. :D
lgtm
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2730073002/#ps140001 (title: "rebase")
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": 140001, "attempt_start_ts": 1488808139356720,
"parent_rev": "a2ef4f94e478ac1eb8b3be47e63cdcd56cf66207", "commit_rev":
"d8bd1b1d82aa0c1ceec901e99e1f30f9ed58cb77"}
Message was sent while issue was closed.
Description was changed from ========== Added large room scenario to full-stack tests. Added thumbnail streams functionality to video quality test. Changed simulcast full-stack tests to be 30fps instead of 50 to better reflect real usecases (expect all kind of perf metrics to improve). BUG=webrtc:7095 ========== to ========== Added large room scenario to full-stack tests. Added thumbnail streams functionality to video quality test. Changed simulcast full-stack tests to be 30fps instead of 50 to better reflect real usecases (expect all kind of perf metrics to improve). BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2730073002 Cr-Commit-Position: refs/heads/master@{#17068} Committed: https://chromium.googlesource.com/external/webrtc/+/d8bd1b1d82aa0c1ceec901e99... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/d8bd1b1d82aa0c1ceec901e99...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.webrtc.org/2734753004/ by ilnik@webrtc.org. The reason for reverting is: webrtc_perf_tests crashes on android and windows due to too large test..
Message was sent while issue was closed.
Description was changed from ========== Added large room scenario to full-stack tests. Added thumbnail streams functionality to video quality test. Changed simulcast full-stack tests to be 30fps instead of 50 to better reflect real usecases (expect all kind of perf metrics to improve). BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2730073002 Cr-Commit-Position: refs/heads/master@{#17068} Committed: https://chromium.googlesource.com/external/webrtc/+/d8bd1b1d82aa0c1ceec901e99... ========== to ========== Added large room scenario to full-stack tests. Added thumbnail streams functionality to video quality test. Changed simulcast full-stack tests to be 30fps instead of 50 to better reflect real usecases (expect all kind of perf metrics to improve). BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2730073002 Cr-Commit-Position: refs/heads/master@{#17068} Committed: https://chromium.googlesource.com/external/webrtc/+/d8bd1b1d82aa0c1ceec901e99... ==========
On 2017/03/06 15:34:42, ilnik wrote: > A revert of this CL (patchset #8 id:140001) has been created in > https://codereview.webrtc.org/2734753004/ by mailto:ilnik@webrtc.org. > > The reason for reverting is: webrtc_perf_tests crashes on android and windows > due to too large test.. Uploaded a hotfix for recent buildbots failures. Erik, please take a look at it.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2730073002/#ps160001 (title: "disabling too huge test everythere except for linux.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please use a separate cl for the reland, with the first patch set being the originally landed commit. https://codereview.webrtc.org/2730073002/diff/160001/webrtc/video/full_stack_... File webrtc/video/full_stack_tests.cc (right): https://codereview.webrtc.org/2730073002/diff/160001/webrtc/video/full_stack_... webrtc/video/full_stack_tests.cc:593: #if defined(WEBRTC_LINUX) I think it would be better to just exclude problematic platforms. Android and iOS I guess? Also, you don't need to comment out the whole test, but can just update the name instead. As an example, see MAYBE_CallSessionFileRotatingStreamTest in webrtc/base/filerotatingstream_unittest.cc
The CQ bit was unchecked by ilnik@webrtc.org
On 2017/03/07 08:57:30, språng wrote: > Please use a separate cl for the reland, with the first patch set being the > originally landed commit. > > https://codereview.webrtc.org/2730073002/diff/160001/webrtc/video/full_stack_... > File webrtc/video/full_stack_tests.cc (right): > > https://codereview.webrtc.org/2730073002/diff/160001/webrtc/video/full_stack_... > webrtc/video/full_stack_tests.cc:593: #if defined(WEBRTC_LINUX) > I think it would be better to just exclude problematic platforms. Android and > iOS I guess? > > Also, you don't need to comment out the whole test, but can just update the name > instead. > As an example, see > MAYBE_CallSessionFileRotatingStreamTest in > webrtc/base/filerotatingstream_unittest.cc Ok. Doing that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
