Add probing to recover faster from large bitrate drops.

Add probing to recover faster from large bitrate drops. A single probe at 85% of the original bitrate is sent when transitioning from underusing back to normal state. The actual sending of the probes is disabled by default, and enabled by the field trial string WebRTC-BweRapidRecoveryExperiment/Enabled/. Existing code that did probing after large drops in ALR have been restructured so that it also delays the probe until we are no longer overusing.

BUG=webrtc:8015
Review-Url: https://codereview.webrtc.org/2986563002
Cr-Commit-Position: refs/heads/master@{#19187}
Committed: https://chromium.googlesource.com/external/webrtc/+/3376c84c90c26ad8daff030ca2205d88ad97ec4a

[terelius, 2017-07-20 13:58:19]

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

[commit-bot: I haz the power, 2017-07-20 13:58:27]

Dry run: CQ is trying da patch.

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

[terelius, 2017-07-20 14:02:01]

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

[commit-bot: I haz the power, 2017-07-20 14:27:17]

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

[commit-bot: I haz the power, 2017-07-20 14:27:18]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-07-25 11:26:04]

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:266:
kBitrateDropProbeFraction * bitrate_before_last_large_drop_ &&
I think a slightly clearer way of doing this check is to replace 
"result.target_bitrate_bps < 
    kBitrateDropProbeFraction * bitrate_before_last_large_drop_"
