Reduce the BWE with 50% when feedback is received too late.

Reduce the BWE with 50% when feedback is received too late.

Lateness is determined by the length of the send-side history, currently
set to 60 seconds.

BUG=webrtc:5079
Review-Url: https://codereview.webrtc.org/2684353004
Cr-Commit-Position: refs/heads/master@{#16588}
Committed: https://chromium.googlesource.com/external/webrtc/+/e3a55672300af797cac012e65d260860156a3efb

[stefan-webrtc, 2017-02-10 14:15:10]

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

[stefan-webrtc, 2017-02-10 14:15:11]

terelius, nisse: ptal, this does what we talked about earlier today.

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

https://codereview.webrtc.org/2684353004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/transport_feedback_adapter.cc:138: if
(!send_time_history_.GetInfo(&info, true))
With this change we will add also acks which have timed out at the sender, but
they will have send_time_ms set to -1, etc.

[stefan-webrtc, 2017-02-10 14:17:09]

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

[commit-bot: I haz the power, 2017-02-10 14:17:19]

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

[terelius, 2017-02-10 15:21:12]

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
Is this DCHECK really guaranteed to pass? There are several code paths that can
set bitrate_is_initialized_, and it is not clear to me which one is supposed to
have executed.

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:204:
current_bitrate_bps =
Maybe rename current_bitrate_bps to new_bitrate_bps here too?

[commit-bot: I haz the power, 2017-02-10 15:58:22]

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

[commit-bot: I haz the power, 2017-02-10 15:58:23]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-02-13 08:22:48]

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
On 2017/02/10 15:21:12, terelius wrote:
> Is this DCHECK really guaranteed to pass? There are several code paths that
can
> set bitrate_is_initialized_, and it is not clear to me which one is supposed
to
> have executed.

It is guranteed, since we always set the start bitrate from the Call
constructor. I can probably be convinced that this isn't a good thing to DCHECK
on though.

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:204:
current_bitrate_bps =
On 2017/02/10 15:21:12, terelius wrote:
> Maybe rename current_bitrate_bps to new_bitrate_bps here too?

Yes, great suggestion

[nisse-webrtc, 2017-02-13 09:55:35]

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
On 2017/02/13 08:22:48, stefan-webrtc wrote:
> On 2017/02/10 15:21:12, terelius wrote:
> > Is this DCHECK really guaranteed to pass? There are several code paths that
> can
> > set bitrate_is_initialized_, and it is not clear to me which one is supposed
> to
> > have executed.
> 
> It is guranteed, since we always set the start bitrate from the Call
> constructor. I can probably be convinced that this isn't a good thing to
DCHECK
> on though.

What's the alternative? Do nothing if there's no estimate?

https://codereview.webrtc.org/2684353004/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right):

