Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2789233005:
Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl. (Closed)
DescriptionMove BWE period calculation from ProbingIntervalEstimator to AimdRateControl.
Remove the ProbingIntervalEstimator and MockAimdRateControl.
BUG=webrtc:7441
Review-Url: https://codereview.webrtc.org/2789233005
Cr-Commit-Position: refs/heads/master@{#17769}
Committed: https://chromium.googlesource.com/external/webrtc/+/6737045af14f3df3537be4c9400936c226601119
Patch Set 1 #Patch Set 2 : Remove unused include. #
Total comments: 24
Patch Set 3 : Comments #Patch Set 4 : Nit #
Total comments: 19
Patch Set 5 : Comments #
Total comments: 1
Patch Set 6 : Nits #Messages
Total messages: 42 (27 generated)
The CQ bit was checked by terelius@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/...
terelius@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19210) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24511) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/20461) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24533)
The CQ bit was checked by terelius@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.
Description was changed from ========== Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl. Remove the ProbingIntervalEstimator and MockAimdRateControl. BUG=webrtc:7441 ========== to ========== Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl. Remove the ProbingIntervalEstimator and MockAimdRateControl. BUG=webrtc:7441 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
On 2017/04/04 16:15:01, terelius wrote: We must add Michael to take a look.
https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:96: void SendSideCongestionController::RegisterNetworkObserver(Observer* observer) { This change seams unrelated with the topic of the CL. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (left): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:185: bitrate_is_initialized_ = true; This change seams unrelated with the topic of the CL. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:129: constexpr double kMinIncreaseRateBps = 4000; What was the reason for this change ? https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (left): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: // Returns the increase rate which is used when used bandwidth is near the Was the comment to obvious ? https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: int GetNearMaxIncreaseRateBps() const; This function could be made private, but I'm not sure about it since we would not be able to test it then. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:130: TEST(AimdRateControlTest, DefaultIntervalUntilFirstOveruse) { Seams to be the same test as in DefaultPeriodBeforeFirstOveruse. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:142: TEST(AimdRateControlTest, CalcInterval) { Rename the test according to the new Function name. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:155: TEST(AimdRateControlTest, IntervalNotBelowMin) { Same here. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:168: TEST(AimdRateControlTest, IntervalNotAboveMax) { Same here.
This change looks good, but please address Michael's comments. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: int GetNearMaxIncreaseRateBps() const; On 2017/04/05 07:06:02, michaelt wrote: > This function could be made private, but I'm not sure about it since we would > not be able to test it then. I prefer making it private. There should be other ways of testing what we want to test.
The CQ bit was checked by terelius@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/...
https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:96: void SendSideCongestionController::RegisterNetworkObserver(Observer* observer) { On 2017/04/05 07:06:02, michaelt wrote: > This change seams unrelated with the topic of the CL. Huh? There is no change in SendSideCongestionController::RegisterNetworkObserver. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (left): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:185: bitrate_is_initialized_ = true; On 2017/04/05 07:06:02, michaelt wrote: > This change seams unrelated with the topic of the CL. Since the unit tests now use the AimdRateControl logic rather than mocking it, I had to do something to make the tests pass. Before this change you'd set last_decrease even if current_bitrate_bps_ had not been initialized. The alternative would be to always call SetStartBitrate() in the unit tests, but that seems less robust. Doing it this way means that the code will produce a sensible BWE period even if the starting bitrate has not been set. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:129: constexpr double kMinIncreaseRateBps = 4000; On 2017/04/05 07:06:02, michaelt wrote: > What was the reason for this change ? Originally wanted to make avg_packet_size into a constant, and in that case it makes sense to keep all parameters together. Do you want me to move it back? https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (left): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: // Returns the increase rate which is used when used bandwidth is near the On 2017/04/05 07:06:02, michaelt wrote: > Was the comment to obvious ? Initially I tried to remove the entire function and I forgot to reinsert the comment when I changed my mind. Fixed. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h:46: int GetNearMaxIncreaseRateBps() const; On 2017/04/05 07:20:42, stefan-webrtc wrote: > On 2017/04/05 07:06:02, michaelt wrote: > > This function could be made private, but I'm not sure about it since we would > > not be able to test it then. > > I prefer making it private. There should be other ways of testing what we want > to test. In principle I agree, but as I don't see a clean way to test the functionality, I'd prefer to keep it for now. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:130: TEST(AimdRateControlTest, DefaultIntervalUntilFirstOveruse) { On 2017/04/05 07:06:02, michaelt wrote: > Seams to be the same test as in DefaultPeriodBeforeFirstOveruse. Done. Removed DefaultPeriodBeforeFirstOveruse. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:142: TEST(AimdRateControlTest, CalcInterval) { On 2017/04/05 07:06:02, michaelt wrote: > Rename the test according to the new Function name. Done. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:155: TEST(AimdRateControlTest, IntervalNotBelowMin) { On 2017/04/05 07:06:02, michaelt wrote: > Same here. Done. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:168: TEST(AimdRateControlTest, IntervalNotAboveMax) { On 2017/04/05 07:06:02, michaelt wrote: > Same here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:96: void SendSideCongestionController::RegisterNetworkObserver(Observer* observer) { Hmm this is probably shown to me because you rebased ? https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (left): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:185: bitrate_is_initialized_ = true; On 2017/04/05 10:58:03, terelius wrote: > On 2017/04/05 07:06:02, michaelt wrote: > > This change seams unrelated with the topic of the CL. > > Since the unit tests now use the AimdRateControl logic rather than mocking it, I > had to do something to make the tests pass. Before this change you'd set > last_decrease even if current_bitrate_bps_ had not been initialized. The > alternative would be to always call SetStartBitrate() in the unit tests, but > that seems less robust. Doing it this way means that the code will produce a > sensible BWE period even if the starting bitrate has not been set. Acknowledged. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:129: constexpr double kMinIncreaseRateBps = 4000; Would be nice since then the const is defined where its used.
The CQ bit was checked by terelius@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/...
https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:96: void SendSideCongestionController::RegisterNetworkObserver(Observer* observer) { On 2017/04/06 11:35:13, michaelt wrote: > Hmm this is probably shown to me because you rebased ? > Does this really show up as modified in codereview.webrtc.org for you? While a rebase could cause things to show up as modified, I think RegisterNetworkObserver was landed before I created the branch. In any case, the codereview should not be account dependent and neither I nor minyue see this as modified. https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2789233005/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:129: constexpr double kMinIncreaseRateBps = 4000; On 2017/04/06 11:35:13, michaelt wrote: > Would be nice since then the const is defined where its used. > Done. However, I don't think there is any advantage in defining constants where they are used. I prefer collecting all behavior-controlling parameters before the code that they affect, i.e. at the beginning of the function or the file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:73: TEST(AimdRateControlTest, GetLastBitrateDecrease) { rename test https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:80: EXPECT_NEAR(14000, states.aimd_rate_control->GetNearMaxIncreaseRateBps(), can we be stricter on this? https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:82: ASSERT_TRUE(states.aimd_rate_control->GetExpectedBandwidthPeriodMs()); what does 82 do? https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:137: TEST(AimdRateControlTest, ExpectedPeriodAfter20kbpsDrop5kbpsIncrease) { Preferrably not hard code numerics in the name https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:143: // Make the bitrate drop to 20 kbps fo get to 90 kbps. fo -> to https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:145: UpdateRateControl(states, kBwOverusing, (kInitialBitrate - 20000) / 0.85, what is 0.85 https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:151: TEST(AimdRateControlTest, BandwidthPeriodNotBelowMin) { Not -> CanNot https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:164: TEST(AimdRateControlTest, BandwidthPeriodNotAboveMax) { Not -> CanNot
The CQ bit was checked by terelius@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/...
https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:73: TEST(AimdRateControlTest, GetLastBitrateDecrease) { On 2017/04/11 10:50:02, minyue-webrtc wrote: > rename test Done. https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:80: EXPECT_NEAR(14000, states.aimd_rate_control->GetNearMaxIncreaseRateBps(), On 2017/04/11 10:50:02, minyue-webrtc wrote: > can we be stricter on this? Yes, we could enforce strict equality but that would just make the test fail whenever the code changes. What we really want to test is that GetNearMaxIncreaseRateBps gives reasonable results, e.g. about 14 seconds at 300 kbps. Afaik, we don't care if the period is 14 or 14.51 seconds. WDYT? https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:82: ASSERT_TRUE(states.aimd_rate_control->GetExpectedBandwidthPeriodMs()); On 2017/04/11 10:50:02, minyue-webrtc wrote: > what does 82 do? Left-over from previous version. Removed. https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:137: TEST(AimdRateControlTest, ExpectedPeriodAfter20kbpsDrop5kbpsIncrease) { On 2017/04/11 10:50:02, minyue-webrtc wrote: > Preferrably not hard code numerics in the name I'm following the convention used in the other test cases, e.g. NearMaxIncreaseRateIs5kbpsOn60kbpsAnd100msRtt https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:143: // Make the bitrate drop to 20 kbps fo get to 90 kbps. On 2017/04/11 10:50:02, minyue-webrtc wrote: > fo -> to Done. https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:145: UpdateRateControl(states, kBwOverusing, (kInitialBitrate - 20000) / 0.85, On 2017/04/11 10:50:02, minyue-webrtc wrote: > what is 0.85 We back off to 85% of the measured bitrate after an overuse. If we want to drop to 90 kbps then we need to "measure" 90/0.85 kbps. https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:151: TEST(AimdRateControlTest, BandwidthPeriodNotBelowMin) { On 2017/04/11 10:50:02, minyue-webrtc wrote: > Not -> CanNot Done. (IsNot) https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:164: TEST(AimdRateControlTest, BandwidthPeriodNotAboveMax) { On 2017/04/11 10:50:02, minyue-webrtc wrote: > Not -> CanNot Done. (IsNot)
lgtm % nit https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:80: EXPECT_NEAR(14000, states.aimd_rate_control->GetNearMaxIncreaseRateBps(), On 2017/04/19 07:36:01, terelius wrote: > On 2017/04/11 10:50:02, minyue-webrtc wrote: > > can we be stricter on this? > > Yes, we could enforce strict equality but that would just make the test fail > whenever the code changes. What we really want to test is that > GetNearMaxIncreaseRateBps gives reasonable results, e.g. about 14 seconds at 300 > kbps. Afaik, we don't care if the period is 14 or 14.51 seconds. WDYT? Sure, if you think 1s a good margin. https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:145: UpdateRateControl(states, kBwOverusing, (kInitialBitrate - 20000) / 0.85, On 2017/04/19 07:36:01, terelius wrote: > On 2017/04/11 10:50:02, minyue-webrtc wrote: > > what is 0.85 > > We back off to 85% of the measured bitrate after an overuse. If we want to drop > to 90 kbps then we need to "measure" 90/0.85 kbps. can we give a name to it and constexpr? https://codereview.webrtc.org/2789233005/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:171: UpdateRateControl(states, BandwidthUsage::kBwOverusing, 10000 / 0.85, a name to 0,85, as commented at another place.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc (right): https://codereview.webrtc.org/2789233005/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc:145: UpdateRateControl(states, kBwOverusing, (kInitialBitrate - 20000) / 0.85, On 2017/04/19 07:49:05, minyue-webrtc wrote: > On 2017/04/19 07:36:01, terelius wrote: > > On 2017/04/11 10:50:02, minyue-webrtc wrote: > > > what is 0.85 > > > > We back off to 85% of the measured bitrate after an overuse. If we want to > drop > > to 90 kbps then we need to "measure" 90/0.85 kbps. > > can we give a name to it and constexpr? Done.
lgtm
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2789233005/#ps100001 (title: "Nits")
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": 100001, "attempt_start_ts": 1492615168403250,
"parent_rev": "e52a203a56c7876bc226c04464563555433fa0c1", "commit_rev":
"6737045af14f3df3537be4c9400936c226601119"}
Message was sent while issue was closed.
Description was changed from ========== Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl. Remove the ProbingIntervalEstimator and MockAimdRateControl. BUG=webrtc:7441 ========== to ========== Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl. Remove the ProbingIntervalEstimator and MockAimdRateControl. BUG=webrtc:7441 Review-Url: https://codereview.webrtc.org/2789233005 Cr-Commit-Position: refs/heads/master@{#17769} Committed: https://chromium.googlesource.com/external/webrtc/+/6737045af14f3df3537be4c94... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/6737045af14f3df3537be4c94... |
