Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474913002:
Logging basic bad call detection (Closed)
DescriptionLogging basic bad call detection
BUG=webrtc:6814
Committed: https://crrev.com/349092befe47a10d9179837d0de93c0334fe3088
Cr-Commit-Position: refs/heads/master@{#15568}
Patch Set 1 #Patch Set 2 : Logging on state change #
Total comments: 9
Patch Set 3 : Rebase + double thresholds + variance calculation #Patch Set 4 : Actual judgement + no high/low until certain #Patch Set 5 : Rename + Tests #
Total comments: 6
Patch Set 6 : Comments #
Total comments: 11
Patch Set 7 : Comment + Rebase #
Total comments: 7
Patch Set 8 : Comments #
Messages
Total messages: 31 (13 generated)
palmkvist@webrtc.org changed reviewers: + sprang@webrtc.org
Basic bad call detection and logging
Some initial comments. Let's continue discussion from here when we both remember the context better :) https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... File webrtc/video/bad_call_threshold.cc (right): https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.cc:46: } What tradeoffs are there with doing state change detection outside this class? https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... File webrtc/video/bad_call_threshold.h (right): https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.h:30: }; Could you add a short comment explaining these? Also, I'd prefer an enum class. https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.h:35: const std::unique_ptr<int[]> buffer_; Unless we want to be able to dynamically change the threshold at runtime, I don't think we actually need to store the values. If you just have a ringbuffer of states and a counter for each state you can add and subtract values without looping.
https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... File webrtc/video/bad_call_threshold.cc (right): https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.cc:46: } On 2016/11/14 13:14:15, språng wrote: > What tradeoffs are there with doing state change detection outside this class? You're suggesting just calculating which state we're in here and letting a caller figure out if that state is different from the previous? It would essentially necessitate code like this: bool high = thresh.getState(); thresh.AddMeasurement(thing); if (high != thresh.getState()) ... Which is slightly more cumbersome than the current approach, but it would simplify the code in AddMeasurement. On the other hand the current scheme does not give a way to retrieve the state without adding a measurement, which is weird, and any such method would either only return STILL_LOW or STILL_HIGH, or need to do some extra caching. Overally probably worth it to move the change detection out. https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... File webrtc/video/bad_call_threshold.h (right): https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.h:18: class BadCallThreshold { I am by the way rather unhappy with this name, so any ideas are welcome. https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.h:35: const std::unique_ptr<int[]> buffer_; On 2016/11/14 13:14:15, språng wrote: > Unless we want to be able to dynamically change the threshold at runtime, I > don't think we actually need to store the values. If you just have a ringbuffer > of states and a counter for each state you can add and subtract values without > looping. We do need the values to calculate the variance though. As for removing the loop, when we need to calculate the variance as well we get the choice between linear time per measurement and accurate variance or an imprecise variance (that I'm not entirely sure how it relates) in constant time. (Also, is there a preexisting ringbuffer somewhere? A cursory search when I first did this didn't find anything that seemed general purpose and reusable)
https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... File webrtc/video/bad_call_threshold.cc (right): https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.cc:46: } On 2016/11/15 15:01:01, palmkvist wrote: > On 2016/11/14 13:14:15, språng wrote: > > What tradeoffs are there with doing state change detection outside this class? > > You're suggesting just calculating which state we're in here and letting a > caller figure out if that state is different from the previous? > > It would essentially necessitate code like this: > bool high = thresh.getState(); > thresh.AddMeasurement(thing); > if (high != thresh.getState()) > ... > > Which is slightly more cumbersome than the current approach, but it would > simplify the code in AddMeasurement. > > On the other hand the current scheme does not give a way to retrieve the state > without adding a measurement, which is weird, and any such method would either > only return STILL_LOW or STILL_HIGH, or need to do some extra caching. > > Overally probably worth it to move the change detection out. Maybe also consider returning an rtc::Optional<State>, and don't report a value if there are foo few data points. https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... File webrtc/video/bad_call_threshold.h (right): https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.h:18: class BadCallThreshold { On 2016/11/15 15:01:01, palmkvist wrote: > I am by the way rather unhappy with this name, so any ideas are welcome. Hm, dunno. How about QualityThreshold? https://codereview.webrtc.org/2474913002/diff/20001/webrtc/video/bad_call_thr... webrtc/video/bad_call_threshold.h:35: const std::unique_ptr<int[]> buffer_; On 2016/11/15 15:01:01, palmkvist wrote: > On 2016/11/14 13:14:15, språng wrote: > > Unless we want to be able to dynamically change the threshold at runtime, I > > don't think we actually need to store the values. If you just have a > ringbuffer > > of states and a counter for each state you can add and subtract values without > > looping. > > We do need the values to calculate the variance though. > As for removing the loop, when we need to calculate the variance as well we get > the choice between linear time per measurement and accurate variance or an > imprecise variance (that I'm not entirely sure how it relates) in constant time. > True. Though I think just keeping a sum of squares is good enough. > (Also, is there a preexisting ringbuffer somewhere? A cursory search when I > first did this didn't find anything that seemed general purpose and reusable) There is one in the rtc event log directory. Not sure why it's not in webrtc/base... Otherwise maybe just use a std::deque?
Threshold now only reports high or low when it's certain, e.g. if we start and stay between the thresholds it will never report anything
palmkvist@webrtc.org changed reviewers: + stefan@webrtc.org
First step: try to detect and log when the call appears bad to the user. Later: API to listen for bad calls, detection and prioritisation of possible/likely causes.
https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... File webrtc/video/quality_threshold.cc (right): https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... webrtc/video/quality_threshold.cc:43: if (until_full_ <= 0 && prev_val <= low_threshold_) can until_full_ ever be < 0 ? https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... webrtc/video/quality_threshold.cc:46: count_high_--; if (until_full_ <= 0) { if (prev_val <= low_thershold_) { ... } else if (prev_val >= high_threshold_) { ... } } https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... webrtc/video/quality_threshold.cc:50: if (measurement >= high_threshold_) else if https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... File webrtc/video/quality_threshold_unittest.cc (right): https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... webrtc/video/quality_threshold_unittest.cc:18: QualityThreshold thresh(0, 1, 0.75f, 10); Define constants for high / low / capacity and use them below for loop length and values. https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... webrtc/video/quality_threshold_unittest.cc:38: const int max_measurements = 10; kMaxMeasurements or MAX_MEASUREMENTS https://codereview.webrtc.org/2474913002/diff/80001/webrtc/video/quality_thre... webrtc/video/quality_threshold_unittest.cc:41: double variances[] = {476.9, 687.6, 552, 336.4, 278.767, 265.167}; please comment what these numbers are
lgtm Should follow this up with some histograms, will make it easier to track.
stefan@webrtc.org changed reviewers: + asapersson@webrtc.org
Åsa should review this since it's touching receive_statistics_proxy.cc https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/quality_thr... File webrtc/video/quality_threshold.cc (right): https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/quality_thr... webrtc/video/quality_threshold.cc:41: sum_ += -prev_val + measurement; sum_ += measurement - prev_val; https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/quality_thr... webrtc/video/quality_threshold.cc:45: count_low_--; --count_low_ here and on all other places. https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/quality_thr... webrtc/video/quality_threshold.cc:74: return {}; Never seen this as a short for rtc::Optional<double>() before. Seems odd, so I'd prefer return rtc::Optional<double>(); https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:31: const int kLowQpThreshold = 60; seems like a high threshold to be considered "low". Or does it mean that it's low enough to not be considered "bad"? https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:193: int qp = qp_sample_.Avg(1); What does 1 mean here? https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:429: sum = 0; num_samples_ sum_ https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.h:115: uint64_t last_sample_time_ GUARDED_BY(crit_); int64_t is usually used for time in webrtc.
https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:31: const int kLowQpThreshold = 60; On 2016/11/28 15:55:25, stefan-webrtc (holmer) wrote: > seems like a high threshold to be considered "low". Or does it mean that it's > low enough to not be considered "bad"? It means it is low enough to not be considered bad. Should I add a comment or change the name to clarify? https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:193: int qp = qp_sample_.Avg(1); On 2016/11/28 15:55:24, stefan-webrtc (holmer) wrote: > What does 1 mean here? It's number of samples required to return an average. The intention is to take the average qp since the last QualitySample, so as long as there is a single sample the average is interesting. Should I add a named local variable or comment for clarity?
https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:31: const int kLowQpThreshold = 60; On 2016/12/01 13:03:45, palmkvist wrote: > On 2016/11/28 15:55:25, stefan-webrtc (holmer) wrote: > > seems like a high threshold to be considered "low". Or does it mean that it's > > low enough to not be considered "bad"? > > It means it is low enough to not be considered bad. Should I add a comment or > change the name to clarify? Ok, I'd rename it I think. Comment is probably fine too. https://codereview.webrtc.org/2474913002/diff/100001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:193: int qp = qp_sample_.Avg(1); On 2016/12/01 13:03:45, palmkvist wrote: > On 2016/11/28 15:55:24, stefan-webrtc (holmer) wrote: > > What does 1 mean here? > > It's number of samples required to return an average. The intention is to take > the average qp since the last QualitySample, so as long as there is a single > sample the average is interesting. Should I add a named local variable or > comment for clarity? I see. I guess it's fine as is.
The CQ bit was checked by palmkvist@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/11000)
Description was changed from ========== Logging basic bad call detection BUG= ========== to ========== Logging basic bad call detection BUG=webrtc:6814 ==========
https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/quality_thr... File webrtc/video/quality_threshold.cc (right): https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/quality_thr... webrtc/video/quality_threshold.cc:33: RTC_CHECK_LT(low_threshold, high_threshold); RTC_CHECK max_measurements > 1 https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:35: const int kLowVarianceThreshold = 1; For variance, low enough to be good..? https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:40: const int kHighQpThreshold = 70; Add Vp8 to variable name (since the qp range depends on the codec). https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:49: // 1000ms window, scale 1000 for ms to s. Move comment to decode_fps_estimator_ https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:207: Perhaps check that some minimum time has passed since last_sample_time_. https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:210: int qp = qp_sample_.Avg(1); Handle -1 (i.e. no samples)?
https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2474913002/diff/120001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.cc:210: int qp = qp_sample_.Avg(1); On 2016/12/07 15:34:51, åsapersson wrote: > Handle -1 (i.e. no samples)? My original thinking was that that was ok, since that either means lost frames (i.e. fps will be bad, so we'll report bad anyway) or we're not getting qp data at all (so qp cannot be a cause of a bad judgement), but I guess it's just as well/better to just ignore it.
lgtm
lgtm
The CQ bit was checked by palmkvist@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2474913002/#ps140001 (title: "Comments")
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": 140001, "attempt_start_ts": 1481624789021210,
"parent_rev": "ce271cce954c3cce0b2aceb0d427a69aa4a99bc2", "commit_rev":
"e2ed9a06ead4f50c0f20f154d65e5455fd84479b"}
Message was sent while issue was closed.
Description was changed from ========== Logging basic bad call detection BUG=webrtc:6814 ========== to ========== Logging basic bad call detection BUG=webrtc:6814 Review-Url: https://codereview.webrtc.org/2474913002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Logging basic bad call detection BUG=webrtc:6814 Review-Url: https://codereview.webrtc.org/2474913002 ========== to ========== Logging basic bad call detection BUG=webrtc:6814 Committed: https://crrev.com/349092befe47a10d9179837d0de93c0334fe3088 Cr-Commit-Position: refs/heads/master@{#15568} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/349092befe47a10d9179837d0de93c0334fe3088 Cr-Commit-Position: refs/heads/master@{#15568} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