https://codereview.webrtc.org/2684353004/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:64: uint32_t
ClampBitrate(uint32_t new_bitrate_bps,
I think this method deserves a comment.

[stefan-webrtc, 2017-02-13 14:41:13]

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
On 2017/02/13 09:55:34, nisse-webrtc wrote:
> On 2017/02/13 08:22:48, stefan-webrtc wrote:
> > On 2017/02/10 15:21:12, terelius wrote:
> > > Is this DCHECK really guaranteed to pass? There are several code paths
that
> > can
> > > set bitrate_is_initialized_, and it is not clear to me which one is
supposed
> > to
> > > have executed.
> > 
> > It is guranteed, since we always set the start bitrate from the Call
> > constructor. I can probably be convinced that this isn't a good thing to
> DCHECK
> > on though.
> 
> What's the alternative? Do nothing if there's no estimate?

Either that, or to take our estimated acked bitrate, if we have one, and use
that as our estimate.

Another option would be to compute an estimate as we talked about, by making an
assumption of how large the packets sent actually were. That's a bigger change,
so I'd prefer to do that later if we think it's worth it.

https://codereview.webrtc.org/2684353004/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right):

https://codereview.webrtc.org/2684353004/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:64: uint32_t
ClampBitrate(uint32_t new_bitrate_bps,
On 2017/02/13 09:55:35, nisse-webrtc wrote:
> I think this method deserves a comment. 

Done.

[terelius, 2017-02-13 14:46:47]

lgtm

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
On 2017/02/13 08:22:48, stefan-webrtc wrote:
> On 2017/02/10 15:21:12, terelius wrote:
> > Is this DCHECK really guaranteed to pass? There are several code paths that
> can
> > set bitrate_is_initialized_, and it is not clear to me which one is supposed
> to
> > have executed.
> 
> It is guranteed, since we always set the start bitrate from the Call
> constructor. I can probably be convinced that this isn't a good thing to
DCHECK
> on though.

Acknowledged.

[nisse-webrtc, 2017-02-13 14:49:49]

lgtm, with one more comment.

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
On 2017/02/13 14:41:13, stefan-webrtc wrote:
> On 2017/02/13 09:55:34, nisse-webrtc wrote:
> > On 2017/02/13 08:22:48, stefan-webrtc wrote:
> > > On 2017/02/10 15:21:12, terelius wrote:
> > > > Is this DCHECK really guaranteed to pass? There are several code paths
> that
> > > can
> > > > set bitrate_is_initialized_, and it is not clear to me which one is
> supposed
> > > to
> > > > have executed.
> > > 
> > > It is guranteed, since we always set the start bitrate from the Call
> > > constructor. I can probably be convinced that this isn't a good thing to
> > DCHECK
> > > on though.
> > 
> > What's the alternative? Do nothing if there's no estimate?
> 
> Either that, or to take our estimated acked bitrate, if we have one, and use
> that as our estimate.
> 
> Another option would be to compute an estimate as we talked about, by making
an
> assumption of how large the packets sent actually were. That's a bigger
change,
> so I'd prefer to do that later if we think it's worth it.

If we think RTC_DCHECK is good enough for now, can you add a comment pointing
out why it's true (thanks to the Call constructor)?

[stefan-webrtc, 2017-02-13 15:35:49]

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

https://codereview.webrtc.org/2684353004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:285:
RTC_DCHECK(rate_control_.ValidEstimate());
On 2017/02/13 14:49:49, nisse-webrtc wrote:
> On 2017/02/13 14:41:13, stefan-webrtc wrote:
> > On 2017/02/13 09:55:34, nisse-webrtc wrote:
> > > On 2017/02/13 08:22:48, stefan-webrtc wrote:
> > > > On 2017/02/10 15:21:12, terelius wrote:
> > > > > Is this DCHECK really guaranteed to pass? There are several code paths
> > that
> > > > can
> > > > > set bitrate_is_initialized_, and it is not clear to me which one is
> > supposed
> > > > to
> > > > > have executed.
> > > > 
> > > > It is guranteed, since we always set the start bitrate from the Call
> > > > constructor. I can probably be convinced that this isn't a good thing to
> > > DCHECK
> > > > on though.
> > > 
> > > What's the alternative? Do nothing if there's no estimate?
> > 
> > Either that, or to take our estimated acked bitrate, if we have one, and use
> > that as our estimate.
> > 
> > Another option would be to compute an estimate as we talked about, by making
> an
> > assumption of how large the packets sent actually were. That's a bigger
> change,
> > so I'd prefer to do that later if we think it's worth it.
> 
> If we think RTC_DCHECK is good enough for now, can you add a comment pointing
> out why it's true (thanks to the Call constructor)?

Extended the comment. Thanks

[stefan-webrtc, 2017-02-13 15:35:59]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-02-13 15:36:00]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org,
nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2684353004/#ps100001 (title:
"Updated comment.")

[commit-bot: I haz the power, 2017-02-13 15:36:10]

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-02-13 17:08:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487000159534680,
"parent_rev": "9b0dbd443888852065e8584646d65460435d0196", "commit_rev":
"e3a55672300af797cac012e65d260860156a3efb"}

[commit-bot: I haz the power, 2017-02-13 17:08:26]

Description was changed from

==========
Reduce the BWE with 50% when feedback is received too late.

Lateness is determined by the length of the send-side history, currently
set to 60 seconds.

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

to

==========
Reduce the BWE with 50% when feedback is received too late.

Lateness is determined by the length of the send-side history, currently
set to 60 seconds.

BUG=webrtc:5079

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

[commit-bot: I haz the power, 2017-02-13 17:08:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/e3a55672300af797cac012e65...