Update screen simulcast config and fix periodic encoder param update

Update screen simulcast config and fix periodic encoder param update

Lower then bitrate so that the delta between the highest layer of the
lower stream and lowest layer of the higher stream is not too large.

Also fix a bug in vie_encoder where the codec was not perioducally
updated unless a new bitrate estimate was triggered.

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

[sprang_webrtc, 2017-04-03 14:43:02]

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

[sprang_webrtc, 2017-04-03 14:43:19]

Description was changed from

==========
Update screen simulcast config and fix periodic encoder param update

Lower then bitrate so that the delta between the highest layer of the
lower stream and lowest layer of the higher stream is not too large.

Also fix a bug in vie_encoder where the codec was not perioducally
updated unless a new bitrate estimate was triggered.

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

to

==========
Update screen simulcast config and fix periodic encoder param update

Lower then bitrate so that the delta between the highest layer of the
lower stream and lowest layer of the higher stream is not too large.

Also fix a bug in vie_encoder where the codec was not perioducally
updated unless a new bitrate estimate was triggered.

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

[ilnik, 2017-04-05 14:19:15]

On 2017/04/03 14:43:02, språng_OOO_1May wrote:

Stefan, you should probably review this. I will try to address your comments
instead of Sprang@.

[stefan-webrtc, 2017-04-05 15:06:48]

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast.cc
File webrtc/media/engine/simulcast.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:181: std::min<int>(max_streams,
kDefaultScreenshareSimulcastStreams);
kDefaultScreenshareSimulcastStreams should maybe be called
kMaxScreenshareSimulcastStreams?

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:201: if (is_screencast) {
I think it would be good to have a general comment of what the code below does.
Is the goal to configure "legacy" screenshare with two temporal layers if no
spatial layers are enabled, and if spatial layers configure up to two spatial
layers with 3 regular temporal layers each?

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:215: 1000);
Looks like we're doing 2 temporal layers here, and 3 for the higher quality
stream if there simulcast is configured.

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:217: if (num_simulcast_layers == 2) {
Should this be kDefaultScreenshareSimulcastStreams?

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:223: // stalled bitrate estimates.
I don't fully understand this. Why would the bitrate estimate stall? Is that
something that needs fixing elsewhere? AFAIK we are padding to be able to reach
higher bitrates independent of what the codec produces?

Why do we not use some factor of streams[0].max_bitrate_bps for this? Should we
not be able to get to that layer before we get to the next spatial layer?

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:260: if (!is_screencast) {
This is always true AFAICT?

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:680: }
I don't immediately understand what the problem was here. Did we get in the
(missing) else clause? Should we not set last_parameters_update_ms_ if we run
ReconfigureEncoder()? After all, it does call
video_sender_.UpdateChannelParemeters().

[sprang_webrtc, 2017-05-08 08:50:54]

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast.cc
File webrtc/media/engine/simulcast.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:181: std::min<int>(max_streams,
kDefaultScreenshareSimulcastStreams);
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> kDefaultScreenshareSimulcastStreams should maybe be called
> kMaxScreenshareSimulcastStreams?

Sure.

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:201: if (is_screencast) {
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> I think it would be good to have a general comment of what the code below
does.
> Is the goal to configure "legacy" screenshare with two temporal layers if no
> spatial layers are enabled, and if spatial layers configure up to two spatial
> layers with 3 regular temporal layers each?

Added comments.
In both cases we use the legacy screenshare setup for the lowest simulcast
stream. If simulcast is enabled, we add a second high quality stream on top of
the legacy one. This will use the normal 3TL setup. So the two streams will have
completely independent frame rate and temporal layer config. This is not changed
with this CL, the main change is that we can now explicitly set the config for
the higher stream without a bunch of checks for "is_screenshare && blah"

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:215: 1000);
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> Looks like we're doing 2 temporal layers here, and 3 for the higher quality
> stream if there simulcast is configured.

Yes, that's the idea.

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:217: if (num_simulcast_layers == 2) {
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> Should this be kDefaultScreenshareSimulcastStreams?

Done.

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:223: // stalled bitrate estimates.
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> I don't fully understand this. Why would the bitrate estimate stall? Is that
> something that needs fixing elsewhere? AFAIK we are padding to be able to
reach
> higher bitrates independent of what the codec produces?
> 
> Why do we not use some factor of streams[0].max_bitrate_bps for this? Should
we
> not be able to get to that layer before we get to the next spatial layer?

We do at the source, yes. But there is no guarantee for this after pass-through
via an MCU.

https://codereview.webrtc.org/2791273002/diff/1/webrtc/media/engine/simulcast...
webrtc/media/engine/simulcast.cc:260: if (!is_screencast) {
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> This is always true AFAICT?

Done.

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:680: }
On 2017/04/05 15:06:48, stefan-webrtc wrote:
> I don't immediately understand what the problem was here. Did we get in the
> (missing) else clause? Should we not set last_parameters_update_ms_ if we run
> ReconfigureEncoder()? After all, it does call
> video_sender_.UpdateChannelParemeters().

The intent for |last_parameters_update_ms_| is to indicate when we last called
video_sender_.UpdateChannelParemeters().
The second check above looks if more than one second has passed since last we
did that and if so we should update the encoder. The problem is that we now
reset that timestamp on every frame, so unless there is more than one second
between incoming frames that update will never be triggered.

It's true that ReconfigureEncoder() also updates the encoder, so if you want to
I can add an update to the timer there as well, but it's probably a rare
occasion so I don't the we risk updating the parameters too often...

[stefan-webrtc, 2017-05-08 11:09:40]

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:680: }
On 2017/05/08 08:50:53, sprang_webrtc wrote:
> On 2017/04/05 15:06:48, stefan-webrtc wrote:
> > I don't immediately understand what the problem was here. Did we get in the
> > (missing) else clause? Should we not set last_parameters_update_ms_ if we
run
> > ReconfigureEncoder()? After all, it does call
> > video_sender_.UpdateChannelParemeters().
> 
> The intent for |last_parameters_update_ms_| is to indicate when we last called
> video_sender_.UpdateChannelParemeters().
> The second check above looks if more than one second has passed since last we
> did that and if so we should update the encoder. The problem is that we now
> reset that timestamp on every frame, so unless there is more than one second
> between incoming frames that update will never be triggered.
> 
> It's true that ReconfigureEncoder() also updates the encoder, so if you want
to
> I can add an update to the timer there as well, but it's probably a rare
> occasion so I don't the we risk updating the parameters too often...

I don't know. Maybe it's good to be consistent? Otherwise I think we should
comment on why we don't set it, as it'll be very hard for someone new to the
code to understand this.

[sprang_webrtc, 2017-05-08 11:12:28]

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:680: }
On 2017/05/08 11:09:40, stefan-webrtc wrote:
> On 2017/05/08 08:50:53, sprang_webrtc wrote:
> > On 2017/04/05 15:06:48, stefan-webrtc wrote:
> > > I don't immediately understand what the problem was here. Did we get in
the
> > > (missing) else clause? Should we not set last_parameters_update_ms_ if we
> run
> > > ReconfigureEncoder()? After all, it does call
> > > video_sender_.UpdateChannelParemeters().
> > 
> > The intent for |last_parameters_update_ms_| is to indicate when we last
called
> > video_sender_.UpdateChannelParemeters().
> > The second check above looks if more than one second has passed since last
we
> > did that and if so we should update the encoder. The problem is that we now
> > reset that timestamp on every frame, so unless there is more than one second
> > between incoming frames that update will never be triggered.
> > 
> > It's true that ReconfigureEncoder() also updates the encoder, so if you want
> to
> > I can add an update to the timer there as well, but it's probably a rare
> > occasion so I don't the we risk updating the parameters too often...
> 
> I don't know. Maybe it's good to be consistent? Otherwise I think we should
> comment on why we don't set it, as it'll be very hard for someone new to the
> code to understand this.

