Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954923003:
Reduce send rate to 50% if overusing before we have an acknowledged bitrate. (Closed)
DescriptionReduce send rate to 50% if overusing before we have an acknowledged bitrate.
Check TimeToReducefurther to avoid reducing too often.
BUG=webrtc:7884
Review-Url: https://codereview.webrtc.org/2954923003
Cr-Commit-Position: refs/heads/master@{#18888}
Committed: https://chromium.googlesource.com/external/webrtc/+/a9521e248ef1c92bbbd992a010d5d130000f29f8
Patch Set 1 #
Total comments: 4
Patch Set 2 : Nits and add test. #
Total comments: 2
Patch Set 3 : Verify overuse detected in test. #
Messages
Total messages: 22 (16 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: + stefan@webrtc.org, tschumim@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/14902)
LGTM with comments addressed. Would be nice with a simple unittest to verify this too. https://codereview.webrtc.org/2954923003/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2954923003/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:209: // reducing too often End with . https://codereview.webrtc.org/2954923003/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:216: printf("Halving send rate because no acknowledged bitrate\n"); Remove
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/...
holmer@google.com changed reviewers: + holmer@google.com
lgtm but consider my comment. https://codereview.webrtc.org/2954923003/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): https://codereview.webrtc.org/2954923003/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:228: EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate / 2, 15000); Should we add an EXPECT_TRUE(seen_overuse)?
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/2954923003/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2954923003/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:209: // reducing too often On 2017/06/30 15:31:34, stefan-webrtc wrote: > End with . Done. https://codereview.webrtc.org/2954923003/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/delay_based_bwe.cc:216: printf("Halving send rate because no acknowledged bitrate\n"); On 2017/06/30 15:31:34, stefan-webrtc wrote: > Remove Done. Thanks. https://codereview.webrtc.org/2954923003/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right): https://codereview.webrtc.org/2954923003/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:228: EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate / 2, 15000); On 2017/07/04 10:54:30, holmer wrote: > Should we add an EXPECT_TRUE(seen_overuse)? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, holmer@google.com Link to the patchset: https://codereview.webrtc.org/2954923003/#ps40001 (title: "Verify overuse detected in test.")
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": 40001, "attempt_start_ts": 1499169007915400,
"parent_rev": "2c3161c86e4d96df42ef558dff932baf0b2feb04", "commit_rev":
"a9521e248ef1c92bbbd992a010d5d130000f29f8"}
Message was sent while issue was closed.
Description was changed from ========== Reduce send rate to 50% if overusing before we have an acknowledged bitrate. Check TimeToReducefurther to avoid reducing too often. BUG=webrtc:7884 ========== to ========== Reduce send rate to 50% if overusing before we have an acknowledged bitrate. Check TimeToReducefurther to avoid reducing too often. BUG=webrtc:7884 Review-Url: https://codereview.webrtc.org/2954923003 Cr-Commit-Position: refs/heads/master@{#18888} Committed: https://chromium.googlesource.com/external/webrtc/+/a9521e248ef1c92bbbd992a01... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/a9521e248ef1c92bbbd992a01... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
