Add FullStack test for simulcast screenshare mode.

Add FullStack test for simulcast screenshare mode.

BUG=webrtc:4172
Review-Url: https://codereview.webrtc.org/2745523002
Cr-Commit-Position: refs/heads/master@{#17150}
Committed: https://chromium.googlesource.com/external/webrtc/+/cb8c1467bdf2b90423781b811201b4cb1ea96dec

[ilnik, 2017-03-09 11:20:22]

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

[ilnik, 2017-03-09 11:20:23]

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

https://codereview.webrtc.org/2745523002/diff/20001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:326: false,   false, "",    ""};
Erik, does it make sense to make 3 temporal layers on the high stream and 2 on
the low stream? Also, I think selected TL will be ignored here and will be taken
from |screenshare.video|. So network transport will strip highest temporal layer
of the high stream and will not strip anything from the lowest stream (if we
happen to select it). Is it intended to strip half frames even on 5 fps? Also
why is it called 2TL if there are 3 temporal layers?

[sprang_webrtc, 2017-03-09 11:43:48]

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

https://codereview.webrtc.org/2745523002/diff/20001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:326: false,   false, "",    ""};
On 2017/03/09 11:20:23, ilnik wrote:
> Erik, does it make sense to make 3 temporal layers on the high stream and 2 on
> the low stream?
Yes, this is by design. The low stream keeps the current conference mode
settings, with 5fps and two temporal layers assigned based on produced bitrate
rather than a fixed pattern. The top layer uses the same temporal layer
allocation normal video and does not have a framerate restriction.

> Also, I think selected TL will be ignored here and will be taken
> from |screenshare.video|. So network transport will strip highest temporal
layer
> of the high stream and will not strip anything from the lowest stream (if we
> happen to select it). Is it intended to strip half frames even on 5 fps?

For this test case I did not intend any stripping, so should change this.

> Also
> why is it called 2TL if there are 3 temporal layers?

That probably just from copy/paste. Go ahead and change it.

Should probably also add a version of this with higher framerate.

[ilnik, 2017-03-09 13:21:01]

I fixed both timestamp mismatches and crash. Please take a look.

[sprang_webrtc, 2017-03-09 13:30:58]

lgtm

Feels weird to say that for the code I wrote though :)

+stefan as owner for common_video

[ilnik, 2017-03-09 13:34:13]

ilnik@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2017-03-09 14:07:46]

lgtm for common video.

I reviewed more of this by accident, but feel free to ignore :)

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_rate_allocator.cc:33:
RTC_DCHECK(layers != nullptr);
I think you can omit nullptr

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1178: }
Is this a functional change or just a rewrite?

[ilnik, 2017-03-09 14:34:35]

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_rate_allocator.cc:33:
RTC_DCHECK(layers != nullptr);
On 2017/03/09 14:07:46, stefan-webrtc wrote:
> I think you can omit nullptr

Done.

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2745523002/diff/100001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1178: }
On 2017/03/09 14:07:46, stefan-webrtc wrote:
> Is this a functional change or just a rewrite?

It's a more correct way to do it. Before we had some random numbers I randomly
chosen. Now it's in line with other places.

[ilnik, 2017-03-09 16:48:05]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-03-09 16:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2745523002/#ps120001 (title:
"Implement Stefan@ comments")

[commit-bot: I haz the power, 2017-03-09 16:48:09]

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-09 17:23:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489078085108590,
"parent_rev": "32f2c56e9fbb326a42936e5aa02482268ba0f138", "commit_rev":
"cb8c1467bdf2b90423781b811201b4cb1ea96dec"}

[commit-bot: I haz the power, 2017-03-09 17:23:33]

Description was changed from

==========
Add FullStack test for simulcast screenshare mode.

BUG=webrtc:4172
==========

to

==========
Add FullStack test for simulcast screenshare mode.

BUG=webrtc:4172

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

[commit-bot: I haz the power, 2017-03-09 17:23:35]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/cb8c1467bdf2b90423781b811...