Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2965233002:
Let alr dectector use a budged to detect underuse. (Closed)
|
Created:
3 years, 5 months ago by tschumi Modified:
3 years, 5 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhuangzesen_agora.io, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLet alr detector use a budged to detect underuse.
BUG=webrtc:7947
Review-Url: https://codereview.webrtc.org/2965233002
Cr-Commit-Position: refs/heads/master@{#18972}
Committed: https://chromium.googlesource.com/external/webrtc/+/82c5593cb6a04cef829a0924fb332f56976a5f69
Patch Set 1 #
Total comments: 11
Patch Set 2 : Response to comments #
Total comments: 28
Patch Set 3 : Respond to comments. #Patch Set 4 : Added TODO's #Patch Set 5 : Rebased. #
Messages
Total messages: 35 (20 generated)
The CQ bit was checked by tschumim@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18967)
Description was changed from ========== Let alr dectector use a budged to detect underuse. BUG= ========== to ========== Let alr detector use a budged to detect underuse. BUG=webrtc:7947 ==========
tschumim@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector.cc (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector.cc:85: if (sscanf(group_name.c_str(), "%f,%" PRId64 ",%d,%d,%d", If we change this we may break an existing experiment, right? Talk to Philip about what we need to do to get that updated in the right places. https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector.h (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector.h:60: static constexpr int kDefaultAlrStartBudgetLevePercent = 20; Level? https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector_unittest.cc (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector_unittest.cc:60: // Verify that ALR ends when usage is above 60%. Not clear how for instance 60% relates to 1000, 100 below. Can we express those in terms of 60% instead, or in some way make it easier to read? https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector_unittest.cc:77: // ALR ends when usage is above 100%. Do we want to change from 70% to 100%? https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector_unittest.cc:86: // ALR starts when bitrate drops below 20%. Is this comment still correct? Why did it change from 500 to 1000?
https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector.cc (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector.cc:85: if (sscanf(group_name.c_str(), "%f,%" PRId64 ",%d,%d,%d", I already have, and he told me the experiment is not active at the moment and it should be save to change it. https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector.h (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector.h:60: static constexpr int kDefaultAlrStartBudgetLevePercent = 20; Yes :) https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector_unittest.cc (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector_unittest.cc:60: // Verify that ALR ends when usage is above 60%. Changed the impl a little so that the test code should be better understandable. https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector_unittest.cc:77: // ALR ends when usage is above 100%. Yea my mistake => changed it to 65 % (new default for alr) https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector_unittest.cc:86: // ALR starts when bitrate drops below 20%. Because of the new impl. it takes more time to detect alr. The comment is still valid.
holmer@google.com changed reviewers: + holmer@google.com
https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... File webrtc/modules/pacing/alr_detector.cc (right): https://codereview.webrtc.org/2965233002/diff/1/webrtc/modules/pacing/alr_det... webrtc/modules/pacing/alr_detector.cc:85: if (sscanf(group_name.c_str(), "%f,%" PRId64 ",%d,%d,%d", On 2017/07/07 08:47:30, tschumi wrote: > I already have, and he told me the experiment is not active at the moment and it > should be save to change it. Good! https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector.h (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:61: static constexpr int kDefaultAlrStopBudgetLevelPercent = -20; -20% budget sounds to me like we're sending 20% too much? Isn't that way too late to exit ALR? https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:68: int bandwidth_usage_percent_; It's not entirely clear to me what bandwidth_usage_percent_ represents. I think the old thresholds were easier to understand. Why do we have three thresholds now, and not just a start and an end threshold?
https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector.h (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:61: static constexpr int kDefaultAlrStopBudgetLevelPercent = -20; How does the budget level relate to BW usage. Is there any hysteresis or will any bandwidth usage < 65% eventually cause us to enter ALR, and any bandwidth > 65% exit it?
https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector.h (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:61: static constexpr int kDefaultAlrStopBudgetLevelPercent = -20; The budged can reach from -500ms overuse to +500ms underused. 20% budget means 0.2 * 500ms = 100ms underused -20% budget means -0.2 * 500ms = -100ms overused I totally agree that the previous impl was way easier to understand. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:61: static constexpr int kDefaultAlrStopBudgetLevelPercent = -20; Is there any hysteresis => yes. will any bandwidth usage < 65% eventually cause us to enter ALR, and any bandwidth > 65% exit it => yes. :) The hysteresis is done with the budged level. (20%,-20%) So the hysteresis is dependent on time and bandwidth diff. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:68: int bandwidth_usage_percent_; It represents: alr_budget = bandwidth_usage_percent_/ 100 * estimated_bitrate. when we overuse the alr_budget we are not in alr. when we underuse the alr_budget we are in alr.
https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector.h (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.h:61: static constexpr int kDefaultAlrStopBudgetLevelPercent = -20; On 2017/07/07 09:36:15, tschumi wrote: > Is there any hysteresis => yes. > will any bandwidth usage < 65% eventually cause us to enter ALR, and any > bandwidth > 65% exit it => yes. > > :) > > The hysteresis is done with the budged level. (20%,-20%) > So the hysteresis is dependent on time and bandwidth diff. > So that means that there is no hysteresis based on bandwidth estimate, only how long we are sending at 64% or 66%. This might be desired, but we could modify the bandwidth_usage_percent_whenever we enter/exit ALR if we wanted to keep the old 60-70% hysteresis.
https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.cc:46: alr_budget_.IncreaseBudget(delta_time_ms); Not sure if it matters, but wont calling UseBudget and then IncreaseBudget cause a bias to exit ALR somewhat quicker than entering it? https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.cc:50: alr_started_time_ms_ = rtc::Optional<int64_t>(rtc::TimeMillis()); Nit: alr_start_time_ms_.emplace(rtc::TimeMillis()); https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector_unittest.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:31: SimulateOutgoingTrafficIn ForTimeMs(int time_ms) { Return type should be SimulateOutgoingTrafficIn& right? https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:32: interval_ms_ = rtc::Optional<int>(time_ms); nit: emplace https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:37: SimulateOutgoingTrafficIn AtPercentOfEstimatedBitrate(int usage_percentage) { SimulateOutgoingTrafficIn& https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:38: usage_percentage_ = rtc::Optional<int>(usage_percentage); nit: emplace https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... File webrtc/modules/pacing/interval_budget.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:36: void IntervalBudget::IncreaseBudget(int64_t delta_time_ms) { I think it might be good to merge IncreaseBudget and UseBudget into a UpdateBudget(size_t bytes_used, int64_t delta_time_ms) function. That way we can calculate remaining bytes without a bias. Maybe it doesn't matter that much, but I think it's good to avoid quirks so they don't come back and bite you later on. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:43: bytes_remaining_ = std::min(bytes, max_bytes_in_budget_); It looks like |bytes_remaining_| can be positive even when |can_build_up_underuse_| is true. If we stop sending for a short while then |bytes| can be pretty large. Shouldn't we always update |bytes_remaining_| like we do on line 40 and then just: if (!can_build_up_underuse_) bytes_remaining_ = std::min(bytes_remaining_, 1);
https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.cc:46: alr_budget_.IncreaseBudget(delta_time_ms); I could do it the other way around but then we would have a bias on entering ALR. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector.cc:50: alr_started_time_ms_ = rtc::Optional<int64_t>(rtc::TimeMillis()); On 2017/07/10 11:41:05, philipel wrote: > Nit: > alr_start_time_ms_.emplace(rtc::TimeMillis()); Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... File webrtc/modules/pacing/alr_detector_unittest.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:31: SimulateOutgoingTrafficIn ForTimeMs(int time_ms) { On 2017/07/10 11:41:05, philipel wrote: > Return type should be SimulateOutgoingTrafficIn& right? Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:32: interval_ms_ = rtc::Optional<int>(time_ms); On 2017/07/10 11:41:05, philipel wrote: > nit: emplace Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:37: SimulateOutgoingTrafficIn AtPercentOfEstimatedBitrate(int usage_percentage) { On 2017/07/10 11:41:05, philipel wrote: > SimulateOutgoingTrafficIn& Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/alr... webrtc/modules/pacing/alr_detector_unittest.cc:38: usage_percentage_ = rtc::Optional<int>(usage_percentage); On 2017/07/10 11:41:05, philipel wrote: > nit: emplace Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... File webrtc/modules/pacing/interval_budget.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:36: void IntervalBudget::IncreaseBudget(int64_t delta_time_ms) { Would be a bit of a refactoring in the pacer => maybe in another cl ? https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:43: bytes_remaining_ = std::min(bytes, max_bytes_in_budget_); delta_time_ms is clamped by the pacer to 5ms so bytes should not become very large. The actual logic kind of make sense to me. Me may could implement it in a nicer way. For example: Change Interval Budget so that we can define a over- and under-use window in ms. So we could use the a budget for the pacer like budget(5ms,-500ms) and the remove the clamp in the pacer.
lgtm % comments https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... File webrtc/modules/pacing/interval_budget.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:36: void IntervalBudget::IncreaseBudget(int64_t delta_time_ms) { On 2017/07/11 09:33:31, tschumi wrote: > Would be a bit of a refactoring in the pacer => maybe in another cl ? Yeah, a separate CL sounds good. Please add a TODO. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:43: bytes_remaining_ = std::min(bytes, max_bytes_in_budget_); On 2017/07/11 09:33:31, tschumi wrote: > delta_time_ms is clamped by the pacer to 5ms so bytes should not become very Sound like we rely on external behavior, don't think we should do that. > large. > The actual logic kind of make sense to me. Me may could implement it in a nicer > way. > For example: > Change Interval Budget so that we can define a over- and under-use window in ms. > So we could use the a budget for the pacer like budget(5ms,-500ms) and the > remove the clamp in the pacer. I think specifying min/max budget in terms of ms is a good idea, another TODO you could add :)
https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... File webrtc/modules/pacing/interval_budget.cc (right): https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:36: void IntervalBudget::IncreaseBudget(int64_t delta_time_ms) { On 2017/07/11 12:29:05, philipel wrote: > On 2017/07/11 09:33:31, tschumi wrote: > > Would be a bit of a refactoring in the pacer => maybe in another cl ? > > Yeah, a separate CL sounds good. Please add a TODO. Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:43: bytes_remaining_ = std::min(bytes, max_bytes_in_budget_); On 2017/07/11 12:29:05, philipel wrote: > On 2017/07/11 09:33:31, tschumi wrote: > > delta_time_ms is clamped by the pacer to 5ms so bytes should not become very > > Sound like we rely on external behavior, don't think we should do that. > > > large. > > The actual logic kind of make sense to me. Me may could implement it in a > nicer > > way. > > For example: > > Change Interval Budget so that we can define a over- and under-use window in > ms. > > So we could use the a budget for the pacer like budget(5ms,-500ms) and the > > remove the clamp in the pacer. > > I think specifying min/max budget in terms of ms is a good idea, another TODO > you could add :) > Done. https://codereview.webrtc.org/2965233002/diff/20001/webrtc/modules/pacing/int... webrtc/modules/pacing/interval_budget.cc:43: bytes_remaining_ = std::min(bytes, max_bytes_in_budget_); On 2017/07/11 12:29:05, philipel wrote: > On 2017/07/11 09:33:31, tschumi wrote: > > delta_time_ms is clamped by the pacer to 5ms so bytes should not become very > > Sound like we rely on external behavior, don't think we should do that. > > > large. > > The actual logic kind of make sense to me. Me may could implement it in a > nicer > > way. > > For example: > > Change Interval Budget so that we can define a over- and under-use window in > ms. > > So we could use the a budget for the pacer like budget(5ms,-500ms) and the > > remove the clamp in the pacer. > > I think specifying min/max budget in terms of ms is a good idea, another TODO > you could add :) > Done.
tschumim@webrtc.org changed reviewers: + sprang@webrtc.org
@Erik: Can you take a look at the full_stack_tests ?
lgtm
The CQ bit was checked by tschumim@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: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/26204) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/23045)
The CQ bit was checked by tschumim@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.
The CQ bit was checked by tschumim@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2965233002/#ps80001 (title: "Rebased.")
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": 80001, "attempt_start_ts": 1499781195783480,
"parent_rev": "87b6ddb5610df73400202481233a60d6fc601cae", "commit_rev":
"82c5593cb6a04cef829a0924fb332f56976a5f69"}
Message was sent while issue was closed.
Description was changed from ========== Let alr detector use a budged to detect underuse. BUG=webrtc:7947 ========== to ========== Let alr detector use a budged to detect underuse. BUG=webrtc:7947 Review-Url: https://codereview.webrtc.org/2965233002 Cr-Commit-Position: refs/heads/master@{#18972} Committed: https://chromium.googlesource.com/external/webrtc/+/82c5593cb6a04cef829a0924f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/82c5593cb6a04cef829a0924f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
