Tweaked version of BBR for WebRTC.

Final version of BBR, with tweaks made for WebRTC, major changes:
1) Entering PROBE_RTT when necessary.
2) Congestion window gain of 0.65 instead of constant 4 packets.
3) {1.1, 0.9} pair instead of {1.25, 0.75}
4) Recovery mode.
5) No reaction to losses due to Recovery mode's implementation.
6) Supports encoder.
7) A new test compiling most of the simulation tests.
8) Bucket for high gain phase, disabled by default.
9) Pacer specific to BBR.

BUG=webrtc:7713
Review-Url: https://codereview.webrtc.org/2999073002
Cr-Commit-Position: refs/heads/master@{#19418}
Committed: https://chromium.googlesource.com/external/webrtc/+/a36165c77b81e48201fb907df0eb49fa81f13295

[gnish1, 2017-08-16 15:23:21]

Description was changed from

==========
final tweaked BBR.

BUG=
==========

to

==========
Final version of BBR, with tweaks made for WebRTC, major changes:
1) Entering PROBE_RTT when necessary.
2) Congestion window gain of 0.65 instead of constant 4 packets.
3) {1.1, 0.9} pair instead of {1.25, 0.75}
4) Recovery mode.
5) No reaction to losses due to Recovery mode's implementation.
6) Supports encoder.
7) A new test compiling most of the simulation tests.
8) Bucket for high gain phase, disabled by default.
9) Pacer specific to BBR.

BUG=webrtc:7713
==========

[gnish1, 2017-08-16 15:23:22]

gnish@webrtc.org changed reviewers:
+ philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org

[gnish1, 2017-08-16 15:35:13]

Could be very ugly but 2 days left.

[terelius, 2017-08-16 16:53:08]

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

[commit-bot: I haz the power, 2017-08-16 16:53:12]

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-08-16 16:58:22]

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

[commit-bot: I haz the power, 2017-08-16 16:58:23]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/28638)
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/11473)

[terelius, 2017-08-17 09:33:29]

Could you fix the style issues in pacer.h please.

https://codereview.webrtc.org/2999073002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (left):

https://codereview.webrtc.org/2999073002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:64: // Creates
RtcpBandwidthObserver caller responsible to delete.
Why remove this comment?

[terelius, 2017-08-17 09:35:10]

stefan, could you review the bitrate controller changes?

[philipel, 2017-08-17 10:43:57]

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

[commit-bot: I haz the power, 2017-08-17 10:44:03]

Dry run: CQ is trying da patch.

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

[gnish1, 2017-08-17 10:44:29]

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

[commit-bot: I haz the power, 2017-08-17 10:44:34]

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-08-17 11:18:25]

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

[commit-bot: I haz the power, 2017-08-17 11:18:26]

Dry run: Try jobs failed on following builders:
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/28237)

[gnish1, 2017-08-17 11:22:20]

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

[commit-bot: I haz the power, 2017-08-17 11:22:26]

Dry run: CQ is trying da patch.

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

