Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380883003:
Add interval estimator to remote bitrate estimator (Closed)
DescriptionAdd interval estimator to remote bitrate estimator.
To be able to smooth the bandwidth estimation according to the probing interval.
BUG=webrtc:6443
Committed: https://crrev.com/4a4b3cfc01d265f8708f3010214c04c51cbcaff7
Cr-Commit-Position: refs/heads/master@{#15123}
Patch Set 1 #Patch Set 2 : Rebased and changed gtest includes #Patch Set 3 : Simplify probing estimator. #
Total comments: 6
Patch Set 4 : Response to an offline discussion. #
Total comments: 14
Patch Set 5 : Respond to comments. #Patch Set 6 : Move probing interval estimator to congestion controller. #
Total comments: 4
Patch Set 7 : Respond to comments. #
Total comments: 32
Patch Set 8 : Respond to comments. #
Total comments: 5
Patch Set 9 : Respond to comments. #Patch Set 10 : Fix gyp files. #Patch Set 11 : Rebased. #Messages
Total messages: 79 (48 generated)
Description was changed from ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. Add BWE probing interval estimator. BUG= ========== to ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. Add BWE probing interval estimator. BUG= ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org
The CQ bit was checked by minyue@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: Try jobs failed on following builders: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/13341) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/13273) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/18597) linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/5460) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/18048) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/14757) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18695)
Description was changed from ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. Add BWE probing interval estimator. BUG= ========== to ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. BUG= ==========
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/14683)
Description was changed from ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. BUG= ========== to ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. BUG=6443 ==========
minyue@webrtc.org changed reviewers: + stefan@webrtc.org
FYI, you have to press "Publish" for the reviewers to get notified. It's not enough to edit the issue and add the reviewers from there. Please add "webrtc:" in front of the number in BUG=.
Hi Stefan, Michael is on vacation but he told me that this CL is ready to review.
A few main questions before I go ahead and review the details: 1. Have you considered picking a window size for your bitrate averaging which is large enough to cover several intervals so that the inaccuracy caused by picking a fixed, (and therefore not entirely correct) interval is negligible? Seems like it would be a lot easier if we could simplify it that way, and avoid estimating the saw tooth altogether. It would also make things a lot simpler when we decide to change the bandwidth estimator. 2. Is it possible to express the increase rate in a fairly simple way since it's more or less constant if we're in additive increase mode? This is not as true for the decrease, which can vary more since it depends on the actual bitrate received, but in steady state it will be fairly stable too? Essentially, we should be able to use what's given by aimd_rate_control.cc, right? bits_per_frame = current_rate / 30 packets_per_frame = ceil(bits_per_frame / (8 * 1200)) increase = 0.5 * bits_per_frame / packets_per_frame decrease = 0.85 * incoming_bitrate A simplification could use incoming_rate = current_rate. Then you'd have something like: 51 / packets_per_frame = 51 / ceil(current_rate / 30 / (8 * 1200)) = 51 / ceil(320 * current_rate) I probably failed with the math there, but maybe that could be computed directly by AimdRateControl and provided via an API.
Description was changed from ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. BUG=6443 ========== to ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. BUG=webrtc:6443 ==========
1. Yes we have considered this, it was actually our first solution we implemented. The problem is that we like to react as fast as possible to BW changes. Which is hard if we have a very long smoothing time. 2. Yes this was my first idea as well but i run into soem problems: On "low bit-rates" (bw < 288000bps) the additive bit-rate is not relay additive. Because of packets_per_frame will be one. So we end-up with "bw = bw + max(1000, beta(rtt, updated_interval) * bw / 30)". We see that this function is dependent on the current bw, rtt and the updated_interval. To calculate the increase rate depending on this values, seamed no to be a good solution to me. Since the increase rate depend hardly on the internal impl of the AdditiveRateIncrease function.
Description was changed from ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the interval. BUG=webrtc:6443 ========== to ========== Add a probing interval estimator. BUG=webrtc:6443 ==========
Description was changed from ========== Add a probing interval estimator. BUG=webrtc:6443 ========== to ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 ==========
Description was changed from ========== Add a probing interval estimator to be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 ========== to ========== Add interval estimator to remote bitrate estimator. To be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 ==========
Uploaded a strongly simplified version of the probing interval estimator.
The CQ bit was checked by michaelt@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.
https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:179: (now_ms - *last_near_max_increase_rate_update_)); An alternative to this would be to do something like: std::min(now_ms - time_last_bitrate_change_, 500) or something like that. WDYT? https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:268: uint32_t additive_increase_bps = std::max( If we compute near_max_increase_rate_bps_ here instead we should be able to do: near_max_increase_rate_bps_ = 1000 * std::max(1000.0, avg_packet_size_bits) / response_time; to get the increase per second, right? https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc:38: auto drop_hight = aimd_rate_control_->GetLastDecrease(); drop_height I would also prefer not using auto for this fairly simple type. https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc:45: return rtc::Optional<int>(); Have the "expected path" return here instead. so basically: if (!drop_height || !increase_rate || *increase_rate <= 0) return ...; return ...;
https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:179: (now_ms - *last_near_max_increase_rate_update_)); On 2016/10/26 14:30:06, stefan-webrtc (holmer) wrote: > An alternative to this would be to do something like: > std::min(now_ms - time_last_bitrate_change_, 500) or something like that. I taught a little what would be the best solution. I would prefer to have a "NearMaxIncresaseRate" function which would calculate the actual increase rate depending on rtt,bitrate,.... This function would be then used in AdditiveRateIncrease and for the probing interval estimator. We would have to remove the min_increse per call, but we could replace it with a min_increase_rate. WDYT? https://codereview.webrtc.org/2380883003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:268: uint32_t additive_increase_bps = std::max( Without the max function it would be: near_max_increase_rate_bps_ = 1000 * avg_packet_size_bits / response_time;
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12181) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14516)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12189) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14525) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18441)
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2913)
https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:140: constexpr double kMinIncreaseRateBps = 4000; 4000 is larger than before when it was 1000, right? https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:212: if (incoming_bitrate_bps < current_bitrate_bps_) {} https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:255: return (GetNearMaxIncreaseRateBps() / 1000.0) * (now_ms - last_ms); You can remove () around GetNear... https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:91: } Thanks for adding tests! https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc:38: auto drop_hight = aimd_rate_control_->GetLastDecrease(); rtc::Optional<int> drop_height https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.h (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.h:27: const AimdRateControl* const aimd_rate_control); Do we need this constructor?
https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:140: constexpr double kMinIncreaseRateBps = 4000; No its actually smaller. Before we had 1kbps per call. And we called it with the current impl. in a interval of 200ms. So the increase rate per second was 5kbps. https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:212: if (incoming_bitrate_bps < current_bitrate_bps_) On 2016/11/08 13:06:28, stefan-webrtc (holmer) wrote: > {} Done. https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:255: return (GetNearMaxIncreaseRateBps() / 1000.0) * (now_ms - last_ms); On 2016/11/08 13:06:28, stefan-webrtc (holmer) wrote: > You can remove () around GetNear... Done. https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.cc:38: auto drop_hight = aimd_rate_control_->GetLastDecrease(); On 2016/11/08 13:06:28, stefan-webrtc (holmer) wrote: > rtc::Optional<int> drop_height Done. https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.h (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/probing_interval_estimator.h:27: const AimdRateControl* const aimd_rate_control); Yes, its used for testing.
Who will own the probing interval estimator? I would have expected it to live in DelayBasedBwe and therefore perhaps in modules/congestion_controller/? https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:140: constexpr double kMinIncreaseRateBps = 4000; On 2016/11/09 07:54:44, michaelt wrote: > No its actually smaller. > Before we had 1kbps per call. And we called it with the current impl. in a > interval of 200ms. > So the increase rate per second was 5kbps. Should we keep 5 kbps then? BTW, how come it was always 200 ms? Looks to me like it could vary? Probably not a big difference, so this is likely fine.
On 2016/11/09 18:01:11, stefan-webrtc (holmer) wrote: > Who will own the probing interval estimator? I would have expected it to live in > DelayBasedBwe and therefore perhaps in modules/congestion_controller/? > > https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... > File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): > > https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... > webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:140: constexpr > double kMinIncreaseRateBps = 4000; > On 2016/11/09 07:54:44, michaelt wrote: > > No its actually smaller. > > Before we had 1kbps per call. And we called it with the current impl. in a > > interval of 200ms. > > So the increase rate per second was 5kbps. > > Should we keep 5 kbps then? BTW, how come it was always 200 ms? Looks to me like > it could vary? Probably not a big difference, so this is likely fine. Yes probing interval estimator will be owed by DelayBasedBwe. So i moved it to congestion_controller.
https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:140: constexpr double kMinIncreaseRateBps = 4000; On 2016/11/09 18:01:10, stefan-webrtc (holmer) wrote: > On 2016/11/09 07:54:44, michaelt wrote: > > No its actually smaller. > > Before we had 1kbps per call. And we called it with the current impl. in a > > interval of 200ms. > > So the increase rate per second was 5kbps. > > Should we keep 5 kbps then? BTW, how come it was always 200 ms? Looks to me like > it could vary? Probably not a big difference, so this is likely fine. this 200ms are not const as you expected, they are dependent on GetFeedbackInterval (min interval 200ms) and on the TWCC report interval (50ms - 250ms). For small bit rates the interval is longer for large bitrates the interval is shorter. If you are OK with it I would let it for the moment stay at 4kbps. We will do some real call analyzes. With this CL to see how the system behaves.
lgtm % nits https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:140: constexpr double kMinIncreaseRateBps = 4000; On 2016/11/10 08:57:31, michaelt wrote: > On 2016/11/09 18:01:10, stefan-webrtc (holmer) wrote: > > On 2016/11/09 07:54:44, michaelt wrote: > > > No its actually smaller. > > > Before we had 1kbps per call. And we called it with the current impl. in a > > > interval of 200ms. > > > So the increase rate per second was 5kbps. > > > > Should we keep 5 kbps then? BTW, how come it was always 200 ms? Looks to me > like > > it could vary? Probably not a big difference, so this is likely fine. > this 200ms are not const as you expected, they are dependent on > GetFeedbackInterval (min interval 200ms) and on the TWCC report interval (50ms - > 250ms). For small bit rates the interval is longer for large bitrates the > interval is shorter. If you are OK with it I would let it for the moment stay at > 4kbps. We will do some real call analyzes. With this CL to see how the system > behaves. > > SG https://codereview.webrtc.org/2380883003/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:38: rtc::Optional<int> drop_hight = aimd_rate_control_->GetLastDecrease(); drop_height https://codereview.webrtc.org/2380883003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:40: if (drop_hight && increase_rate > 0) { Please change this as I suggested before: if (!drop_height || increase_rate <= 0) return rtc::Optional<int>(); return rtc::Optional<int>(std::min( max_interval_ms_, std::max(1000 * (*drop_hight) / increase_rate, min_interval_ms_)));
https://codereview.webrtc.org/2380883003/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:38: rtc::Optional<int> drop_hight = aimd_rate_control_->GetLastDecrease(); On 2016/11/14 14:53:48, stefan-webrtc (holmer) wrote: > drop_height Done. https://codereview.webrtc.org/2380883003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:40: if (drop_hight && increase_rate > 0) { On 2016/11/14 14:53:48, stefan-webrtc (holmer) wrote: > Please change this as I suggested before: > > if (!drop_height || increase_rate <= 0) > return rtc::Optional<int>(); > > return rtc::Optional<int>(std::min( > max_interval_ms_, > std::max(1000 * (*drop_hight) / increase_rate, > min_interval_ms_))); Done.
Hi, I trust on the way quantities are calculated. As a reader, I found it might help to rename some vars and APIs. Take my comments as suggestions. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:24: const AimdRateControl* const aimd_rate_control) second "const" can be omitted https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:32: const AimdRateControl* const aimd_rate_control) the second "const" can be omitted https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc:42: TEST(ProbingIntervalEstimatorTest, NoIntervalTillWeHaveDrop) { Till -> Untill and good to add a case that when GetIntervalMs() gives a value. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc:62: TEST(ProbingIntervalEstimatorTest, IntervalNotExceedMin) { Not->DoesNot https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc:72: TEST(ProbingIntervalEstimatorTest, IntervalNotExceedMax) { Not->DoesNot https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:136: double avg_packet_size_bits = bits_per_frame / packets_per_frame; possible division of zero? https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:142: (avg_packet_size_bits * 1000) / response_time); Add explicit cast https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:256: return (now_ms - last_ms) * GetNearMaxIncreaseRateBps() / 1000.0; why double on 1000.0 or do we want to return a double (or float)? https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:45: virtual int GetNearMaxIncreaseRateBps() const; why virtual? Does "Near" mean "Latest"? I feel that we can drop "Near" https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: virtual rtc::Optional<int> GetLastDecrease() const; why virtual? and the name may be made clearer: how about GetLastBitrateDecreaseBps? https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:19: // constexpr int kTimeConstantMs = 1000; can we remove these commented lines https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:35: void InitToBitrate(const AimdRateControlStates& states, InitBitrate? https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:63: TEST(AimdRateControlTest, NearMaxIncreaseRateIs5kbpsOn90kbpsAn200msRtt) { An->And https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:70: TEST(AimdRateControlTest, NearMaxIncreaseRateIs5kbpsOn60kbpsAn100msRtt) { An->And https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:78: TEST(AimdRateControlTest, UnknownDropHightBeforeFirstOveruse) { Hight -> Height or maybe even rename to "UnknownBitrateDecrease..."
https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:24: const AimdRateControl* const aimd_rate_control) On 2016/11/14 16:50:42, minyue-webrtc wrote: > second "const" can be omitted Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:32: const AimdRateControl* const aimd_rate_control) On 2016/11/14 16:50:42, minyue-webrtc wrote: > the second "const" can be omitted Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc:42: TEST(ProbingIntervalEstimatorTest, NoIntervalTillWeHaveDrop) { On 2016/11/14 16:50:42, minyue-webrtc wrote: > Till -> Untill > > and good to add a case that when GetIntervalMs() gives a value. Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc:62: TEST(ProbingIntervalEstimatorTest, IntervalNotExceedMin) { On 2016/11/14 16:50:42, minyue-webrtc wrote: > Not->DoesNot Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator_unittest.cc:72: TEST(ProbingIntervalEstimatorTest, IntervalNotExceedMax) { On 2016/11/14 16:50:42, minyue-webrtc wrote: > Not->DoesNot Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:136: double avg_packet_size_bits = bits_per_frame / packets_per_frame; On 2016/11/14 16:50:42, minyue-webrtc wrote: > possible division of zero? Theoretical yes. Added a DCHECK of current_bitrate_bps_ https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:142: (avg_packet_size_bits * 1000) / response_time); On 2016/11/14 16:50:42, minyue-webrtc wrote: > Add explicit cast Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:256: return (now_ms - last_ms) * GetNearMaxIncreaseRateBps() / 1000.0; Removed ".0" from 1000 https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:45: virtual int GetNearMaxIncreaseRateBps() const; The function is virtual so that I can mock them. Added a comment to calcify the function name. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: virtual rtc::Optional<int> GetLastDecrease() const; The function is virtual so that I can mock them. I changed the name to "GetLastBitrateDecreaseBps" https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:19: // constexpr int kTimeConstantMs = 1000; sure :) https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:35: void InitToBitrate(const AimdRateControlStates& states, On 2016/11/14 16:50:42, minyue-webrtc wrote: > InitBitrate? Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:63: TEST(AimdRateControlTest, NearMaxIncreaseRateIs5kbpsOn90kbpsAn200msRtt) { On 2016/11/14 16:50:42, minyue-webrtc wrote: > An->And Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:70: TEST(AimdRateControlTest, NearMaxIncreaseRateIs5kbpsOn60kbpsAn100msRtt) { On 2016/11/14 16:50:42, minyue-webrtc wrote: > An->And Done. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:78: TEST(AimdRateControlTest, UnknownDropHightBeforeFirstOveruse) { On 2016/11/14 16:50:42, minyue-webrtc wrote: > Hight -> Height or maybe even rename to "UnknownBitrateDecrease..." Done.
LG % nit https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right): https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:45: virtual int GetNearMaxIncreaseRateBps() const; On 2016/11/15 09:41:59, michaelt wrote: > The function is virtual so that I can mock them. > Added a comment to calcify the function name. > ah, right. https://codereview.webrtc.org/2380883003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: virtual rtc::Optional<int> GetLastDecrease() const; On 2016/11/15 09:41:59, michaelt wrote: > The function is virtual so that I can mock them. > I changed the name to "GetLastBitrateDecreaseBps" > Acknowledged. https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:38: rtc::Optional<int> drop_height = even better to rename this var to bitrate_drop (or decrease) https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:257: return (now_ms - last_ms) * GetNearMaxIncreaseRateBps() / 1000; there is some cast too. and should we return uint32_t?
https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_interval_estimator.cc (right): https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_interval_estimator.cc:38: rtc::Optional<int> drop_height = On 2016/11/15 12:20:36, minyue-webrtc wrote: > even better to rename this var to bitrate_drop (or decrease) Done. https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:257: return (now_ms - last_ms) * GetNearMaxIncreaseRateBps() / 1000; Did a explicit cast to unit32_t => we may could switch to int but for this CL I like to keep the old interface. Since if we change it we have to change it in the entire class.
lgtm https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2380883003/diff/160001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:257: return (now_ms - last_ms) * GetNearMaxIncreaseRateBps() / 1000; On 2016/11/15 13:08:15, michaelt wrote: > Did a explicit cast to unit32_t => we may could switch to int but for this CL I > like to keep the old interface. Since if we change it we have to change it in > the entire class. > Acknowledged.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2380883003/#ps180001 (title: "Respond to comments.")
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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16127)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2380883003/#ps200001 (title: "Fix gyp files.")
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: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by minyue@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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16187)
The CQ bit was checked by michaelt@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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2380883003/#ps220001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add interval estimator to remote bitrate estimator. To be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 ========== to ========== Add interval estimator to remote bitrate estimator. To be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add interval estimator to remote bitrate estimator. To be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 ========== to ========== Add interval estimator to remote bitrate estimator. To be able to smooth the bandwidth estimation according to the probing interval. BUG=webrtc:6443 Committed: https://crrev.com/4a4b3cfc01d265f8708f3010214c04c51cbcaff7 Cr-Commit-Position: refs/heads/master@{#15123} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/4a4b3cfc01d265f8708f3010214c04c51cbcaff7 Cr-Commit-Position: refs/heads/master@{#15123} |
