Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2990163002:
Almost full implementation of BBR's core. (Closed)
DescriptionAlmost 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)
Description was changed from ========== Almost full BBR 2. gnish stuff BUG= ========== to ========== 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 ==========
Description was changed from ========== Almost full BBR 2. gnish stuff BUG= ========== to ========== 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 ==========
gnish@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
gnish@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:41: const int kMinimumCongestionWindow = 4000; kMinimumCongestionWindow should not be defined in this file. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:123: void BbrBweSender::HandleLoss(uint64_t last_acked_packet, So, in the case of loss we immediately mark the lost packet as acked? Please add a comment about that. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:128: for (uint64_t i = last_acked_packet + 1; i < recently_acked_packet; i++) Are |last_acked_packet| and |recently_acked_packet| wrapping numbers, in that case this won't work (if last is 65530 and recently is 0 for example). You should use webrtc/modules/sequence_numer_util.h to handle wrapping numbers. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:135: last_round = past_rtts_.rbegin()->round; ... = past_rtts_.back().round; https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:139: BbrBweSender::AverageRtt& last_round_sample = *(past_rtts_.rbegin()); BbrBweSender::AverageRtt* = &past_rtts_.back(); We only use const references, if you need to change the value make it a pointer. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:160: for (const auto& f : feedback_vector) { We use auto for situations where it is both obvious which type it is and the type is long to type, in this case I think it should be: for (uint64_t f : feedback_vector) { https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:162: PacketStats& packet = packet_stats_[f]; PacketStats* https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:207: TryExitingRecovery(); I think having |new_round_started| and |full_bandwidth_reached| as parameters to TryExitRecovery would be cleaner since you can move this to the switch statement below. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:293: int64_t now, now_ms https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:297: for (const auto& f : feedback_vector) { for (uint64_t f: feedback_vector) https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:384: void BbrBweSender::TryEnteringProbeRtt(int64_t now_ms, bool min_rtt_expired) { This looks like a revert of the changes you made in your last CL. Undo this change. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:398: bool min_rtt_expired) { Undo this change. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:400: if (congestion_window_->data_inflight() <= kMinimumCongestionWindow) { Undo. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:420: for (const auto i : past_rtts_) { Don't use auto. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:71: // Send time of the packet which was sent, when current packet got acked. I don't quite understand this comment. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:73: // The actual send time of the packet. Also not that clear, maybe explaining the difference between the two might help. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:83: size_t data_acked_before; Please add comments for all the member variables. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:90: int64_t sum_of_rtts; miliseconds? microseconds? please add _ms https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:92: int64_t quantity; num_samples https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:121: int64_t time_elapsed_1, Is this the delta send time? https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:123: int64_t time_elapsed_2); and this the delta receive time? https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:174: int64_t pacing_rate_; bps? https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:178: rtc::Optional<int64_t> first_high_gain_send_time_; Maybe |high_gain_start_ms_|? WDYT? Please also take a look at the name of the members declared below. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:222: // int64_t last_feedback_ms_; Remove unused member. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:29: void add_rtt_sample(int64_t rtt_ms, int64_t now_ms, bool min_rtt_expired) { AddRttSample Also, it should not take |min_rtt_expired| as an argument, just use the MinRttExpired function below instead. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:39: bool min_rtt_expired(int64_t now_ms, int64_t min_rtt_filter_window_size_ms) { MinRttExpired.
https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:41: const int kMinimumCongestionWindow = 4000; On 2017/08/04 12:08:06, philipel wrote: > kMinimumCongestionWindow should not be defined in this file. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:123: void BbrBweSender::HandleLoss(uint64_t last_acked_packet, On 2017/08/04 12:08:07, philipel wrote: > So, in the case of loss we immediately mark the lost packet as acked? Please add > a comment about that. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:128: for (uint64_t i = last_acked_packet + 1; i < recently_acked_packet; i++) On 2017/08/04 12:08:06, philipel wrote: > Are |last_acked_packet| and |recently_acked_packet| wrapping numbers, in that > case this won't work (if last is 65530 and recently is 0 for example). > > You should use webrtc/modules/sequence_numer_util.h to handle wrapping numbers. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:135: last_round = past_rtts_.rbegin()->round; On 2017/08/04 12:08:06, philipel wrote: > ... = past_rtts_.back().round; Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:139: BbrBweSender::AverageRtt& last_round_sample = *(past_rtts_.rbegin()); On 2017/08/04 12:08:06, philipel wrote: > BbrBweSender::AverageRtt* = &past_rtts_.back(); > > We only use const references, if you need to change the value make it a pointer. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:160: for (const auto& f : feedback_vector) { On 2017/08/04 12:08:06, philipel wrote: > We use auto for situations where it is both obvious which type it is and the > type is long to type, in this case I think it should be: > for (uint64_t f : feedback_vector) { Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:162: PacketStats& packet = packet_stats_[f]; On 2017/08/04 12:08:06, philipel wrote: > PacketStats* Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:207: TryExitingRecovery(); On 2017/08/04 12:08:06, philipel wrote: > I think having |new_round_started| and |full_bandwidth_reached| as parameters to > TryExitRecovery would be cleaner since you can move this to the switch statement > below. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:293: int64_t now, On 2017/08/04 12:08:06, philipel wrote: > now_ms Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:297: for (const auto& f : feedback_vector) { On 2017/08/04 12:08:06, philipel wrote: > for (uint64_t f: feedback_vector) Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:384: void BbrBweSender::TryEnteringProbeRtt(int64_t now_ms, bool min_rtt_expired) { On 2017/08/04 12:08:06, philipel wrote: > This looks like a revert of the changes you made in your last CL. Undo this > change. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:398: bool min_rtt_expired) { On 2017/08/04 12:08:06, philipel wrote: > Undo this change. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:400: if (congestion_window_->data_inflight() <= kMinimumCongestionWindow) { On 2017/08/04 12:08:06, philipel wrote: > Undo. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:420: for (const auto i : past_rtts_) { On 2017/08/04 12:08:06, philipel wrote: > Don't use auto. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:71: // Send time of the packet which was sent, when current packet got acked. On 2017/08/04 12:08:07, philipel wrote: > I don't quite understand this comment. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:73: // The actual send time of the packet. On 2017/08/04 12:08:07, philipel wrote: > Also not that clear, maybe explaining the difference between the two might help. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:83: size_t data_acked_before; On 2017/08/04 12:08:07, philipel wrote: > Please add comments for all the member variables. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:90: int64_t sum_of_rtts; On 2017/08/04 12:08:08, philipel wrote: > miliseconds? microseconds? please add _ms Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:92: int64_t quantity; On 2017/08/04 12:08:07, philipel wrote: > num_samples Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:121: int64_t time_elapsed_1, On 2017/08/04 12:08:07, philipel wrote: > Is this the delta send time? Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:123: int64_t time_elapsed_2); On 2017/08/04 12:08:07, philipel wrote: > and this the delta receive time? Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:174: int64_t pacing_rate_; On 2017/08/04 12:08:07, philipel wrote: > bps? Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:178: rtc::Optional<int64_t> first_high_gain_send_time_; On 2017/08/04 12:08:07, philipel wrote: > Maybe |high_gain_start_ms_|? WDYT? > > Please also take a look at the name of the members declared below. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:222: // int64_t last_feedback_ms_; On 2017/08/04 12:08:07, philipel wrote: > Remove unused member. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:29: void add_rtt_sample(int64_t rtt_ms, int64_t now_ms, bool min_rtt_expired) { On 2017/08/04 12:08:08, philipel wrote: > AddRttSample > > Also, it should not take |min_rtt_expired| as an argument, just use the > MinRttExpired function below instead. Done. https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:39: bool min_rtt_expired(int64_t now_ms, int64_t min_rtt_filter_window_size_ms) { On 2017/08/04 12:08:08, philipel wrote: > MinRttExpired. Done.
subtle ping
https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/1/webrtc/modules/remote_bitrate... 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 2017/08/04 12:08:07, philipel wrote: > > Please add comments for all the member variables. > > Done. If it's hard to find good names for the members then you have to add comments. https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:319: min_rtt_filter_->AddRttSample(*min_rtt_sample_ms, now_ms); Don't you want to reverse line 318 and 319? https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:60: int64_t last_sent_packet_send_time_ms_, I would suggest |time_last_sent_packet_ms_| or just |last_sent_packet_ms_|. https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:81: uint16_t sequence_number; To me it's still not clear what these member variables represent.
https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... 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 you want to reverse line 318 and 319? Done. https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:60: int64_t last_sent_packet_send_time_ms_, On 2017/08/08 11:54:54, philipel wrote: > I would suggest |time_last_sent_packet_ms_| or just |last_sent_packet_ms_|. In that case it isn't clear whether we mean send time or akc time. https://codereview.webrtc.org/2990163002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:81: uint16_t sequence_number; On 2017/08/08 11:54:54, philipel wrote: > To me it's still not clear what these member variables represent. Done.
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:39: // Least amount number of rounds PROBE_RTT should last. nit: Remove "amount"? https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:58: // Threshold to assume average rtt has increased for a round. Chosen by rtt -> RTT. Same below. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:63: const float kRttDecreaseThreshold = 1.5f; I wouldn't call it a "decrease" if the RTT is 1.5 times larger than the minimum. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:122: AheadOrAt<uint64_t>(recently_acked_packet - 1, i); i++) Is it safe to subtract one here, or could the subtraction underflow? https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:122: AheadOrAt<uint64_t>(recently_acked_packet - 1, i); i++) I'd prefer curly braces around the loop body because of the line wrap in the loop condition. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:152: for (uint64_t f : feedback_vector) { Better name than f? https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:271: int64_t send_time_delta_ms = last_packet_send_time_during_high_gain_ms_ - Won't this overestimate the bandwidth? If we are sending one packet of size 1 kilobyte every 100 ms, then we should be sending 8 kb / 0.1 s = 80 kbps. However, if the high gain phase consists of 3 packets, it seems your calculation would measure the time between the first and the last packet as 200 ms so the rate would be 24 kb / 0.2 s = 120 kbps. Same for the ack_time_delta. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:52: // cur_packet - the packet for which PacketStats is stored. Is this the same as sequence_number_ below? https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:98: // Sum of rtts over the round. nit: I think we generally capitalize RTT in comments. i.e. rtts -> RTTs https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:100: // Number of rtt samples over the round. RTT https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:142: // declare those packets as lost immediately. What happens in case of reordering on the network? https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:35: if (!min_rtt_ms_ || rtt_ms <= *min_rtt_ms_ || MinRttExpired(now_ms)) { Should expiring the lowest RTT really expire all other RTT measurements?
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:39: // Least amount number of rounds PROBE_RTT should last. On 2017/08/08 17:32:40, terelius wrote: > nit: Remove "amount"? Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:58: // Threshold to assume average rtt has increased for a round. Chosen by On 2017/08/08 17:32:40, terelius wrote: > rtt -> RTT. Same below. Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:63: const float kRttDecreaseThreshold = 1.5f; On 2017/08/08 17:32:40, terelius wrote: > I wouldn't call it a "decrease" if the RTT is 1.5 times larger than the minimum. Yes, but I didn't want to create another mode similar to PROBE_RTT, so I try to exit it as soon as I see that RTTs have started to decrease and "hope" that they will continue to do so. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:122: AheadOrAt<uint64_t>(recently_acked_packet - 1, i); i++) On 2017/08/08 17:32:40, terelius wrote: > I'd prefer curly braces around the loop body because of the line wrap in the > loop condition. Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:122: AheadOrAt<uint64_t>(recently_acked_packet - 1, i); i++) On 2017/08/08 17:32:40, terelius wrote: > Is it safe to subtract one here, or could the subtraction underflow? It will underflow, but it is safe, the loop will work as intended. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:152: for (uint64_t f : feedback_vector) { On 2017/08/08 17:32:40, terelius wrote: > Better name than f? Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... 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/08 17:32:40, terelius wrote: > Won't this overestimate the bandwidth? If we are sending one packet of size 1 > kilobyte every 100 ms, then we should be sending 8 kb / 0.1 s = 80 kbps. > However, if the high gain phase consists of 3 packets, it seems your calculation > would measure the time between the first and the last packet as 200 ms so the > rate would be 24 kb / 0.2 s = 120 kbps. > > Same for the ack_time_delta. It could overestimate bw, as normal bw calculation could, if we build big queues, Recovery mode should get triggered, if not drain gain should drain the queues created during cycle_gain, it isn't perfect but there is no way around it. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:52: // cur_packet - the packet for which PacketStats is stored. On 2017/08/08 17:32:40, terelius wrote: > Is this the same as sequence_number_ below? Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:98: // Sum of rtts over the round. On 2017/08/08 17:32:40, terelius wrote: > nit: I think we generally capitalize RTT in comments. i.e. rtts -> RTTs Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:100: // Number of rtt samples over the round. On 2017/08/08 17:32:40, terelius wrote: > RTT Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:142: // declare those packets as lost immediately. On 2017/08/08 17:32:40, terelius wrote: > What happens in case of reordering on the network? I would assume current implementation will not handle that. What I wanted was a simple way to handle loss, and in simulation tool it appears, that when we have a gap this automatically implies that there is a loss. I couldn't find another way to handle loss in the tool, and this solution was proposed. https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:35: if (!min_rtt_ms_ || rtt_ms <= *min_rtt_ms_ || MinRttExpired(now_ms)) { On 2017/08/08 17:32:41, terelius wrote: > Should expiring the lowest RTT really expire all other RTT measurements? As discussed, this could be changed depending on the implementation.
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... 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 wrote: > On 2017/08/08 17:32:40, terelius wrote: > > Won't this overestimate the bandwidth? If we are sending one packet of size 1 > > kilobyte every 100 ms, then we should be sending 8 kb / 0.1 s = 80 kbps. > > However, if the high gain phase consists of 3 packets, it seems your > calculation > > would measure the time between the first and the last packet as 200 ms so the > > rate would be 24 kb / 0.2 s = 120 kbps. > > > > Same for the ack_time_delta. > > It could overestimate bw, as normal bw calculation could, if we build big > queues, Recovery mode should get triggered, if not drain gain should drain the > queues created during cycle_gain, it isn't perfect but there is no way around > it. What I am saying is that the calculation seems to always overestimate the bandwidth compared to what the data actually suggests. One way to "unbias" the estimator is to only count half of the data size for the first and last packet. For the example above, instead of counting 24 kb sent in 0.2 seconds, we'd count 4+8+4 = 16 kb in 0.2 seconds. Another way would be to count the interval from the first packet in high gain until first packet not in high gain, rather than until last packet in high gain. In the example above, this would give 24 kb / 0.3 seconds = 80 kbps. https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h (right): https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/min_rtt_filter.h:35: if (!min_rtt_ms_ || rtt_ms <= *min_rtt_ms_ || MinRttExpired(now_ms)) { On 2017/08/09 10:05:53, gnish1 wrote: > On 2017/08/08 17:32:41, terelius wrote: > > Should expiring the lowest RTT really expire all other RTT measurements? > > As discussed, this could be changed depending on the implementation. Acknowledged.
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:142: // declare those packets as lost immediately. On 2017/08/09 10:05:53, gnish1 wrote: > On 2017/08/08 17:32:40, terelius wrote: > > What happens in case of reordering on the network? > > I would assume current implementation will not handle that. What I wanted was a > simple way to handle loss, and in simulation tool it appears, that when we have > a gap this automatically implies that there is a loss. I couldn't find another > way to handle loss in the tool, and this solution was proposed. Can you check if there is a "ReorderingFilter" or something similar for the simulation tool, and in that case run a simulation with reordering just to verify that nothing really bad happens?
https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... 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 wrote: > On 2017/08/09 10:05:53, gnish1 wrote: > > On 2017/08/08 17:32:40, terelius wrote: > > > Won't this overestimate the bandwidth? If we are sending one packet of size > 1 > > > kilobyte every 100 ms, then we should be sending 8 kb / 0.1 s = 80 kbps. > > > However, if the high gain phase consists of 3 packets, it seems your > > calculation > > > would measure the time between the first and the last packet as 200 ms so > the > > > rate would be 24 kb / 0.2 s = 120 kbps. > > > > > > Same for the ack_time_delta. > > > > It could overestimate bw, as normal bw calculation could, if we build big > > queues, Recovery mode should get triggered, if not drain gain should drain the > > queues created during cycle_gain, it isn't perfect but there is no way around > > it. > > What I am saying is that the calculation seems to always overestimate the > bandwidth compared to what the data actually suggests. One way to "unbias" the > estimator is to only count half of the data size for the first and last packet. > For the example above, instead of counting 24 kb sent in 0.2 seconds, we'd count > 4+8+4 = 16 kb in 0.2 seconds. Another way would be to count the interval from > the first packet in high gain until first packet not in high gain, rather than > until last packet in high gain. In the example above, this would give 24 kb / > 0.3 seconds = 80 kbps. > > Done. https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h (right): https://codereview.webrtc.org/2990163002/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.h:142: // declare those packets as lost immediately. On 2017/08/11 09:31:36, terelius wrote: > On 2017/08/09 10:05:53, gnish1 wrote: > > On 2017/08/08 17:32:40, terelius wrote: > > > What happens in case of reordering on the network? > > > > I would assume current implementation will not handle that. What I wanted was > a > > simple way to handle loss, and in simulation tool it appears, that when we > have > > a gap this automatically implies that there is a loss. I couldn't find another > > way to handle loss in the tool, and this solution was proposed. > > Can you check if there is a "ReorderingFilter" or something similar for the > simulation tool, and in that case run a simulation with reordering just to > verify that nothing really bad happens? I couldn't find anything similar, will check with Stefan, but his proposal was to implement handling packet loss in a way it is implemented. So I believe nothing really bad should happen. https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc (right): https://codereview.webrtc.org/2990163002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc:122: AheadOrAt<uint64_t>(recently_acked_packet - 1, i); i++) Should be AheadOrAt<uint16_t>.
lgtm
lgtm
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/20117)
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/2990163002/#ps140001 (title: "fixed patch failure")
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": 1502786723161670,
"parent_rev": "2ee076dfa35014adc7869972da6454bf2391c223", "commit_rev":
"53d76c619034785ce3d1472539d0792194a4a0ce"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/53d76c619034785ce3d147253... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/53d76c619034785ce3d147253... |
