Push back on the video encoder to avoid building queues in the pacer.

Push back on the video encoder to avoid building queues in the pacer.

Implemented behind the field trial "WebRTC-PacerPushbackExperiment/Enabled/"

BUG=webrtc:8171, webrtc:8287
Review-Url: https://codereview.webrtc.org/3004783002
Cr-Commit-Position: refs/heads/master@{#19969}
Committed: https://webrtc.googlesource.com/src/+/a42055116d3f0c20f7ddcb011926e97e2a6bcc08

[philipel, 2017-08-29 11:05:36]

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

[stefan-webrtc, 2017-08-29 12:17:20]

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:50: }
Replace with:
webrtc::field_trial::IsEnabled(kPacerPushbackExperiment)

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:439: }
else if (queue_length_ms > 50) {
50 ms dead zone sounds pretty small. WDYT of increasing to at least 100 ms?

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:440:
float encoding_rate = 1.0 - queue_length_ms / 1000.0;
Should this be something like this, so that we have a dead zone followed by a
linear slope, instead of now where we'll have a jump and then a slope?
1.0 - (queue_length_ms - 50) / 1000.0

[stefan-webrtc, 2017-08-29 12:17:38]

It would be nice to have tests for this even though it's behind a field trial

[philipel, 2017-08-29 15:14:18]

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:50: }
On 2017/08/29 12:17:20, stefan-webrtc wrote:
> Replace with:
> webrtc::field_trial::IsEnabled(kPacerPushbackExperiment)

Done.

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:439: }
else if (queue_length_ms > 50) {
On 2017/08/29 12:17:20, stefan-webrtc wrote:
> 50 ms dead zone sounds pretty small. WDYT of increasing to at least 100 ms?

I got better results using 50 ms rather than 100, but I think it might not
matter, see my comment below.

https://codereview.webrtc.org/3004783002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:440:
float encoding_rate = 1.0 - queue_length_ms / 1000.0;
On 2017/08/29 12:17:20, stefan-webrtc wrote:
> Should this be something like this, so that we have a dead zone followed by a
> linear slope, instead of now where we'll have a jump and then a slope?
> 1.0 - (queue_length_ms - 50) / 1000.0

The idea is to drain the queue completely, so I think having a jump is
preferable.

I also realized we might need to change this P-like controller to a PI-like
controller. The reason we need pushback in the first place is to not build
sender side queues since the pacing rate will be 1.0 or something close to it.
In the current state we could still cause a standing queue in the pacer if the
encoder overproduce >= 5%.

[philipel, 2017-08-30 09:08:20]

As I mentioned in one of my comment, this is a bit harder than I originally
thought. This works well for the screenshare + video case though, so I think we
can start with that.

Maybe the solution is to have a higher pacing rate when you only have video
streams, but as soon as you have a screenshare stream you set it closer to 1.0.

[stefan-webrtc, 2017-09-01 09:01:08]

I'd like a unittest for this as well before landing.

How much does this pushback affect the screenshare? I think it'd be preferred to
degrade the regular video rather than getting blocky / low quality screenshare.

I'm also not sure it's right to scale the bitrate of the video stream too low.
Maybe it's better to freeze it than to send 50 kbps?

https://codereview.webrtc.org/3004783002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/3004783002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:46:
return webrtc::field_trial::IsEnabled(kPacerPushbackExperiment);
Might as well call this function directly below.

[philipel, 2017-09-01 11:46:19]

https://codereview.webrtc.org/3004783002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/3004783002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:46:
return webrtc::field_trial::IsEnabled(kPacerPushbackExperiment);
On 2017/09/01 09:01:08, stefan-webrtc wrote:
> Might as well call this function directly below.

Done.

[philipel, 2017-09-04 11:19:58]

ping

[stefan-webrtc, 2017-09-26 11:42:24]

lgtm

[philipel, 2017-09-26 11:48:48]

Description was changed from

==========
Push back on the video encoder to avoid building queues in the pacer.

Implemented behind the field trial "WebRTC-PacerPushbackExperiment/Enabled/"

BUG=webrtc:8171
==========

to

==========
Push back on the video encoder to avoid building queues in the pacer.

Implemented behind the field trial "WebRTC-PacerPushbackExperiment/Enabled/"

BUG=webrtc:8171, webrtc:8287
==========

[philipel, 2017-09-26 11:52:36]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-09-26 11:52:37]

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/3004783002/#ps100001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-09-26 11:52:47]

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-09-26 12:36:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1506426756590020,
"parent_rev": "e19d8bfd5b680715f10e82bd464d74f06556b5dc", "commit_rev":
"a42055116d3f0c20f7ddcb011926e97e2a6bcc08"}

[commit-bot: I haz the power, 2017-09-26 12:37:03]

Description was changed from

==========
Push back on the video encoder to avoid building queues in the pacer.

Implemented behind the field trial "WebRTC-PacerPushbackExperiment/Enabled/"

BUG=webrtc:8171, webrtc:8287
==========

to

==========
Push back on the video encoder to avoid building queues in the pacer.

Implemented behind the field trial "WebRTC-PacerPushbackExperiment/Enabled/"

BUG=webrtc:8171, webrtc:8287

Review-Url: https://codereview.webrtc.org/3004783002
Cr-Commit-Position: refs/heads/master@{#19969}
Committed:
https://webrtc.googlesource.com/src/+/a42055116d3f0c20f7ddcb011926e97e2a6bcc08
==========

[commit-bot: I haz the power, 2017-09-26 12:37:04]

Committed patchset #6 (id:100001) as
https://webrtc.googlesource.com/src/+/a42055116d3f0c20f7ddcb011926e97e2a6bcc08