Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966403002:
Added implementation of three classes in BBR,with unit-tests. (Closed)
DescriptionAdded implementation of three classes:
1) MaxBandwidthFilter
2) MinRttFilter
3) CongestionWindow
Added unit-tests for those classes.
BUG=webrtc:7713
Review-Url: https://codereview.webrtc.org/2966403002
Cr-Commit-Position: refs/heads/master@{#19067}
Committed: https://chromium.googlesource.com/external/webrtc/+/157cbbd3a734150aa702a4b46cd6d1eb10dc7be8
Patch Set 1 #
Total comments: 38
Patch Set 2 : Updated according to reviews. #
Total comments: 6
Patch Set 3 : Minor changes #
Total comments: 16
Patch Set 4 : Changed names of some variables. #
Total comments: 2
Patch Set 5 : Variables' names changed, added units in which they are measured. #
Total comments: 1
Patch Set 6 : Missed some variables, now it should be all good. #
Total comments: 3
Patch Set 7 : Unittests. #Patch Set 8 : Bot fix. #Patch Set 9 : Added logic for entering/exiting modes in BBR, added new bandwidth filter. #Messages
Total messages: 32 (13 generated)
Description was changed from ========== Unittests. BUG= ========== to ========== Added implementation of three classes: 1) MaxBandwidthFilter 2) MinRttFilter 3) CongestionWindow Added unit-tests for those classes. BUG=webrtc:7713 ==========
gnish@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:21: const int64_t kStartingCongestionWindow = 6000; Add comments about what these values mean and where you got them from. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:21: const int64_t kStartingCongestionWindow = 6000; Put const values inside an anonymous namespace. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:34: if (mode == 3) Use enum, not '3'. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:53: if (!congestion_window) { remove {} https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:53: if (!congestion_window) { How/why can the congestion window be 0? Add comment. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:16: #include <cstdint> Do you need to include both of these? https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:27: int64_t GetCongestionWindow(BbrBweSender::Mode mode, Why int64_t as return type? Can the congestion window be 0 or negative, and if so, what does it mean? Maybe we should rtc::Optional instead? https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window_unittest.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window_unittest.cc:19: const int64_t kStartingCongestionWindow = 6000; anonymous namespace, comment where you got this from. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:25: void MaxBandwidthFilter::AddBandwidthSample(int64_t sample, what is sample in this case? bps? kbps? https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:26: int64_t round, what is round? rtt_ms? https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:27: size_t filter_size) { bytes? ms? https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:16: #include <cstdint> Do you need both? https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:12: #include "webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h" This class is small enough to not have a .cc file, inline everything in the .h file instead. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:17: MinRttFilter::MinRttFilter() : min_rtt_(10000000), discovery_time_(0) {} Use std::numeric_limits<int64_t>::max() instead of 10000000. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:19: void MinRttFilter::UpdateMinRtt(int64_t min_rtt) { Don't you want a AddRttSample(int64_t rtt_ms, int64_t now_ms) function instead? Currently this class does not have the functionality I would expect of a min filter. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:24: int64_t min_rtt() { return min_rtt_; } Add newline between line 23 24.
https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:20: min_rtt_ = min_rtt; This doesn't look like a min filter?
https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:21: const int64_t kStartingCongestionWindow = 6000; On 2017/07/06 12:15:14, philipel wrote: > Put const values inside an anonymous namespace. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:21: const int64_t kStartingCongestionWindow = 6000; On 2017/07/06 12:15:14, philipel wrote: > Put const values inside an anonymous namespace. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:21: const int64_t kStartingCongestionWindow = 6000; On 2017/07/06 12:15:14, philipel wrote: > Put const values inside an anonymous namespace. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:21: const int64_t kStartingCongestionWindow = 6000; On 2017/07/06 12:15:14, philipel wrote: > Add comments about what these values mean and where you got them from. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:34: if (mode == 3) On 2017/07/06 12:15:15, philipel wrote: > Use enum, not '3'. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:53: if (!congestion_window) { On 2017/07/06 12:15:14, philipel wrote: > How/why can the congestion window be 0? Add comment. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:53: if (!congestion_window) { On 2017/07/06 12:15:14, philipel wrote: > remove {} Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:16: #include <cstdint> On 2017/07/06 12:15:15, philipel wrote: > Do you need to include both of these? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:27: int64_t GetCongestionWindow(BbrBweSender::Mode mode, On 2017/07/06 12:15:15, philipel wrote: > Why int64_t as return type? Can the congestion window be 0 or negative, and if > so, what does it mean? Maybe we should rtc::Optional instead? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window_unittest.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window_unittest.cc:19: const int64_t kStartingCongestionWindow = 6000; On 2017/07/06 12:15:15, philipel wrote: > anonymous namespace, comment where you got this from. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:25: void MaxBandwidthFilter::AddBandwidthSample(int64_t sample, On 2017/07/06 12:15:15, philipel wrote: > what is sample in this case? bps? kbps? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:26: int64_t round, On 2017/07/06 12:15:15, philipel wrote: > what is round? rtt_ms? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:27: size_t filter_size) { On 2017/07/06 12:15:15, philipel wrote: > bytes? ms? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:16: #include <cstdint> On 2017/07/06 12:15:15, philipel wrote: > Do you need both? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:12: #include "webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h" On 2017/07/06 12:15:15, philipel wrote: > This class is small enough to not have a .cc file, inline everything in the .h > file instead. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:17: MinRttFilter::MinRttFilter() : min_rtt_(10000000), discovery_time_(0) {} On 2017/07/06 12:15:16, philipel wrote: > Use std::numeric_limits<int64_t>::max() instead of 10000000. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:19: void MinRttFilter::UpdateMinRtt(int64_t min_rtt) { On 2017/07/06 12:15:16, philipel wrote: > Don't you want a AddRttSample(int64_t rtt_ms, int64_t now_ms) function instead? > Currently this class does not have the functionality I would expect of a min > filter. Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.cc:20: min_rtt_ = min_rtt; On 2017/07/06 13:39:25, stefan-webrtc wrote: > This doesn't look like a min filter? Done. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:24: int64_t min_rtt() { return min_rtt_; } On 2017/07/06 12:15:16, philipel wrote: > Add newline between line 23 24. Done.
https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:26: int64_t round, On 2017/07/07 13:43:34, gnish1 wrote: > On 2017/07/06 12:15:15, philipel wrote: > > what is round? rtt_ms? > > Done. Is round the same as rtt? If so, change to rtt_ms. https://codereview.webrtc.org/2966403002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:27: size_t filter_size) { On 2017/07/07 13:43:34, gnish1 wrote: > On 2017/07/06 12:15:15, philipel wrote: > > bytes? ms? > > Done. What does filter_size_round represent? https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:27: void MaxBandwidthFilter::AddBandwidthSample(int64_t sample_Bpms, If this is bytes per milliseconds, change it to |sample_bytes_per_ms| https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:16: #include <cstdint> Do we need to include both? https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:26: int64_t min_rtt() { return min_rtt_; } Does the MinRttFilter always have an rtt estimate? If not we should change this to return an rtc::Optional instead.
https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc (right): https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.cc:27: void MaxBandwidthFilter::AddBandwidthSample(int64_t sample_Bpms, On 2017/07/10 11:53:35, philipel wrote: > If this is bytes per milliseconds, change it to |sample_bytes_per_ms| Done. https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:16: #include <cstdint> On 2017/07/10 11:53:35, philipel wrote: > Do we need to include both? Done. https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:26: int64_t min_rtt() { return min_rtt_; } On 2017/07/10 11:53:35, philipel wrote: > Does the MinRttFilter always have an rtt estimate? If not we should change this > to return an rtc::Optional instead. Done.
https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: // experiments,according to the design document. Nit: ", " https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:26: const int kStartingCongestionWindow = 6000; Is there any reason for not setting it to the minimum possible, i.e. reuse kMinimumCongestionWindow? https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:45: multiplier * bytes_acked; Why do we add bytes_acked? https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:64: congestion_window = gain * kStartingCongestionWindow; Shouldn't the gain factor already be part of kStartingCongestionWindow? https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:27: float gain, The difference between gain and multiplier is not clear to me. Coiuld you give them more explicit names? https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:29: void add_rtt_sample(int64_t min_rtt, int64_t time_discovered) { Rename min_rtt to rtt? WDYT? Will time_discovered always be the current time? If so, rename it now_ms. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:38: // seconds. Seconds or milliseconds? https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:39: bool min_rtt_expired(int64_t now, int64_t min_rtt_filter_window_size_ms) { Please include units where appropriate, i.e. now_ms.
https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:28: // experiments,according to the design document. On 2017/07/12 10:34:50, terelius wrote: > Nit: ", " Done. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:26: const int kStartingCongestionWindow = 6000; On 2017/07/12 10:34:50, terelius wrote: > Is there any reason for not setting it to the minimum possible, i.e. reuse > kMinimumCongestionWindow? It depends on the situation, this variable shouldn't be used at all in most cases, it is just for safety, but it is different from kMinimumCongestionWindow, its value may change in the upcoming cl's but for now I don't see any clear reason to do so, it just exists because it is different from kMinimumCongestionWindow. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:45: multiplier * bytes_acked; On 2017/07/12 10:34:51, terelius wrote: > Why do we add bytes_acked? Done. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:64: congestion_window = gain * kStartingCongestionWindow; On 2017/07/12 10:34:50, terelius wrote: > Shouldn't the gain factor already be part of kStartingCongestionWindow? No, gain factor may change, so it is not possible to include it in kStartingCongestionWindow. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:27: float gain, On 2017/07/12 10:34:51, terelius wrote: > The difference between gain and multiplier is not clear to me. Coiuld you give > them more explicit names? Done. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:29: void add_rtt_sample(int64_t min_rtt, int64_t time_discovered) { On 2017/07/12 10:34:51, terelius wrote: > Rename min_rtt to rtt? WDYT? > Will time_discovered always be the current time? If so, rename it now_ms. Done. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:38: // seconds. On 2017/07/12 10:34:51, terelius wrote: > Seconds or milliseconds? Done. https://codereview.webrtc.org/2966403002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:39: bool min_rtt_expired(int64_t now, int64_t min_rtt_filter_window_size_ms) { On 2017/07/12 10:34:51, terelius wrote: > Please include units where appropriate, i.e. now_ms. Done.
lgtm
lg, but please go through the code and add units where possible. For example min_rtt --> min_rtt_ms, packet_size --> packet_size_bytes and so on. https://codereview.webrtc.org/2966403002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h (right): https://codereview.webrtc.org/2966403002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:25: int64_t bandwidth_estimate, Include unit when possible: bandwidth_estimate_bps? bandwidth_estimate_kbps? https://codereview.webrtc.org/2966403002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.h:26: int64_t min_rtt, min_rtt_ms
https://codereview.webrtc.org/2966403002/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc (right): https://codereview.webrtc.org/2966403002/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/congestion_window.cc:43: return GetTargetCongestionWindow(bandwidth_estimate_bytes_per_ms, min_rtt_ms, Measured in bytes/ms, the lowest rate you could estimate is 8 kbps. This isn't very good resolution. https://codereview.webrtc.org/2966403002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2966403002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:27: return max_bandwidth_estimate_bytes_per_ms_; Same here. https://codereview.webrtc.org/2966403002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2966403002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:29: void add_rtt_sample(int64_t rtt, int64_t now_ms) { rtt_ms
https://codereview.webrtc.org/2966403002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h (right): https://codereview.webrtc.org/2966403002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/test/estimators/max_bandwidth_filter.h:27: return max_bandwidth_estimate_bytes_per_ms_; On 2017/07/12 14:52:21, terelius wrote: > Same here. Done.
lgtm after some offline discussion
The CQ bit was checked by gnish@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2966403002/#ps120001 (title: "Unittests.")
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/20599) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/10706)
The CQ bit was checked by gnish@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
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, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2966403002/#ps140001 (title: "Bot fix.")
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": 140001, "attempt_start_ts": 1500371214207140,
"parent_rev": "3124cc6921dcc4bc774236dccec0f163e86b80e5", "commit_rev":
"157cbbd3a734150aa702a4b46cd6d1eb10dc7be8"}
Message was sent while issue was closed.
Description was changed from ========== Added implementation of three classes: 1) MaxBandwidthFilter 2) MinRttFilter 3) CongestionWindow Added unit-tests for those classes. BUG=webrtc:7713 ========== to ========== Added implementation of three classes: 1) MaxBandwidthFilter 2) MinRttFilter 3) CongestionWindow Added unit-tests for those classes. BUG=webrtc:7713 Review-Url: https://codereview.webrtc.org/2966403002 Cr-Commit-Position: refs/heads/master@{#19067} Committed: https://chromium.googlesource.com/external/webrtc/+/157cbbd3a734150aa702a4b46... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/157cbbd3a734150aa702a4b46... |