Alright, added update after ReconfigureEncoder() as well.

[sprang_webrtc, 2017-05-08 11:12:28]

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2791273002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:680: }
On 2017/05/08 11:09:40, stefan-webrtc wrote:
> On 2017/05/08 08:50:53, sprang_webrtc wrote:
> > On 2017/04/05 15:06:48, stefan-webrtc wrote:
> > > I don't immediately understand what the problem was here. Did we get in
the
> > > (missing) else clause? Should we not set last_parameters_update_ms_ if we
> run
> > > ReconfigureEncoder()? After all, it does call
> > > video_sender_.UpdateChannelParemeters().
> > 
> > The intent for |last_parameters_update_ms_| is to indicate when we last
called
> > video_sender_.UpdateChannelParemeters().
> > The second check above looks if more than one second has passed since last
we
> > did that and if so we should update the encoder. The problem is that we now
> > reset that timestamp on every frame, so unless there is more than one second
> > between incoming frames that update will never be triggered.
> > 
> > It's true that ReconfigureEncoder() also updates the encoder, so if you want
> to
> > I can add an update to the timer there as well, but it's probably a rare
> > occasion so I don't the we risk updating the parameters too often...
> 
> I don't know. Maybe it's good to be consistent? Otherwise I think we should
> comment on why we don't set it, as it'll be very hard for someone new to the
> code to understand this.

Alright, added update after ReconfigureEncoder() as well.

[stefan-webrtc, 2017-05-08 11:14:52]

lgtm

[sprang_webrtc, 2017-05-08 12:38:27]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-05-08 12:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2791273002/#ps60001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-05-08 12:38:34]

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-05-08 12:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 12:50:24]

Try jobs failed on following builders:
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/22364)

[sprang_webrtc, 2017-05-22 14:13:03]

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

[commit-bot: I haz the power, 2017-05-22 14:13:10]

Dry run: CQ is trying da patch.

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

[sprang_webrtc, 2017-05-22 14:14:44]

All rate update logic and tests have been moved to
https://codereview.webrtc.org/2883963002/
Only simulcast config remains in this cl and is unchanged since last lgtm, so
I'm gonna assume this cl still has lgtm
Unless you complain :)

[commit-bot: I haz the power, 2017-05-22 15:02:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 15:02:21]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-05-23 13:43:30]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-05-23 13:43:33]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2791273002/#ps80001 (title:
"Remove period reconfig and test, moved to separate cl")

[commit-bot: I haz the power, 2017-05-23 13:43:36]

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-05-23 13:45:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495547010388680,
"parent_rev": "c3d4b48e7e87a3d0ed0a6444cd7f15fa6ef622c7", "commit_rev":
"dceb42da3e7d24ffeee93e20c4ce1ce90953bf88"}

[commit-bot: I haz the power, 2017-05-23 13:45:14]

Description was changed from

==========
Update screen simulcast config and fix periodic encoder param update

Lower then bitrate so that the delta between the highest layer of the
lower stream and lowest layer of the higher stream is not too large.

Also fix a bug in vie_encoder where the codec was not perioducally
updated unless a new bitrate estimate was triggered.

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

to

==========
Update screen simulcast config and fix periodic encoder param update

Lower then bitrate so that the delta between the highest layer of the
lower stream and lowest layer of the higher stream is not too large.

Also fix a bug in vie_encoder where the codec was not perioducally
updated unless a new bitrate estimate was triggered.

BUG=webrtc:4172

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

[commit-bot: I haz the power, 2017-05-23 13:45:16]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/dceb42da3e7d24ffeee93e20c...