Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(126)

Issue 2990163002: Almost full implementation of BBR's core. (Closed)

Created:
3 years, 4 months ago by gnish1
Modified:
3 years, 4 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Almost full implementation of BBR's core, missing receiver side implementation, pacer, and BitrateObserver class which is responsible for communication between BBR and pacer/encoder. Significant changes: Recovery mode and a separate bucket for the high gain phase. BUG=webrtc:7713 Review-Url: https://codereview.webrtc.org/2990163002 Cr-Commit-Position: refs/heads/master@{#19349} Committed: https://chromium.googlesource.com/external/webrtc/+/53d76c619034785ce3d1472539d0792194a4a0ce

Patch Set 1 #

Total comments: 53

Patch Set 2 : Updated according to comments. #

Total comments: 6

Patch Set 3 : Fixed a small bug. #

Total comments: 26

Patch Set 4 : Updated according to comments #

Total comments: 4

Patch Set 5 : Updated according to comments. #

Patch Set 6 : Fixed a bug where bw calculation for high gain phase would not be accurate. #

Patch Set 7 : Small bug fix with wrapping sequence numbers. #

Patch Set 8 : fixed patch failure #

Messages

Total messages: 28 (10 generated)
gnish1
3 years, 4 months ago (2017-08-02 09:11:08 UTC) #5
philipel
https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode41 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:41: const int kMinimumCongestionWindow = 4000; kMinimumCongestionWindow should not be ...
3 years, 4 months ago (2017-08-04 12:08:08 UTC) #6
gnish1
https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode41 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:41: const int kMinimumCongestionWindow = 4000; On 2017/08/04 12:08:06, philipel ...
3 years, 4 months ago (2017-08-07 10:34:29 UTC) #7
gnish1
3 years, 4 months ago (2017-08-07 12:10:21 UTC) #8
gnish1
subtle ping
3 years, 4 months ago (2017-08-08 11:50:24 UTC) #9
philipel
https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h#newcode83 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:83: size_t data_acked_before; On 2017/08/07 10:34:29, gnish1 wrote: > On ...
3 years, 4 months ago (2017-08-08 11:54:55 UTC) #10
gnish1
https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode319 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:319: min_rtt_filter_->AddRttSample(*min_rtt_sample_ms, now_ms); On 2017/08/08 11:54:54, philipel wrote: > Don't ...
3 years, 4 months ago (2017-08-08 13:19:56 UTC) #11
terelius
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode39 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:39: // Least amount number of rounds PROBE_RTT should last. ...
3 years, 4 months ago (2017-08-08 17:32:41 UTC) #12
gnish1
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode39 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:39: // Least amount number of rounds PROBE_RTT should last. ...
3 years, 4 months ago (2017-08-09 10:05:54 UTC) #13
terelius
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode271 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:271: int64_t send_time_delta_ms = last_packet_send_time_during_high_gain_ms_ - On 2017/08/09 10:05:53, gnish1 ...
3 years, 4 months ago (2017-08-11 09:27:53 UTC) #14
terelius
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h#newcode142 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:142: // declare those packets as lost immediately. On 2017/08/09 ...
3 years, 4 months ago (2017-08-11 09:31:36 UTC) #15
gnish1
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc#newcode271 webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:271: int64_t send_time_delta_ms = last_packet_send_time_during_high_gain_ms_ - On 2017/08/11 09:27:52, terelius ...
3 years, 4 months ago (2017-08-12 11:57:32 UTC) #16
terelius
lgtm
3 years, 4 months ago (2017-08-14 13:07:11 UTC) #17
philipel
lgtm
3 years, 4 months ago (2017-08-14 13:15:01 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2990163002/120001
3 years, 4 months ago (2017-08-14 13:15:34 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/20117)
3 years, 4 months ago (2017-08-14 13:17:45 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2990163002/140001
3 years, 4 months ago (2017-08-15 08:45:29 UTC) #25
commit-bot: I haz the power
3 years, 4 months ago (2017-08-15 09:26:31 UTC) #28
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/53d76c619034785ce3d147253...

Powered by Google App Engine
This is Rietveld 408576698