with
"time_of_last_drop_ms_ == now_ms"

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:268:
result.suggested_probe_bps = static_cast<rtc::Optional<uint32_t>>(
result.suggested_probe_bps.emplace

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:184:
suggested_probing_bitrate_bps > estimated_bitrate_bps_ &&
You might want to use a factor of the probing bitrate to avoid probing when the
BWE is close to the suggested probing bitrate, probing being somewhat jittery
and all.

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:188:
(alr_end_time_ms_.has_value() &&
I think it's cleaner to implement ALR end time in the ALR detector, this way you
can avoid SetAlrEndedTimeMs and |alr_end_time_ms_|.

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:199: // TODO(isheriff):
May want to track when we did ALR probing in order
Does this comment make sense anymore? Otherwise remove it.

[terelius, 2017-07-27 21:02:46]

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:266:
kBitrateDropProbeFraction * bitrate_before_last_large_drop_ &&
On 2017/07/25 11:26:04, philipel wrote:
> I think a slightly clearer way of doing this check is to replace 
> "result.target_bitrate_bps < 
>     kBitrateDropProbeFraction * bitrate_before_last_large_drop_"
> with
> "time_of_last_drop_ms_ == now_ms"

It is not the same thing. However, it seems a bit redundant to check that we're
not probing at a lower bitrate that the current target both here and in the
ProbeController. The main difference is that the ProbeController gets its
estimate from the BitrateController. One the other hand, maybe it is safer and
clearer to do the similar checks in both places? WDYT?

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:268:
result.suggested_probe_bps = static_cast<rtc::Optional<uint32_t>>(
On 2017/07/25 11:26:04, philipel wrote:
> result.suggested_probe_bps.emplace 

Done.

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:184:
suggested_probing_bitrate_bps > estimated_bitrate_bps_ &&
On 2017/07/25 11:26:04, philipel wrote:
> You might want to use a factor of the probing bitrate to avoid probing when
the
> BWE is close to the suggested probing bitrate, probing being somewhat jittery
> and all.

Done. I set the "probe uncertainty" to 5% of the target rate. Does that seem
reasonable?

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:188:
(alr_end_time_ms_.has_value() &&
On 2017/07/25 11:26:04, philipel wrote:
> I think it's cleaner to implement ALR end time in the ALR detector, this way
you
> can avoid SetAlrEndedTimeMs and |alr_end_time_ms_|.

In principle, I agree. However, that would require us to add a GetAlrEndTime to
the pacer, and it still wouldn't be reusable in the AcknowledgedBitrateEstimator
because we want to react to the ALR change exactly once. Because different
classes may want to determine when they have handled the "ALR ended" signal, we
decided to just pass the state change from SendSideCongestionController and let
each class track it separately.

Besides, if the experiment is positive we'll probably make it default which
means that we won't have to test for "ALR ended" anyway.

If you have any ideas on how to implement it in the AlrDetector in a way that
will work for both the AcknowledgedBitrateEstimator and the ProbeController, I'm
all ears. :)

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:199: // TODO(isheriff):
May want to track when we did ALR probing in order
On 2017/07/25 11:26:04, philipel wrote:
> Does this comment make sense anymore? Otherwise remove it.

I have no idea what it means/meant. I just copied it from the old code. Do you
think I should remove it?

[terelius, 2017-07-28 17:14:47]

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:266:
kBitrateDropProbeFraction * bitrate_before_last_large_drop_ &&
On 2017/07/27 21:02:45, terelius wrote:
> On 2017/07/25 11:26:04, philipel wrote:
> > I think a slightly clearer way of doing this check is to replace 
> > "result.target_bitrate_bps < 
> >     kBitrateDropProbeFraction * bitrate_before_last_large_drop_"
> > with
> > "time_of_last_drop_ms_ == now_ms"
> 
> It is not the same thing. However, it seems a bit redundant to check that
we're
> not probing at a lower bitrate that the current target both here and in the
> ProbeController. The main difference is that the ProbeController gets its
> estimate from the BitrateController. One the other hand, maybe it is safer and
> clearer to do the similar checks in both places? WDYT?

Moved the entire drop detection into the ProbeController as discussed offline.

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:188:
(alr_end_time_ms_.has_value() &&
On 2017/07/27 21:02:45, terelius wrote:
> On 2017/07/25 11:26:04, philipel wrote:
> > I think it's cleaner to implement ALR end time in the ALR detector, this way
> you
> > can avoid SetAlrEndedTimeMs and |alr_end_time_ms_|.
> 
> In principle, I agree. However, that would require us to add a GetAlrEndTime
to
> the pacer, and it still wouldn't be reusable in the
AcknowledgedBitrateEstimator
> because we want to react to the ALR change exactly once. Because different
> classes may want to determine when they have handled the "ALR ended" signal,
we
> decided to just pass the state change from SendSideCongestionController and
let
> each class track it separately.
> 
> Besides, if the experiment is positive we'll probably make it default which
> means that we won't have to test for "ALR ended" anyway.
> 
> If you have any ideas on how to implement it in the AlrDetector in a way that
> will work for both the AcknowledgedBitrateEstimator and the ProbeController,
I'm
> all ears. :)

Since the method you proposed will involve changes to the pacer, the alr
detector and the acknowledged bitrate estimator I'd prefer doing that in a
follow-up CL.

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:199: // TODO(isheriff):
May want to track when we did ALR probing in order
On 2017/07/27 21:02:45, terelius wrote:
> On 2017/07/25 11:26:04, philipel wrote:
> > Does this comment make sense anymore? Otherwise remove it.
> 
> I have no idea what it means/meant. I just copied it from the old code. Do you
> think I should remove it?

Removed it.

[terelius, 2017-07-28 17:15:29]

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

[commit-bot: I haz the power, 2017-07-28 17:15:40]

Dry run: 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-07-28 17:39:12]

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

[commit-bot: I haz the power, 2017-07-28 17:39:13]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-07-31 09:40:17]

lgtm

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

https://codereview.webrtc.org/2986563002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_controller.cc:188:
(alr_end_time_ms_.has_value() &&
On 2017/07/28 17:14:47, terelius wrote:
> On 2017/07/27 21:02:45, terelius wrote:
> > On 2017/07/25 11:26:04, philipel wrote:
> > > I think it's cleaner to implement ALR end time in the ALR detector, this
way
> > you
> > > can avoid SetAlrEndedTimeMs and |alr_end_time_ms_|.
> > 
> > In principle, I agree. However, that would require us to add a GetAlrEndTime
> to
> > the pacer, and it still wouldn't be reusable in the
> AcknowledgedBitrateEstimator
> > because we want to react to the ALR change exactly once. Because different
> > classes may want to determine when they have handled the "ALR ended" signal,
> we
> > decided to just pass the state change from SendSideCongestionController and
> let
> > each class track it separately.
> > 
> > Besides, if the experiment is positive we'll probably make it default which
> > means that we won't have to test for "ALR ended" anyway.
> > 
> > If you have any ideas on how to implement it in the AlrDetector in a way
that
> > will work for both the AcknowledgedBitrateEstimator and the ProbeController,
> I'm
> > all ears. :)
> 
> Since the method you proposed will involve changes to the pacer, the alr
> detector and the acknowledged bitrate estimator I'd prefer doing that in a
> follow-up CL.

Acknowledged.

[terelius, 2017-07-31 10:50:55]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2017-07-31 10:51:04]

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-07-31 11:23:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1501498255221680,
"parent_rev": "8eab09c77b01783aef5323a0ed9d0200c76e1ade", "commit_rev":
"3376c84c90c26ad8daff030ca2205d88ad97ec4a"}

[commit-bot: I haz the power, 2017-07-31 11:23:39]

Description was changed from

==========
Add probing to recover faster from large bitrate drops. A single probe at 85% of
the original bitrate is sent when transitioning from underusing back to normal
state. The actual sending of the probes is disabled by default, and enabled by
the field trial string WebRTC-BweRapidRecoveryExperiment/Enabled/. Existing code
that did probing after large drops in ALR have been restructured so that it also
delays the probe until we are no longer overusing.

BUG=webrtc:8015
==========

to

==========
Add probing to recover faster from large bitrate drops. A single probe at 85% of
the original bitrate is sent when transitioning from underusing back to normal
state. The actual sending of the probes is disabled by default, and enabled by
the field trial string WebRTC-BweRapidRecoveryExperiment/Enabled/. Existing code
that did probing after large drops in ALR have been restructured so that it also
delays the probe until we are no longer overusing.

BUG=webrtc:8015

Review-Url: https://codereview.webrtc.org/2986563002
Cr-Commit-Position: refs/heads/master@{#19187}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3376c84c90c26ad8daff030ca...
==========

[commit-bot: I haz the power, 2017-07-31 11:23:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/3376c84c90c26ad8daff030ca...