Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924603002:
Added implementation of four functions in the BBR congestion controller. (Closed)
DescriptionAdded implementation of four functions in the BBR congestion controller:
1) Function responsible for receiving feedback, digesting data and deciding switch scenarios.
2) Function which enters Startup mode.
3) Function which exits Startup mode.
4) Function which calculates, whether or not full bandwidth is reached.
BUG=webrtc:7713
Review-Url: https://codereview.webrtc.org/2924603002
Cr-Commit-Position: refs/heads/master@{#18901}
Committed: https://chromium.googlesource.com/external/webrtc/+/191113a46bd139fda0368eb1be470acc56b5f28c
Patch Set 1 #
Total comments: 14
Patch Set 2 : Updated according to review. #
Total comments: 10
Patch Set 3 : Added some comments and declaration of two functions. #
Total comments: 14
Patch Set 4 : Removed extra header includes, better comment formatting. #
Total comments: 1
Patch Set 5 : Removed all extra #include-s. #
Total comments: 6
Patch Set 6 : Even more headers removed. #
Total comments: 14
Patch Set 7 : Updated according to review. #Patch Set 8 : compile fix #Patch Set 9 : f removed. #Messages
Total messages: 38 (12 generated)
Description was changed from ========== formatted dirty_bbr a some functions testing BUG=webrtc:7713 ========== to ========== Added implementation of four functions: 1) Function responsible for receiving feedback, digesting data and deciding switch scenarios. 2) Function which enters Startup mode. 3) Function which exits Startup mode. 4) Function which calculates, whether or not full bandwidth is reached. ==========
gnish@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: const float kHighGain = 2.885; Comment where you got these values from. https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:56: // Check if new round started for the connection. Don't split comments into unnecessarily many lines, use the 80 char limit https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:68: if (new_round_started && !full_bandwidth_reached_) When the if statement or the following expression is more than one line use {} https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:86: TryEnteringOrExitingProbeRtt(now); Should this function always be called? https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:123: (static_cast<const MediaPacket*>(packets.back()))->sequence_number(); remove one set of () https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:29: class MaxBandwidthFilter { Please implement the full MaxBandwidthFilter class.
https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: const float kHighGain = 2.885; On 2017/06/05 15:04:51, philipel wrote: > Comment where you got these values from. Done. https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:56: // Check if new round started for the connection. On 2017/06/05 15:04:51, philipel wrote: > Don't split comments into unnecessarily many lines, use the 80 char limit Done. https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:68: if (new_round_started && !full_bandwidth_reached_) On 2017/06/05 15:04:50, philipel wrote: > When the if statement or the following expression is more than one line use {} Done. https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:86: TryEnteringOrExitingProbeRtt(now); On 2017/06/05 15:04:51, philipel wrote: > Should this function always be called? Yes,if min_rtt value expired it will start ProbeRtt mode,if our current mode is ProbeRtt it will try to exit. https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:123: (static_cast<const MediaPacket*>(packets.back()))->sequence_number(); On 2017/06/05 15:04:51, philipel wrote: > remove one set of () Done. https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:29: class MaxBandwidthFilter { On 2017/06/05 15:04:51, philipel wrote: > Please implement the full MaxBandwidthFilter class. I think it would be better to add functions from time to time,as I am not sure about some details yet.
https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: // 2/ln(2)=2.885,because derivative of 2^t = 2/ln(2) + constant. space after comma https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: // 2/ln(2)=2.885,because derivative of 2^t = 2/ln(2) + constant. Please clarify your explanation. The derivative of 2^t is ln(2)*2^t https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:121: void BbrBweSender::TryEnteringOrExitingProbeRtt(int64_t now) {} Do we really want one function that both tries to enter and exit ProbeRtt? Doesn't the caller know whether it is trying to start or stop? https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:73: uint64_t rt_count_; round_trip_count_ ? https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:78: bool full_bandwidth_reached_; I don't understand what this variable represents. Please either rename of add a comment.
https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:86: TryEnteringOrExitingProbeRtt(now); On 2017/06/07 07:52:00, gnish1 wrote: > On 2017/06/05 15:04:51, philipel wrote: > > Should this function always be called? > > Yes,if min_rtt value expired it will start ProbeRtt mode,if our current mode is > ProbeRtt it will try to exit. Is this equivalent to: ... case PROBE_RTT: TryExitProbeRtt(); break; } TryEnterProbeRtt()
https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: // 2/ln(2)=2.885,because derivative of 2^t = 2/ln(2) + constant. On 2017/06/07 08:11:22, terelius wrote: > space after comma Done. https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:29: // 2/ln(2)=2.885,because derivative of 2^t = 2/ln(2) + constant. On 2017/06/07 08:11:22, terelius wrote: > Please clarify your explanation. The derivative of 2^t is ln(2)*2^t Done. https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:121: void BbrBweSender::TryEnteringOrExitingProbeRtt(int64_t now) {} On 2017/06/07 08:11:22, terelius wrote: > Do we really want one function that both tries to enter and exit ProbeRtt? > Doesn't the caller know whether it is trying to start or stop? Done. https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:73: uint64_t rt_count_; On 2017/06/07 08:11:22, terelius wrote: > round_trip_count_ ? Should be round_count,updated. https://codereview.webrtc.org/2924603002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:78: bool full_bandwidth_reached_; On 2017/06/07 08:11:22, terelius wrote: > I don't understand what this variable represents. Please either rename of add a > comment. Done.
https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:29: class MaxBandwidthFilter { On 2017/06/07 07:52:00, gnish1 wrote: > On 2017/06/05 15:04:51, philipel wrote: > > Please implement the full MaxBandwidthFilter class. > > I think it would be better to add functions from time to time,as I am not sure > about some details yet. The MaxBandwidthFilter should only be a max filter, no? Or is there some BBR specific logic related to this class? https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: // BBR uses this value to double sending rate each round trip. 80 chars https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:38: } // namespace Add empty line after anonymous namespace. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:60: const std::vector<std::pair<uint64_t, int64_t>>& feedback_vector = What values does the std::pair<uint64_t, uint64_t> contain? Maybe change this to a small struct to make it clearer? https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:62: // Check if new round started for the connection. Char limit https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:79: // If optimal bandwidth has been discovered and reached, Use the full 80 char limit for comments. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:14: #include <time.h> What do you use time.h for? Can it be removed? https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:31: // Minimal bandwidth necessary to assume that char limit
https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: // BBR uses this value to double sending rate each round trip. On 2017/06/07 11:34:31, philipel wrote: > 80 chars Done. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:38: } // namespace On 2017/06/07 11:34:31, philipel wrote: > Add empty line after anonymous namespace. Done. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:60: const std::vector<std::pair<uint64_t, int64_t>>& feedback_vector = On 2017/06/07 11:34:31, philipel wrote: > What values does the std::pair<uint64_t, uint64_t> contain? Maybe change this to > a small struct to make it clearer? Done. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:62: // Check if new round started for the connection. On 2017/06/07 11:34:31, philipel wrote: > Char limit Done. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:79: // If optimal bandwidth has been discovered and reached, On 2017/06/07 11:34:31, philipel wrote: > Use the full 80 char limit for comments. Done. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:14: #include <time.h> On 2017/06/07 11:34:31, philipel wrote: > What do you use time.h for? Can it be removed? Done. https://codereview.webrtc.org/2924603002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:31: // Minimal bandwidth necessary to assume that On 2017/06/07 11:34:31, philipel wrote: > char limit Done.
I think you should go through all files to make sure you don't #include anything you don't use. https://codereview.webrtc.org/2924603002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:15: #include "webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h" Why is mock_rtc_event_log.h and send_time_history.h included?
https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:14: #include <time.h> What is time used for? https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:15: #include <climits> Looks like more unused #includes :) https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:15: #include <memory> Why is memory included?
https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:14: #include <time.h> On 2017/06/08 14:12:34, philipel wrote: > What is time used for? Done. https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:15: #include <climits> On 2017/06/08 14:12:34, philipel wrote: > Looks like more unused #includes :) Done. https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:15: #include <memory> On 2017/06/08 14:12:34, philipel wrote: > Why is memory included? Done.
lgtm
Right now, multiple parts of the estimator are "semi-implemented"; there's more code than just the interfaces, but not enough code to actually make any component work. This makes it very hard to review the changes. I would suggest a bottom-up approach; write a complete implementation of the MaxBandwidthFilter together with unit tests and submit as a separate CL. Similarly for the other components. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:159: const std::vector<uint64_t>& packet_feedback_vector) Do we want to take const std::vector<PacketFeedback>& packet_feedback_vector to get the same interface as the current send side estimator? Or is that not worth the effort? https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:68: full_bandwidth_reached_ = max_bandwidth_filter_->FullBandwidthReached( max_bandwidth_filter hasn't been initialized here. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:22: int64_t GetCongestionWindow(); What does this function return? https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:33: // fixed value for past x rounds and y ms. nit: "Returns" Where are x and y configured? https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:31: void AddBandwidthSample(int64_t sample, int64_t round); Does this compile? Where is AddBandwidthSample implemented? https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:35: bool FullBandwidthReached(float growth_target, int max_rounds_without_growth); Does "full bandwidth reached" mean "we're using the full capacity of the link"? https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:25: // Checks whether or last discovered min_rtt value is older than x seconds. nit: "whether or not"
https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:159: const std::vector<uint64_t>& packet_feedback_vector) On 2017/06/09 14:02:54, terelius wrote: > Do we want to take > const std::vector<PacketFeedback>& packet_feedback_vector > to get the same interface as the current send side estimator? > Or is that not worth the effort? Done. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:68: full_bandwidth_reached_ = max_bandwidth_filter_->FullBandwidthReached( On 2017/06/09 14:02:54, terelius wrote: > max_bandwidth_filter hasn't been initialized here. Done. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:22: int64_t GetCongestionWindow(); On 2017/06/09 14:02:54, terelius wrote: > What does this function return? Size of the congestion window, for the current moment. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:33: // fixed value for past x rounds and y ms. On 2017/06/09 14:02:54, terelius wrote: > nit: "Returns" > Where are x and y configured? This function is no longer necessary, it has been replaced with another one. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:31: void AddBandwidthSample(int64_t sample, int64_t round); On 2017/06/09 14:02:54, terelius wrote: > Does this compile? Where is AddBandwidthSample implemented? It does compile, MaxBandwidthFilter class will be fully implemented in the next cl. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:35: bool FullBandwidthReached(float growth_target, int max_rounds_without_growth); On 2017/06/09 14:02:54, terelius wrote: > Does "full bandwidth reached" mean "we're using the full capacity of the link"? Yes, it means that we found an estimate which is close to the full capacity. https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2924603002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:25: // Checks whether or last discovered min_rtt value is older than x seconds. On 2017/06/09 14:02:54, terelius wrote: > nit: "whether or not" Done.
ping
ping
lgtm
ping
ping
Please add some more datails in the title of this issue. Where are we adding four functions? :) Otherwise rs-lgtm
Also add a BUG=
Description was changed from ========== Added implementation of four functions: 1) Function responsible for receiving feedback, digesting data and deciding switch scenarios. 2) Function which enters Startup mode. 3) Function which exits Startup mode. 4) Function which calculates, whether or not full bandwidth is reached. ========== to ========== Added implementation of four functions in the BBR congestion controller: 1) Function responsible for receiving feedback, digesting data and deciding switch scenarios. 2) Function which enters Startup mode. 3) Function which exits Startup mode. 4) Function which calculates, whether or not full bandwidth is reached. BUG=webrtc:7713 ==========
The CQ bit was checked by gnish@webrtc.org
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/2924603002/#ps120001 (title: "Updated according to review.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/26018)
The CQ bit was checked by gnish@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2924603002/#ps160001 (title: "f removed.")
The CQ bit was unchecked by gnish@webrtc.org
The CQ bit was checked by gnish@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499254786052720,
"parent_rev": "bc0c4f581fbe43c4f55a8a4c23a30b406a4bc2e6", "commit_rev":
"191113a46bd139fda0368eb1be470acc56b5f28c"}
Message was sent while issue was closed.
Description was changed from ========== Added implementation of four functions in the BBR congestion controller: 1) Function responsible for receiving feedback, digesting data and deciding switch scenarios. 2) Function which enters Startup mode. 3) Function which exits Startup mode. 4) Function which calculates, whether or not full bandwidth is reached. BUG=webrtc:7713 ========== to ========== Added implementation of four functions in the BBR congestion controller: 1) Function responsible for receiving feedback, digesting data and deciding switch scenarios. 2) Function which enters Startup mode. 3) Function which exits Startup mode. 4) Function which calculates, whether or not full bandwidth is reached. BUG=webrtc:7713 Review-Url: https://codereview.webrtc.org/2924603002 Cr-Commit-Position: refs/heads/master@{#18901} Committed: https://chromium.googlesource.com/external/webrtc/+/191113a46bd139fda0368eb1b... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/191113a46bd139fda0368eb1b... |
