Use different restrictions of acked bitrate lag depending on operating point.

Use different restrictions of acked bitrate lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE.

BUG=webrtc:5079
Committed: https://crrev.com/5932149c9aeaa7679ad0bc3183047766832ca907
Cr-Commit-Position: refs/heads/master@{#15389}

[stefan-webrtc, 2016-12-01 15:42:24]

Description was changed from

==========
Use different restrictions of lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a
good idea for audio BWE.

BUG=webrtc:5079
==========

to

==========
Use different restrictions of acked bitrate lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a
good idea for audio BWE.

BUG=webrtc:5079
==========

[stefan-webrtc, 2016-12-01 15:42:31]

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

[terelius, 2016-12-02 15:30:14]

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right):

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:234: const uint32_t
max_allowed_lag_kbps =
Could we rename this max_bitrate_bps? Lag makes me think of the time lag between
the network change and when we update the what the encoder target. Also, it is
measured in bps.

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:238: // Don't
change the bit rate if the send side is too far off.
Place this comment above if-statement and other comment?

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:239:
current_bitrate_bps = current_bitrate_bps_;
If max_allowed_lag_kbps is the cap and we are above the cap, shouldn't we update
the bitrate to max_allowed_lag_kbps?

Maybe we can replace the entire if-statement with
current_bitrate_bps = std::max(current_bitrate_bps, max_bitrate_bps)?

[stefan-webrtc, 2016-12-02 16:12:35]

Comments addressed.

[terelius, 2016-12-02 16:15:12]

lgtm

[stefan-webrtc, 2016-12-02 16:15:49]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-12-02 16:15:54]

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

[stefan-webrtc, 2016-12-02 16:16:14]

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right):

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:234: const uint32_t
max_allowed_lag_kbps =
On 2016/12/02 15:30:13, terelius wrote:
> Could we rename this max_bitrate_bps? Lag makes me think of the time lag
between
> the network change and when we update the what the encoder target. Also, it is
> measured in bps.

Done.

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:238: // Don't
change the bit rate if the send side is too far off.
On 2016/12/02 15:30:13, terelius wrote:
> Place this comment above if-statement and other comment? 

Done.

https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:239:
current_bitrate_bps = current_bitrate_bps_;
On 2016/12/02 15:30:13, terelius wrote:
> If max_allowed_lag_kbps is the cap and we are above the cap, shouldn't we
update
> the bitrate to max_allowed_lag_kbps?
> 
> Maybe we can replace the entire if-statement with
> current_bitrate_bps = std::max(current_bitrate_bps, max_bitrate_bps)?

Done as discussed.

[commit-bot: I haz the power, 2016-12-02 16:46:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480695349983490,
"parent_rev": "02aa2292dd44e417cf16146860d19bafdfa19fe1", "commit_rev":
"30179ccb10f932ce958a5568df08f0ebcb4efb6b"}

[commit-bot: I haz the power, 2016-12-02 16:46:27]

Description was changed from

==========
Use different restrictions of acked bitrate lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a
good idea for audio BWE.

BUG=webrtc:5079
==========

to

==========
Use different restrictions of acked bitrate lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a
good idea for audio BWE.

BUG=webrtc:5079
==========

[commit-bot: I haz the power, 2016-12-02 16:46:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-02 16:46:38]

Description was changed from

==========
Use different restrictions of acked bitrate lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a
good idea for audio BWE.

BUG=webrtc:5079
==========

to

==========
Use different restrictions of acked bitrate lag depending on operating point.

Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a
good idea for audio BWE.

BUG=webrtc:5079

Committed: https://crrev.com/5932149c9aeaa7679ad0bc3183047766832ca907
Cr-Commit-Position: refs/heads/master@{#15389}
==========

[commit-bot: I haz the power, 2016-12-02 16:46:39]

Patchset 3 (id:??) landed as
https://crrev.com/5932149c9aeaa7679ad0bc3183047766832ca907
Cr-Commit-Position: refs/heads/master@{#15389}

[Taylor Brandstetter, 2016-12-02 19:29:24]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/2547113002/ by deadbeef@webrtc.org.

The reason for reverting is: Appears to cause a regression to
RampUpTest.SendSideAudioOnlyUpDownUpRtx:

https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20....