Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 1 day ago by gnish1
Modified:
2 days, 9 hours 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+470 lines, -81 lines) Patch
M webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h View 1 2 3 4 7 chunks +131 lines, -11 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc View 1 2 3 4 5 6 12 chunks +284 lines, -31 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h View 1 1 chunk +4 lines, -2 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc View 1 4 chunks +14 lines, -8 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter_unittest.cc View 1 1 chunk +19 lines, -19 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h View 1 2 chunks +9 lines, -4 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter_unittest.cc View 1 1 chunk +9 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 28 (10 generated)
gnish1
2 weeks, 1 day 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 ...
1 week, 6 days 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 ...
1 week, 3 days ago (2017-08-07 10:34:29 UTC) #7
gnish1
1 week, 3 days ago (2017-08-07 12:10:21 UTC) #8
gnish1
subtle ping
1 week, 2 days 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 ...
1 week, 2 days 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 ...
1 week, 2 days 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. ...
1 week, 2 days 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. ...
1 week, 1 day 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 ...
6 days, 9 hours 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 ...
6 days, 9 hours 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 ...
5 days, 7 hours ago (2017-08-12 11:57:32 UTC) #16
terelius
lgtm
3 days, 6 hours ago (2017-08-14 13:07:11 UTC) #17
philipel
lgtm
3 days, 5 hours 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 days, 5 hours 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 days, 5 hours 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
2 days, 10 hours ago (2017-08-15 08:45:29 UTC) #25
commit-bot: I haz the power
2 days, 9 hours 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b