Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537043003:
fix wrong implementation of AimdRateControl::TimeToReduceFurther. (Closed)
Descriptionfix wrong implementation of AimdRateControl::TimeToReduceFurther.
The comment of this method says that it returns true if "the incoming_bitrate is more than 5% above the current estimate".
BUG=
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix wrong implementation of AimdRateControl::TimeToReduceFurther #
Messages
Total messages: 21 (8 generated)
howtofly@gmail.com changed reviewers: + pbos@webrtc.org, stefan@webrtc.org
fix an obvious wrong implementation of AimdRateControl::TimeToReduceFurther
Description was changed from ========== fix wrong implementation of AimdRateControl::TimeToReduceFurther. The comment of this method says that it returns true if "the incoming_bitrate is more than 5% above the current estimate". BUG= ========== to ========== fix wrong implementation of AimdRateControl::TimeToReduceFurther. The comment of this method says that it returns true if "the incoming_bitrate is more than 5% above the current estimate". BUG= ==========
howtofly@gmail.com changed reviewers: + henrik.lundin@webrtc.org - pbos@webrtc.org
Thanks for this! https://codereview.webrtc.org/2537043003/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2537043003/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:77: return incoming_bitrate_bps > threshold; Agree that it's wrong! I think the comment is wrong too, and that this should say: return incoming_bitrate_bps < 0.95 * threshold; The reasoning is that, if the incoming bitrate is even lower than our estimate, it's probably a good idea to further decrease the estimate. Could I request that you add a unittest in aimd_rate_control_unittests.cc and update the comment in the .h file?
add review comments. https://codereview.webrtc.org/2537043003/diff/1/webrtc/modules/remote_bitrate... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2537043003/diff/1/webrtc/modules/remote_bitrate... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:77: return incoming_bitrate_bps > threshold; On 2016/11/29 15:52:08, stefan-webrtc (holmer) wrote: > Agree that it's wrong! I think the comment is wrong too, and that this should > say: > return incoming_bitrate_bps < 0.95 * threshold; > > The reasoning is that, if the incoming bitrate is even lower than our estimate, > it's probably a good idea to further decrease the estimate. After reconsideration, I agree that if we are overusing and the actual bitrate is very close to (or lower than) our lastest estimate, then it may be a good opportunity to further reduce the bitrate estimate. "Hysteresis" implies it was originally intended to detect lags, since actual changes always happen behind estimate changes.If this is the case, then I guess it should be return incoming_bitrate_bps < 1.05 * LatestEstimate; It gives AimdRateControl early chance to reduce bitrate, which make bitrate reduction more aggressive. On the other hand, "return incoming_bitrate_bps < 0.95 * LatestEstimate" seems more conservative, and the 5% losts the original meaning. Anyway, my patch is wrong, though it complies with the comment. > Could I request that you add a unittest in aimd_rate_control_unittests.cc and > update the comment in the .h file? I will follow your suggested modification and re-commit patches.
Hi, Stefan. Here is the updated patch-set. Bitrate reduction becomes less aggressive as expected, which lead to 14 failed unit tests. All of them are related to sudden drop of network capacity, such as: ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: Failure The difference between expected_bitrate_drop_delta and bitrate_drop_time - overuse_start_time is 66, which exceeds 33, where expected_bitrate_drop_delta evaluates to 567, bitrate_drop_time - overuse_start_time evaluates to 633, and 33 evaluates to 33. [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms)
The CQ bit was checked by howtofly@gmail.com
The CQ bit was unchecked by howtofly@gmail.com
On 2016/11/30 12:20:25, zhengpeng wrote: > Hi, Stefan. > > Here is the updated patch-set. > Bitrate reduction becomes less aggressive as expected, which lead to 14 failed > unit tests. All of them are related to sudden drop of network capacity, such as: > > ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: > Failure > The difference between expected_bitrate_drop_delta and bitrate_drop_time - > overuse_start_time is 66, which exceeds 33, where > expected_bitrate_drop_delta evaluates to 567, > bitrate_drop_time - overuse_start_time evaluates to 633, and > 33 evaluates to 33. > [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms) Ok, then those tests must be updated too
On 2016/11/30 13:00:33, stefan-webrtc (holmer) wrote: > On 2016/11/30 12:20:25, zhengpeng wrote: > > Hi, Stefan. > > > > Here is the updated patch-set. > > Bitrate reduction becomes less aggressive as expected, which lead to 14 failed > > unit tests. All of them are related to sudden drop of network capacity, such > as: > > > > > ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: > > Failure > > The difference between expected_bitrate_drop_delta and bitrate_drop_time - > > overuse_start_time is 66, which exceeds 33, where > > expected_bitrate_drop_delta evaluates to 567, > > bitrate_drop_time - overuse_start_time evaluates to 633, and > > 33 evaluates to 33. > > [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms) > > Ok, then those tests must be updated too Unit test updated. Please kindly note that without modifying remote_bitrate_estimator_unittest_helper.cc there will still be one test failed: ../../webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc:512: Failure Value of: latest_bps Actual: 426333 Expected: bitrate_bps Which is: 884920 ../../webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc:512: Failure Value of: latest_bps Actual: 426333 Expected: bitrate_bps Which is: 884920 [ FAILED ] RemoteBitrateEstimatorAbsSendTimeTest.CapacityDropTwoStreamsWrap (22 ms) It is not big problem, we only to call bitrate_observer_->latest_bitrate() once more to fix it.
Code comment added. https://codereview.webrtc.org/2537043003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc (right): https://codereview.webrtc.org/2537043003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc:509: bitrate_bps = bitrate_observer_->latest_bitrate(); when number_of_streams = 2, we need this additional call. I have not figured out why yet.
stefan@webrtc.org changed reviewers: + terelius@webrtc.org - henrik.lundin@webrtc.org
terelius, could you review this also. Especially help figure out why the tests with the new delay estimator goes bad. https://codereview.webrtc.org/2537043003/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): https://codereview.webrtc.org/2537043003/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:217: CapacityDropTestHelper(1, false, 1934, 0); This looks like a pretty big increase. Adding terelius@ who should be able to determine if this change causes troubles here.
On 2016/11/30 12:20:25, zhengpeng wrote: > Hi, Stefan. > > Here is the updated patch-set. > Bitrate reduction becomes less aggressive as expected, which lead to 14 failed > unit tests. All of them are related to sudden drop of network capacity, such as: > > ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: > Failure > The difference between expected_bitrate_drop_delta and bitrate_drop_time - > overuse_start_time is 66, which exceeds 33, where > expected_bitrate_drop_delta evaluates to 567, > bitrate_drop_time - overuse_start_time evaluates to 633, and > 33 evaluates to 33. > [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms) As far as I can tell, reductions should be more aggressive now, not less. The old code would reduce if estimate - incoming_bitrate > 1.05 * incoming_bitrate or equivalently, if incoming_bitrate < 0.4878 * estimate The new code will reduce if incoming_bitrate < 0.95 * estimate
On 2016/11/30 14:58:50, stefan-webrtc (holmer) wrote: > terelius, could you review this also. Especially help figure out why the tests > with the new delay estimator goes bad. > > https://codereview.webrtc.org/2537043003/diff/40001/webrtc/modules/congestion... > File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): > > https://codereview.webrtc.org/2537043003/diff/40001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:217: > CapacityDropTestHelper(1, false, 1934, 0); > This looks like a pretty big increase. Adding terelius@ who should be able to > determine if this change causes troubles here. What happens in DelayBasedBweTrendlineExperimentTest.CapacityDropOneStream is that we back down slightly faster, but to ~503 kbps whereas the old aimd rate control would back down to ~487 kbps. Once we are at 503 kbps, the delay no longer increases so we start ramping up again until we get a second overuse signal at 517 kbps and back off to 425 kbps. Note that we never empty the network queues until we get the second overuse signal. We need to discuss how to solve this issue. Possible solution include: Decreasing the 0.95 factor in AIMD rate controller Increasing the gain on the overuse signal when comparing to the threshold Backing off to less than 85% of the received bitrate It is also possible that this will be affected by your ImprovedBitrateExperiment.
On 2016/11/30 16:38:50, terelius wrote: > On 2016/11/30 12:20:25, zhengpeng wrote: > > Hi, Stefan. > > > > Here is the updated patch-set. > > Bitrate reduction becomes less aggressive as expected, which lead to 14 failed > > unit tests. All of them are related to sudden drop of network capacity, such > as: > > > > > ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: > > Failure > > The difference between expected_bitrate_drop_delta and bitrate_drop_time - > > overuse_start_time is 66, which exceeds 33, where > > expected_bitrate_drop_delta evaluates to 567, > > bitrate_drop_time - overuse_start_time evaluates to 633, and > > 33 evaluates to 33. > > [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms) > > As far as I can tell, reductions should be more aggressive now, not less. > The old code would reduce if > estimate - incoming_bitrate > 1.05 * incoming_bitrate > or equivalently, if > incoming_bitrate < 0.4878 * estimate > The new code will reduce if > incoming_bitrate < 0.95 * estimate Please correct me if wrong: when overusing, do early reduction only when the incoming bitrate is sufficiently low, which is defined by kWithinIncomingBitrateHysteresis, otherwise do 'regular' reductions every rtt. How is kWithinIncomingBitrateHysteresis determined? By experience? If so, only the comment needs to be updated.
On 2016/12/01 03:44:07, zhengpeng wrote: > On 2016/11/30 16:38:50, terelius wrote: > > On 2016/11/30 12:20:25, zhengpeng wrote: > > > Hi, Stefan. > > > > > > Here is the updated patch-set. > > > Bitrate reduction becomes less aggressive as expected, which lead to 14 > failed > > > unit tests. All of them are related to sudden drop of network capacity, such > > as: > > > > > > > > > ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: > > > Failure > > > The difference between expected_bitrate_drop_delta and bitrate_drop_time - > > > overuse_start_time is 66, which exceeds 33, where > > > expected_bitrate_drop_delta evaluates to 567, > > > bitrate_drop_time - overuse_start_time evaluates to 633, and > > > 33 evaluates to 33. > > > [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms) > > > > As far as I can tell, reductions should be more aggressive now, not less. > > The old code would reduce if > > estimate - incoming_bitrate > 1.05 * incoming_bitrate > > or equivalently, if > > incoming_bitrate < 0.4878 * estimate > > The new code will reduce if > > incoming_bitrate < 0.95 * estimate > > Please correct me if wrong: > when overusing, do early reduction only when the incoming bitrate is > sufficiently low, > which is defined by kWithinIncomingBitrateHysteresis, otherwise do 'regular' > reductions every rtt. > > How is kWithinIncomingBitrateHysteresis determined? By experience? > If so, only the comment needs to be updated. I think your code looks good, and I don't think kWithinIncomingBitrateHysteresis was used as intended. However, the problem with making earlier reductions is that the measured bitrate lags behind the capacity drop, so if you back off earlier then you won't back off as much. This impacts the choices of several other parameters, and the current tuning seems to depend on the old, "incorrect", behavior of the aimd rate controller. If you want to land your CL quickly, then the easiest fix might be to set the threshold to 0.5 * LatestEstimate() rather than 0.95 * LatestEstimate(). That way there is (almost) no functional change and you don't have to update any unit tests. Stefan, do you have input?
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
On 2016/12/01 09:31:03, terelius wrote: > On 2016/12/01 03:44:07, zhengpeng wrote: > > On 2016/11/30 16:38:50, terelius wrote: > > > On 2016/11/30 12:20:25, zhengpeng wrote: > > > > Hi, Stefan. > > > > > > > > Here is the updated patch-set. > > > > Bitrate reduction becomes less aggressive as expected, which lead to 14 > > failed > > > > unit tests. All of them are related to sudden drop of network capacity, > such > > > as: > > > > > > > > > > > > > > ../../webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:432: > > > > Failure > > > > The difference between expected_bitrate_drop_delta and bitrate_drop_time - > > > > overuse_start_time is 66, which exceeds 33, where > > > > expected_bitrate_drop_delta evaluates to 567, > > > > bitrate_drop_time - overuse_start_time evaluates to 633, and > > > > 33 evaluates to 33. > > > > [ FAILED ] DelayBasedBweTest.CapacityDropOneStream (11 ms) > > > > > > As far as I can tell, reductions should be more aggressive now, not less. > > > The old code would reduce if > > > estimate - incoming_bitrate > 1.05 * incoming_bitrate > > > or equivalently, if > > > incoming_bitrate < 0.4878 * estimate > > > The new code will reduce if > > > incoming_bitrate < 0.95 * estimate > > > > Please correct me if wrong: > > when overusing, do early reduction only when the incoming bitrate is > > sufficiently low, > > which is defined by kWithinIncomingBitrateHysteresis, otherwise do 'regular' > > reductions every rtt. > > > > How is kWithinIncomingBitrateHysteresis determined? By experience? > > If so, only the comment needs to be updated. > > I think your code looks good, and I don't think kWithinIncomingBitrateHysteresis > was used as intended. However, the problem with making earlier reductions > is that the measured bitrate lags behind the capacity drop, so if you back > off earlier then you won't back off as much. This impacts the choices of > several other parameters, and the current tuning seems to depend on the > old, "incorrect", behavior of the aimd rate controller. > > If you want to land your CL quickly, then the easiest fix might be to set > the threshold to 0.5 * LatestEstimate() rather than 0.95 * LatestEstimate(). > That way there is (almost) no functional change and you don't have to update > any unit tests. Stefan, do you have input? I accidentally started a new issue: https://codereview.webrtc.org/2549453002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