[philipel, 2017-08-17 12:52:27]

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
File webrtc/modules/pacing/bbr_paced_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:23: struct Packet {
I think you should move this struct into BbrPacedSender and remove the
bbr_paced_sender namespace. Also you can define it in the .h file

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:61: // pacing_budget_(0),
remove commented code

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:99: extra_time_ =
std::min(extra_time_, 2.0f);
Why don't we try to compensate the |next_packet_send_time_| with the accumulated
error every time we calculate it?

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:117: assert(data_inflight_ >= bytes);
RTC_DCHECK_GE

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
File webrtc/modules/pacing/bbr_paced_sender.h (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.h:11: #ifndef
WEBRTC_MODULES_PACING_BBR_PACED_SENDER_H_
This file should be moved. I think
webrtc/modules/remote_bitrate_estimator/test/pacers would be a good place.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.h:57: float extra_time_;
Could you come up with a better name than |extra_time_|, it's not particularly
clear.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.h:60: size_t congestion_window_;
Use the already existing congestion window class instead of re-implementing it
here.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:40: // TODO(sprang): Move at least
PacketQueue and MediaBudget out to separate
revert

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:125:
TEST_P(BweSimulation, All) {
Give this a better name.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:178: int zero = 0;
const int kFlowId = 0;

Also add a comment as to why we do this.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:318: int zero = 0;
ditto

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:67: // Time we
send at |kDefaultRatebps| in order to ensure BBR has data to work
Duration

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: const int64_t
kTimeForDefaultSendingMs = 200;
kDurationDefualtSendMs

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:79: const float
kTargetCongestionWindowGainForHighGain = 1.15f;
I don't quite understand why we need a value that is slightly higher than the
pacing gain in this case.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:355: if
(pacing_gain_ <= 1.0f || !min_rtt_filter_->min_rtt_ms() ||
Add comment about what is going on here.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:452: // If we are
already in Recovery don't try to enter.
Remove comment.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:541: int64_t
corrected_send_time_ms = 0L;
Add comment about |corrected_send_time_ms|.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc
(left):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:41:
return CongestionWindow::kMinimumCongestionWindowBytes;
Why don't we have a min congestion window any more?

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc
(right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:58:
int bdp = *min_rtt_ms * bandwidth_estimate_bps / 8000;
Why do we suddenly measure bandwidth delay product in kB?

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h
(right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:25:
// source code of QUIC's implementation.
Is this comment still correct? Or was in incorrect to begin with?

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:164: pacer_(new
BbrPacedSender(&clock_, this, nullptr)) {
Let pacer be a unique ptr, but instantiate it with a PacedSender instead.

[commit-bot: I haz the power, 2017-08-17 13:18:38]

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

[commit-bot: I haz the power, 2017-08-17 13:18:39]

Dry run: This issue passed the CQ dry run.

[gnish1, 2017-08-17 15:37:49]

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
File webrtc/modules/pacing/bbr_paced_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:23: struct Packet {
On 2017/08/17 12:52:26, philipel wrote:
> I think you should move this struct into BbrPacedSender and remove the
> bbr_paced_sender namespace. Also you can define it in the .h file

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:61: // pacing_budget_(0),
On 2017/08/17 12:52:26, philipel wrote:
> remove commented code

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:99: extra_time_ =
std::min(extra_time_, 2.0f);
On 2017/08/17 12:52:26, philipel wrote:
> Why don't we try to compensate the |next_packet_send_time_| with the
accumulated
> error every time we calculate it?

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.cc:117: assert(data_inflight_ >= bytes);
On 2017/08/17 12:52:26, philipel wrote:
> RTC_DCHECK_GE

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
File webrtc/modules/pacing/bbr_paced_sender.h (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.h:11: #ifndef
WEBRTC_MODULES_PACING_BBR_PACED_SENDER_H_
On 2017/08/17 12:52:26, philipel wrote:
> This file should be moved. I think
> webrtc/modules/remote_bitrate_estimator/test/pacers would be a good place.

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.h:57: float extra_time_;
On 2017/08/17 12:52:26, philipel wrote:
> Could you come up with a better name than |extra_time_|, it's not particularly
> clear. 

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/bbr...
webrtc/modules/pacing/bbr_paced_sender.h:60: size_t congestion_window_;
On 2017/08/17 12:52:26, philipel wrote:
> Use the already existing congestion window class instead of re-implementing it
> here.

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:40: // TODO(sprang): Move at least
PacketQueue and MediaBudget out to separate
On 2017/08/17 12:52:26, philipel wrote:
> revert

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:125:
TEST_P(BweSimulation, All) {
On 2017/08/17 12:52:26, philipel wrote:
> Give this a better name.

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:178: int zero = 0;
On 2017/08/17 12:52:26, philipel wrote:
> const int kFlowId = 0;
> 
> Also add a comment as to why we do this.

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:318: int zero = 0;
On 2017/08/17 12:52:26, philipel wrote:
> ditto

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:67: // Time we
send at |kDefaultRatebps| in order to ensure BBR has data to work
On 2017/08/17 12:52:27, philipel wrote:
> Duration

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: const int64_t
kTimeForDefaultSendingMs = 200;
On 2017/08/17 12:52:27, philipel wrote:
> kDurationDefualtSendMs

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:79: const float
kTargetCongestionWindowGainForHighGain = 1.15f;
On 2017/08/17 12:52:27, philipel wrote:
> I don't quite understand why we need a value that is slightly higher than the
> pacing gain in this case.

As we no longer use 1.25 gain value, to increase data in flight to 1.1 * bdp is
okay, but 1.15 has showed better results (quicker ramp-up).

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:355: if
(pacing_gain_ <= 1.0f || !min_rtt_filter_->min_rtt_ms() ||
On 2017/08/17 12:52:27, philipel wrote:
> Add comment about what is going on here. 

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:452: // If we are
already in Recovery don't try to enter.
On 2017/08/17 12:52:27, philipel wrote:
> Remove comment.

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:541: int64_t
corrected_send_time_ms = 0L;
On 2017/08/17 12:52:27, philipel wrote:
> Add comment about |corrected_send_time_ms|.

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc
(left):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:41:
return CongestionWindow::kMinimumCongestionWindowBytes;
On 2017/08/17 12:52:27, philipel wrote:
> Why don't we have a min congestion window any more?

This implementation of BBR doesn't use minimum congestion window, but congestion
window gain of 0.65 instead.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc
(right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:58:
int bdp = *min_rtt_ms * bandwidth_estimate_bps / 8000;
On 2017/08/17 12:52:27, philipel wrote:
> Why do we suddenly measure bandwidth delay product in kB?

Because packet's are measured in bytes, so data_inflight is measured in bytes,
which means congestion window should be measured in bytes as well.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h
(right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:25:
// source code of QUIC's implementation.
On 2017/08/17 12:52:27, philipel wrote:
> Is this comment still correct? Or was in incorrect to begin with?

Done.

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:164: pacer_(new
BbrPacedSender(&clock_, this, nullptr)) {
On 2017/08/17 12:52:27, philipel wrote:
> Let pacer be a unique ptr, but instantiate it with a PacedSender instead.

Done.

[philipel, 2017-08-18 11:22:15]

lg, just a few more comments

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:178: // reference
instead.
pass pointer

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:319: DelayFilter
delay(&uplink_, CreateFlowIds(&kFlowId, 1));
Add comment here as well.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc:73:
rounding_error_time_ms_ = std::min(rounding_error_time_ms_, 1.0f);
Could this logic be simplified?

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.h (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.h:87: size_t
max_data_inflight_bytes_;
Add a TODO to integrate this into the CongestionWindow class.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: const int64_t
kTimeForDefaultSendMs = 200;
kDuration...

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:550:
corrected_send_time_ms =
Remove this code and add a comment about why it exist in QUICKs implementation
but not ours.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h
(right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:25: //
Expiration time for min_rtt sample, which is set to 10 seconds according to
Update comment

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:30:
class MinRttFilter {
Add a comment about what this class does, and why it's not implemented according
to the BBR spec.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:45: //
Checks whether or not last discovered min_rtt value is older than x
Edit comment.

[gnish1, 2017-08-18 11:48:43]

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:178: // reference
instead.
On 2017/08/18 11:22:14, philipel wrote:
> pass pointer 

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:319: DelayFilter
delay(&uplink_, CreateFlowIds(&kFlowId, 1));
On 2017/08/18 11:22:14, philipel wrote:
> Add comment here as well.

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc:73:
rounding_error_time_ms_ = std::min(rounding_error_time_ms_, 1.0f);
On 2017/08/18 11:22:14, philipel wrote:
> Could this logic be simplified?

Will remain the same after a discussion.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.h (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bbr_paced_sender.h:87: size_t
max_data_inflight_bytes_;
On 2017/08/18 11:22:14, philipel wrote:
> Add a TODO to integrate this into the CongestionWindow class.

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:69: const int64_t
kTimeForDefaultSendMs = 200;
On 2017/08/18 11:22:15, philipel wrote:
> kDuration...

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:550:
corrected_send_time_ms =
On 2017/08/18 11:22:15, philipel wrote:
> Remove this code and add a comment about why it exist in QUICKs implementation
> but not ours.

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h
(right):

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:25: //
Expiration time for min_rtt sample, which is set to 10 seconds according to
On 2017/08/18 11:22:15, philipel wrote:
> Update comment

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:30:
class MinRttFilter {
On 2017/08/18 11:22:15, philipel wrote:
> Add a comment about what this class does, and why it's not implemented
according
> to the BBR spec.

Done.

https://codereview.webrtc.org/2999073002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:45: //
Checks whether or not last discovered min_rtt value is older than x
On 2017/08/18 11:22:15, philipel wrote:
> Edit comment.

Done.

[philipel, 2017-08-18 11:54:02]

lgtm

[philipel, 2017-08-18 11:54:10]

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

[commit-bot: I haz the power, 2017-08-18 11:54:17]

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-08-18 12:39:48]

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

[commit-bot: I haz the power, 2017-08-18 12:39:49]

Dry run: This issue passed the CQ dry run.

[gnish1, 2017-08-18 13:31:47]

The CQ bit was checked by gnish@webrtc.org

[commit-bot: I haz the power, 2017-08-18 13:31:58]

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-08-18 13:38:37]

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

[commit-bot: I haz the power, 2017-08-18 13:38:38]

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

[terelius, 2017-08-18 14:34:04]

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (left):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:44: public
RtcpBandwidthObserver {
Add back

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:64: // Creates
RtcpBandwidthObserver caller responsible to delete.
Add back

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:41: virtual void
OnNetworkChanged(uint32_t bitrate_for_encoder_bps,
Add TODO Merge

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.h:33: class IntervalBudget;
Can it be deleted?

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:135: void
BbrBweSender::HandleLoss(uint64_t last_acked_packet,
Pls add comment about what this does.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:138: if
(!last_acked_packet)
Needed?

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:255: // Encoder
producing slightly more due to rounding errors, so we ask for 2%
No longer true?

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:163: //
TODO(gnish): make pacer choice dependant on the algorithm being used.
nit: Capital M

[gnish1, 2017-08-18 14:52:51]

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

[commit-bot: I haz the power, 2017-08-18 14:53:04]

Dry run: CQ is trying da patch.

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

[gnish1, 2017-08-18 14:53:50]

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (left):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:44: public
RtcpBandwidthObserver {
On 2017/08/18 14:34:03, terelius wrote:
> Add back

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:64: // Creates
RtcpBandwidthObserver caller responsible to delete.
On 2017/08/18 14:34:03, terelius wrote:
> Add back

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:41: virtual void
OnNetworkChanged(uint32_t bitrate_for_encoder_bps,
On 2017/08/18 14:34:03, terelius wrote:
> Add TODO Merge

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.h:33: class IntervalBudget;
On 2017/08/18 14:34:03, terelius wrote:
> Can it be deleted?

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:135: void
BbrBweSender::HandleLoss(uint64_t last_acked_packet,
On 2017/08/18 14:34:04, terelius wrote:
> Pls add comment about what this does.

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:138: if
(!last_acked_packet)
On 2017/08/18 14:34:04, terelius wrote:
> Needed?

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:255: // Encoder
producing slightly more due to rounding errors, so we ask for 2%
On 2017/08/18 14:34:03, terelius wrote:
> No longer true?

Done.

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc (right):

https://codereview.webrtc.org/2999073002/diff/100001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc:163: //
TODO(gnish): make pacer choice dependant on the algorithm being used.
On 2017/08/18 14:34:04, terelius wrote:
> nit: Capital M

Done.

[terelius, 2017-08-18 15:00:03]

lgtm

[commit-bot: I haz the power, 2017-08-18 15:46:10]

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

[commit-bot: I haz the power, 2017-08-18 15:46:11]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-08-20 15:41:04]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-08-20 15:41:08]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2999073002/#ps120001 (title:
"Updated according to comments.")

[commit-bot: I haz the power, 2017-08-20 15:41:14]

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-08-20 16:19:59]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1503243664475490,
"parent_rev": "801830691ad6a66309e1255e6d07f8c9471e3849", "commit_rev":
"a36165c77b81e48201fb907df0eb49fa81f13295"}

[commit-bot: I haz the power, 2017-08-20 16:20:17]

Description was changed from

==========
Final version of BBR, with tweaks made for WebRTC, major changes:
1) Entering PROBE_RTT when necessary.
2) Congestion window gain of 0.65 instead of constant 4 packets.
3) {1.1, 0.9} pair instead of {1.25, 0.75}
4) Recovery mode.
5) No reaction to losses due to Recovery mode's implementation.
6) Supports encoder.
7) A new test compiling most of the simulation tests.
8) Bucket for high gain phase, disabled by default.
9) Pacer specific to BBR.

BUG=webrtc:7713
==========

to

==========
Final version of BBR, with tweaks made for WebRTC, major changes:
1) Entering PROBE_RTT when necessary.
2) Congestion window gain of 0.65 instead of constant 4 packets.
3) {1.1, 0.9} pair instead of {1.25, 0.75}
4) Recovery mode.
5) No reaction to losses due to Recovery mode's implementation.
6) Supports encoder.
7) A new test compiling most of the simulation tests.
8) Bucket for high gain phase, disabled by default.
9) Pacer specific to BBR.

BUG=webrtc:7713

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

[commit-bot: I haz the power, 2017-08-20 16:20:18]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/a36165c77b81e48201fb907df...