Reland of "Added large room scenario to full-stack tests

Reland of "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, webrtc:7301
Review-Url: https://codereview.webrtc.org/2733943003
Cr-Commit-Position: refs/heads/master@{#17092}
Committed: https://chromium.googlesource.com/external/webrtc/+/a014cc5eb1f2a562ac31322797967a5336c92f8c

[ilnik, 2017-03-07 09:21:47]

Description was changed from

==========
Reland of issue 2730073002:

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

==========
Reland of issue 2730073002: 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, 2017-03-07 09:27:35]

ilnik@webrtc.org changed reviewers:
+ kjellander@webrtc.org, sprang@webrtc.org

[ilnik, 2017-03-07 09:27:35]

Henrik, please check only call_test.h/cc. There are only cosmetic changes (added
some empty lines).

[kjellander_webrtc, 2017-03-07 09:39:27]

https://codereview.webrtc.org/2733943003/diff/20001/webrtc/video/full_stack_t...
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2733943003/diff/20001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:18: // Fails on Android and win
Please track fixing this in a bug that you add a reference to here. Not tracking
this in a bug will likely lead to this staying disabled forever for these
platforms.

If Android is too weak, could you consider reducing the size down from 1280x720
to VGA (or small) instead of disabling the test entirely? Or will 50 thumbnails
always be too many for Android?

For Windows, I assume the HD version with 50 thumbnails must be possible to fix,
right?

https://codereview.webrtc.org/2733943003/diff/20001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:21: #define MAYBE_FullStackTest FullStackTest
If you're only looking for disabling a single test case, you should add the
MAYBE_ pattern to the test case name instead:

#if defined(WEBRTC_ANDROID) || defined(WEBRTC_WIN)
#define MAYBE_LargeRoomVP8_50thumb DISABLED_LargeRoomVP8_50thumb
#else
#define MAYBE_LargeRoomVP8_50thumb LargeRoomVP8_50thumb

And move it to line 600.

[kjellander_webrtc, 2017-03-07 09:39:59]

Please make the title more descriptive, like 
Reland "Added large room scenario to full-stack tests."

[kjellander_webrtc, 2017-03-07 09:40:40]

On 2017/03/07 09:39:59, kjellander_webrtc wrote:
> Please make the title more descriptive, like 
> Reland "Added large room scenario to full-stack tests."

(if you revert the revert-CL you get a good template CL created that you can
patch into your local branch, then update with the fixes and upload a second
patch set).

[ilnik, 2017-03-07 09:43:43]

On 2017/03/07 09:40:40, kjellander_webrtc wrote:
> On 2017/03/07 09:39:59, kjellander_webrtc wrote:
> > Please make the title more descriptive, like 
> > Reland "Added large room scenario to full-stack tests."
> 
> (if you revert the revert-CL you get a good template CL created that you can
> patch into your local branch, then update with the fixes and upload a second
> patch set).

Wow, that's very nice way to do it. Thanks! Will use that in the future.

[ilnik, 2017-03-07 09:54:07]

https://codereview.webrtc.org/2733943003/diff/20001/webrtc/video/full_stack_t...
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2733943003/diff/20001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:18: // Fails on Android and win
On 2017/03/07 09:39:27, kjellander_webrtc wrote:
> Please track fixing this in a bug that you add a reference to here. Not
tracking
> this in a bug will likely lead to this staying disabled forever for these
> platforms.
> 
> If Android is too weak, could you consider reducing the size down from
1280x720
> to VGA (or small) instead of disabling the test entirely? Or will 50
thumbnails
> always be too many for Android?
> 
> For Windows, I assume the HD version with 50 thumbnails must be possible to
fix,
> right? 

50 thumbnails will be too large even without main HD feed. It consumes 6 full
cores on my workstation and around 4GB of memory. But it would be nice to have
such huge test. Why it fails on Win I don't know yet. maybe buildbot doesn't
have enough computing power or memory.

Added a bug for it.

https://codereview.webrtc.org/2733943003/diff/20001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:21: #define MAYBE_FullStackTest FullStackTest
On 2017/03/07 09:39:27, kjellander_webrtc wrote:
> If you're only looking for disabling a single test case, you should add the
> MAYBE_ pattern to the test case name instead:
> 
> #if defined(WEBRTC_ANDROID) || defined(WEBRTC_WIN)
> #define MAYBE_LargeRoomVP8_50thumb DISABLED_LargeRoomVP8_50thumb
> #else
> #define MAYBE_LargeRoomVP8_50thumb LargeRoomVP8_50thumb
> 
> And move it to line 600.

Done.

[sprang_webrtc, 2017-03-07 10:14:58]

On 2017/03/07 09:54:07, ilnik wrote:

> 50 thumbnails will be too large even without main HD feed. It consumes 6 full
> cores on my workstation and around 4GB of memory. But it would be nice to have
> such huge test. Why it fails on Win I don't know yet. maybe buildbot doesn't
> have enough computing power or memory.
> 

Wow, that feels like something we do need to track from a perf standpoint then
:)

[sprang_webrtc, 2017-03-07 10:15:05]

lgtm

[ilnik, 2017-03-07 10:36:32]

On 2017/03/07 10:15:05, språng wrote:
> lgtm

Henrik, I still need an lgtm for call_test.* files as Erik doesn't have
ownership there.

[kjellander_webrtc, 2017-03-07 12:02:25]

Description was changed from

==========
Reland of issue 2730073002: 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

==========
Reland of issue 2730073002: 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, webrtc:7301
==========

[ilnik, 2017-03-07 12:02:43]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 12:02:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kjellander_webrtc, 2017-03-07 12:03:04]

Description was changed from

==========
Reland of issue 2730073002: 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, webrtc:7301
==========

to

==========
Reland of "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, webrtc:7301
==========

[kjellander_webrtc, 2017-03-07 12:05:42]

Description was changed from

==========
Reland of "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, webrtc:7301
==========

to

==========
Reland of "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, webrtc:7301
==========

[kjellander_webrtc, 2017-03-07 12:06:19]

On 2017/03/07 10:36:32, ilnik wrote:
> On 2017/03/07 10:15:05, språng wrote:
> > lgtm
> 
> Henrik, I still need an lgtm for call_test.* files as Erik doesn't have
> ownership there.

You're only adding two blank lines in webrtc/test so you could easily work
around that :) Actually I'd prefer you revert those changes.

lgtm either way. I also added webrtc:7301 to the BUG= field in the description,
shortened the title and wrapped the description a little to alighn better with
Git recommendations: https://chris.beams.io/posts/git-commit/

[ilnik, 2017-03-07 12:08:44]

The CQ bit was unchecked by ilnik@webrtc.org

[ilnik, 2017-03-07 12:10:08]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-03-07 12:10:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-07 12:21:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488888608823220,
"parent_rev": "d461ffce2ac65ceaa6e6e4bb8d1abe2b0be87bd7", "commit_rev":
"a014cc5eb1f2a562ac31322797967a5336c92f8c"}

[commit-bot: I haz the power, 2017-03-07 12:21:08]

Description was changed from

==========
Reland of "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, webrtc:7301
==========

to

==========
Reland of "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, webrtc:7301

Review-Url: https://codereview.webrtc.org/2733943003
Cr-Commit-Position: refs/heads/master@{#17092}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a014cc5eb1f2a562ac3132279...
==========

[commit-bot: I haz the power, 2017-03-07 12:21:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/a014cc5eb1f2a562ac3132279...