Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2995143002:
Report max interframe delay over window insdead of interframe delay sum (Closed)
|
Created:
3 years, 4 months ago by ilnik Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), peah-webrtc, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, kwiberg-webrtc, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReport max interframe delay over window insdead of interframe delay sum
Maximum of interframe delay is calculated over moving window in
ReceiveStatistics proxy now and reported via GetStats. Name of a metric
is also changed.
BUG=none
Review-Url: https://codereview.webrtc.org/2995143002
Cr-Commit-Position: refs/heads/master@{#19463}
Committed: https://chromium.googlesource.com/external/webrtc/+/a79cc28de1fd7f0fa8e331e9bf3d8bbe2a24fa23
Patch Set 1 #
Total comments: 6
Patch Set 2 : Implemented Sprang@ comments #
Total comments: 15
Patch Set 3 : Implement Sprang@ comments #Patch Set 4 : Dont add several samples with the same time. Make tests fail instead of crash on fail. #
Total comments: 36
Patch Set 5 : Implement Kwiberg@ comments #Patch Set 6 : Implement more Kwiberg@ comments #
Total comments: 5
Patch Set 7 : REBASE #Patch Set 8 : Report -1 in case no samples are available for max interframe delay. #Patch Set 9 : Support negative max interframe delay in receivestatsproxy #
Total comments: 2
Patch Set 10 : Implement more Kwiberg@ comments #
Total comments: 10
Patch Set 11 : Implement more Kwiberg@ comments #
Total comments: 6
Patch Set 12 : Implement more Kwiberg@ comments #
Messages
Total messages: 42 (17 generated)
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org
https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... webrtc/video/receive_statistics_proxy.cc:47: const int kMovingMaxWindowMs = 5000; I think this interval needs to be longer. At least 10s. Also, document it in the api. https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... webrtc/video/receive_statistics_proxy.cc:688: samples_.push_back(std::make_pair(time_ms, sample)); emplace_back even? https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... webrtc/video/receive_statistics_proxy.h:128: }; Interesting class. If no equivalent is available in rtc base, maybe you should move it there and templetize it? That would also make it easier to test this logic independently.
https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... webrtc/video/receive_statistics_proxy.cc:47: const int kMovingMaxWindowMs = 5000; On 2017/08/18 09:06:49, sprang_webrtc wrote: > I think this interval needs to be longer. At least 10s. > Also, document it in the api. Done. https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... webrtc/video/receive_statistics_proxy.cc:688: samples_.push_back(std::make_pair(time_ms, sample)); On 2017/08/18 09:06:49, sprang_webrtc wrote: > emplace_back even? Done. https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2995143002/diff/1/webrtc/video/receive_statisti... webrtc/video/receive_statistics_proxy.h:128: }; On 2017/08/18 09:06:49, sprang_webrtc wrote: > Interesting class. If no equivalent is available in rtc base, maybe you should > move it there and templetize it? > That would also make it easier to test this logic independently. Done.
ilnik@webrtc.org changed reviewers: + tommi@webrtc.org
+tommi for /rtc_base
https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. changed to 2017. Will upload in the next patch set.
https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:37: explicit MovingMaxCounter(int window_length_ms); For consistency, maybe use int64_t for times https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:41: rtc::Optional<T> MovingMax(int64_t time_ms) const; Remove Moving from name https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:49: int window_length_ms_; const https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:78: samples_.emplace_back(std::make_pair(time_ms, sample)); if samples_[time_ms] exists, you can just update the value if the new one is larger https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter_unittest.cc (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter_unittest.cc:45: } Add a test coverage for duplicate data points for the same time. https://codereview.webrtc.org/2995143002/diff/20001/webrtc/video/receive_stat... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/video/receive_stat... webrtc/video/receive_statistics_proxy.h:17: #include <utility> Guess you can remove these includes now?
https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:37: explicit MovingMaxCounter(int window_length_ms); On 2017/08/21 14:41:09, sprang_webrtc wrote: > For consistency, maybe use int64_t for times Done. https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:41: rtc::Optional<T> MovingMax(int64_t time_ms) const; On 2017/08/21 14:41:09, sprang_webrtc wrote: > Remove Moving from name Done. https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:49: int window_length_ms_; On 2017/08/21 14:41:09, sprang_webrtc wrote: > const Done. https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:78: samples_.emplace_back(std::make_pair(time_ms, sample)); On 2017/08/21 14:41:09, sprang_webrtc wrote: > if samples_[time_ms] exists, you can just update the value if the new one is > larger Seems like an unneeded optimization to me. If the new value is larger, the old one will be thrown out automatically, If the new value is less, it will be thrown out together with the old one once both of them are expired. We might save some memory space if there are several samples per millisecond, but at the expense of more checks. https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter_unittest.cc (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter_unittest.cc:45: } On 2017/08/21 14:41:09, sprang_webrtc wrote: > Add a test coverage for duplicate data points for the same time. Done. https://codereview.webrtc.org/2995143002/diff/20001/webrtc/video/receive_stat... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/video/receive_stat... webrtc/video/receive_statistics_proxy.h:17: #include <utility> On 2017/08/21 14:41:09, sprang_webrtc wrote: > Guess you can remove these includes now? Done.
sprang@webrtc.org changed reviewers: + kwiberg@webrtc.org - tommi@webrtc.org
lgtm Changing reviewer to kwiberg@ for rtc_base, sounds like a thing for him https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:78: samples_.emplace_back(std::make_pair(time_ms, sample)); On 2017/08/21 14:59:27, ilnik wrote: > On 2017/08/21 14:41:09, sprang_webrtc wrote: > > if samples_[time_ms] exists, you can just update the value if the new one is > > larger > > Seems like an unneeded optimization to me. If the new value is larger, the old > one will be thrown out automatically, If the new value is less, it will be > thrown out together with the old one once both of them are expired. We might > save some memory space if there are several samples per millisecond, but at the > expense of more checks. Agree that it should not matter for the use case in this cl, but if we put it here eventually someone is going to use it for a case where multiple insertions per millisecond is common, so I'd argue that covering that with an extra check would be worth it. It's just an extra line. Also makes it conceptually easier to reason about correctness if there's only a single entry per time. auto it = samples_.emplace_back(std::make_pair(time_ms, sample)); if (!it.second) it.first = std::max<T>(it.first, sample);
https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/20001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:78: samples_.emplace_back(std::make_pair(time_ms, sample)); On 2017/08/22 08:44:50, sprang_webrtc wrote: > On 2017/08/21 14:59:27, ilnik wrote: > > On 2017/08/21 14:41:09, sprang_webrtc wrote: > > > if samples_[time_ms] exists, you can just update the value if the new one is > > > larger > > > > Seems like an unneeded optimization to me. If the new value is larger, the old > > one will be thrown out automatically, If the new value is less, it will be > > thrown out together with the old one once both of them are expired. We might > > save some memory space if there are several samples per millisecond, but at > the > > expense of more checks. > > Agree that it should not matter for the use case in this cl, but if we put it > here eventually someone is going to use it for a case where multiple insertions > per millisecond is common, so I'd argue that covering that with an extra check > would be worth it. It's just an extra line. Also makes it conceptually easier to > reason about correctness if there's only a single entry per time. > > auto it = samples_.emplace_back(std::make_pair(time_ms, sample)); > if (!it.second) it.first = std::max<T>(it.first, sample); Fair enough. I am adding the checks now.
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:31: // |MovingMax|. |time_ms| in consecutive calls to Add and MovingMax should never consecutive -> successive https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:33: // Not thread-safe. Specifically, "Not thread-safe even if you only call const methods." https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:39: void Add(const T& sample, int64_t time_ms); "Advances the current time, and adds a new sample. The new current time must be at least as large as the old current time." Probably also rename time_ms -> current_time_ms. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:41: rtc::Optional<T> Max(int64_t time_ms) const; "Advances the current time, and returns the maximum sample in the time window ending at the current time. The new current time must be at least as large as the old current time." Probably also rename time_ms -> current_time_ms. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: Hmm. It's slightly difficult to document Add() and Max() well, since they each do two things: advance the time and adding or finding the max. It might be easier to use this class if you had this set of public methods instead: void Add(const T& sample); // add new sample at the current time rtc::Optional<T> Max() const; // return max for the current time void AdvanceTime(int64_t current_time_ms); // advance the current time https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:48: void RollWindow(int64_t time_ms) const; I don't really like that this is const---conceptually, it does change the object (by updating its idea of the current time). Would it be terribly inconvenient if it weren't const? https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:56: mutable int64_t last_call_time_ms_; Is last_call_time_ms_ necessary? Couldn't you just use samples_.back().first, if it exists? (last_call_time_ms_ *is* necessary if you take my advice about the AdvanceTime() method.) https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:58: RTC_DISALLOW_COPY_AND_ASSIGN(MovingMaxCounter); Why? Copying a MovingMaxCounter should not be problematic. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:105: int64_t window_begin_ms = time_ms - window_length_ms_; const https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:108: } I'm guessing it'll be faster (but not by a huge margin) to first find an iterator that points to the first element you want to keep, and then make one call to std::deque::erase(). https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter_unittest.cc (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter_unittest.cc:17: EXPECT_EQ(counter.Max(1).value_or(-1), 1); You can write this sort of thing as EXPECT_EQ(rtc::Optional<int>(1), counter.Max(1)); That way, it's obvious to the reader that you're expecting an Optional that contains the specified value.
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:31: // |MovingMax|. |time_ms| in consecutive calls to Add and MovingMax should never On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > consecutive -> successive Done. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:33: // Not thread-safe. On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > Specifically, "Not thread-safe even if you only call const methods." Done. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:39: void Add(const T& sample, int64_t time_ms); On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > "Advances the current time, and adds a new sample. The new current time must be > at least as large as the old current time." > > Probably also rename time_ms -> current_time_ms. Advancing a current time is internal logic, not important to users. I don't think this should be mentioned here. Renaming is fine, done it. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:41: rtc::Optional<T> Max(int64_t time_ms) const; On 2017/08/22 11:13:29, kwiberg-webrtc wrote: > "Advances the current time, and returns the maximum sample in the time window > ending at the current time. The new current time must be at least as large as > the old current time." > > Probably also rename time_ms -> current_time_ms. Ditto. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > Hmm. It's slightly difficult to document Add() and Max() well, since they each > do two things: advance the time and adding or finding the max. It might be > easier to use this class if you had this set of public methods instead: > > void Add(const T& sample); // add new sample at the current time > rtc::Optional<T> Max() const; // return max for the current time > void AdvanceTime(int64_t current_time_ms); // advance the current time The time advancement is an internal thing only. The public documentation is easy: one adds sample at a given time, the other gives maximum in the window with a fixed end time. Implementation specific restriction is that time should never decrease. Additionally, Add() can skip advancement of a time, it will just increase the memory usage. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:48: void RollWindow(int64_t time_ms) const; On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > I don't really like that this is const---conceptually, it does change the object > (by updating its idea of the current time). Would it be terribly inconvenient if > it weren't const? The problem is that Max() is assumed to be const in existing code (and from the external point of view it should be. No sequence of Max() calls will ever change the result of any subsequent Add() or Max() operation). But Max() must call RollWindow(). So it can only be const too. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:56: mutable int64_t last_call_time_ms_; On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > Is last_call_time_ms_ necessary? Couldn't you just use samples_.back().first, if > it exists? (last_call_time_ms_ *is* necessary if you take my advice about the > AdvanceTime() method.) It's necessary. We also need Max() to be called in correct temporal order. Otherwise we can't throw out any samples in Max(), and can never implement it efficiently. Even if no samples were added in between two Max calls: there may be samples added window_len_ms milliseconds ago. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:58: RTC_DISALLOW_COPY_AND_ASSIGN(MovingMaxCounter); On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > Why? Copying a MovingMaxCounter should not be problematic. It doesn't introduce any problems, only if the realtime clock is used to get time_ms. If, however, some fake clock is used, it will have to be copied together with the counter. Also, it doesn't make any sense to copy a counter to me. If you insist, I can remove this line, but other statistics counters are not copyable too (e.g. webrtc/video/stats_counter.h) https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:105: int64_t window_begin_ms = time_ms - window_length_ms_; On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > const Done. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:108: } On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > I'm guessing it'll be faster (but not by a huge margin) to first find an > iterator that points to the first element you want to keep, and then make one > call to std::deque::erase(). Done. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter_unittest.cc (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter_unittest.cc:17: EXPECT_EQ(counter.Max(1).value_or(-1), 1); On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > You can write this sort of thing as > > EXPECT_EQ(rtc::Optional<int>(1), counter.Max(1)); > > That way, it's obvious to the reader that you're expecting an Optional that > contains the specified value. Cool, it never occurred to me to compare optionals directly.
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:39: void Add(const T& sample, int64_t time_ms); On 2017/08/22 12:03:56, ilnik wrote: > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > "Advances the current time, and adds a new sample. The new current time must > be > > at least as large as the old current time." > > > > Probably also rename time_ms -> current_time_ms. > > Advancing a current time is internal logic, not important to users. I don't > think this should be mentioned here. > > Renaming is fine, done it. The reason I suggested mentioning it here, or even having a separate method to advance the time, is that this internal state is visible to callers (by way of the restriction that two successive calls must specify non-decreasing timestamps), so we need to tell them *somehow*. This was simply the most pedagogical way I could think of. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 12:03:56, ilnik wrote: > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > Hmm. It's slightly difficult to document Add() and Max() well, since they each > > do two things: advance the time and adding or finding the max. It might be > > easier to use this class if you had this set of public methods instead: > > > > void Add(const T& sample); // add new sample at the current time > > rtc::Optional<T> Max() const; // return max for the current time > > void AdvanceTime(int64_t current_time_ms); // advance the current time > > The time advancement is an internal thing only. The public documentation is > easy: one adds sample at a given time, the other gives maximum in the window > with a fixed end time. Implementation specific restriction is that time should > never decrease. Well, you can say that it's "internal" and "implementation specific", but that doesn't change the fact that callers need to be aware of how it works in order to avoid making illegal calls; so it must be documented. I found the documentation on this point confusing until I'd read the implementation two or three times, since it tries to pretend that the current time isn't part of the state of the MovingMaxCounter. > Additionally, Add() can skip advancement of a time, it will just increase the > memory usage. Hmm, I don't quite get what you mean here. That you can call Add() several times with the same timestamp? https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:48: void RollWindow(int64_t time_ms) const; On 2017/08/22 12:03:56, ilnik wrote: > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > I don't really like that this is const---conceptually, it does change the > object > > (by updating its idea of the current time). Would it be terribly inconvenient > if > > it weren't const? > > The problem is that Max() is assumed to be const in existing code (and from the > external point of view it should be. No sequence of Max() calls will ever change > the result of any subsequent Add() or Max() operation). This is not true. Consider c.Add(1, 1); Foo(&c); // void Foo(const MovingMaxCounter*); c.Add(2, 2); This sequence of calls is supposed to be unconditionally safe, since Foo() shouldn't be modifying c. But if Foo() calls c->Max(3), this sequence of calls is in fact illegal. Like it or not, as this is currently implemented, MovingMaxCounter de facto keeps track of the most recent timestamp passed to it by an Add() or Max() call, and that state is not invisible to callers. So Max() shouldn't be const. > But Max() must call RollWindow(). So it can only be const too. If it needs to be const, it shouldn't update visible state. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:56: mutable int64_t last_call_time_ms_; On 2017/08/22 12:03:56, ilnik wrote: > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > Is last_call_time_ms_ necessary? Couldn't you just use samples_.back().first, > if > > it exists? (last_call_time_ms_ *is* necessary if you take my advice about the > > AdvanceTime() method.) > > It's necessary. We also need Max() to be called in correct temporal order. > Otherwise we can't throw out any samples in Max(), and can never implement it > efficiently. Even if no samples were added in between two Max calls: there may > be samples added window_len_ms milliseconds ago. If Max() is in fact called in the correct temporal order, you don't need this member; you can just trust that the timestamp argument is correct. But having the last call time saved here does enable you to DCHECK that the caller is doing the right thing, so I agree that it's useful to have it here. As an optimization, consider letting this member exist only if RTC_DCHECK_IS_ON. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:58: RTC_DISALLOW_COPY_AND_ASSIGN(MovingMaxCounter); On 2017/08/22 12:03:56, ilnik wrote: > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > Why? Copying a MovingMaxCounter should not be problematic. > > It doesn't introduce any problems, only if the realtime clock is used to get > time_ms. If, however, some fake clock is used, it will have to be copied > together with the counter. Also, it doesn't make any sense to copy a counter to > me. If you insist, I can remove this line, but other statistics counters are not > copyable too (e.g. webrtc/video/stats_counter.h) OK. Keep this as-is for now.
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:39: void Add(const T& sample, int64_t time_ms); On 2017/08/22 12:35:37, kwiberg-webrtc wrote: > On 2017/08/22 12:03:56, ilnik wrote: > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > "Advances the current time, and adds a new sample. The new current time must > > > be > > > at least as large as the old current time." > > > > > > Probably also rename time_ms -> current_time_ms. > > > > Advancing a current time is internal logic, not important to users. I don't > > think this should be mentioned here. > > > > Renaming is fine, done it. > > The reason I suggested mentioning it here, or even having a separate method to > advance the time, is that this internal state is visible to callers (by way of > the restriction that two successive calls must specify non-decreasing > timestamps), so we need to tell them *somehow*. This was simply the most > pedagogical way I could think of. Ok, I agree, This has to be mentioned here. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 12:35:38, kwiberg-webrtc wrote: > On 2017/08/22 12:03:56, ilnik wrote: > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > Hmm. It's slightly difficult to document Add() and Max() well, since they > each > > > do two things: advance the time and adding or finding the max. It might be > > > easier to use this class if you had this set of public methods instead: > > > > > > void Add(const T& sample); // add new sample at the current time > > > rtc::Optional<T> Max() const; // return max for the current time > > > void AdvanceTime(int64_t current_time_ms); // advance the current time > > > > The time advancement is an internal thing only. The public documentation is > > easy: one adds sample at a given time, the other gives maximum in the window > > with a fixed end time. Implementation specific restriction is that time should > > never decrease. > > Well, you can say that it's "internal" and "implementation specific", but that > doesn't change the fact that callers need to be aware of how it works in order > to avoid making illegal calls; so it must be documented. I found the > documentation on this point confusing until I'd read the implementation two or > three times, since it tries to pretend that the current time isn't part of the > state of the MovingMaxCounter. Callers need to be aware only of one condition: time should always increase in all calls. More comments on that should be enough I think. Splitting Max and AdvanceTime Will not be clear to the user since AdvanceTime will have to be const too, as it will be called from the same GetStats function. > > Additionally, Add() can skip advancement of a time, it will just increase the > > memory usage. > > Hmm, I don't quite get what you mean here. That you can call Add() several times > with the same timestamp? I mean, that RollWindow call can be skipped in Add(). https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:48: void RollWindow(int64_t time_ms) const; On 2017/08/22 12:35:37, kwiberg-webrtc wrote: > On 2017/08/22 12:03:56, ilnik wrote: > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > I don't really like that this is const---conceptually, it does change the > > object > > > (by updating its idea of the current time). Would it be terribly > inconvenient > > if > > > it weren't const? > > > > The problem is that Max() is assumed to be const in existing code (and from > the > > external point of view it should be. No sequence of Max() calls will ever > change > > the result of any subsequent Add() or Max() operation). > > This is not true. Consider > > c.Add(1, 1); > Foo(&c); // void Foo(const MovingMaxCounter*); > c.Add(2, 2); > > This sequence of calls is supposed to be unconditionally safe, since Foo() > shouldn't be modifying c. But if Foo() calls c->Max(3), this sequence of calls > is in fact illegal. > > Like it or not, as this is currently implemented, MovingMaxCounter de facto > keeps track of the most recent timestamp passed to it by an Add() or Max() call, > and that state is not invisible to callers. So Max() shouldn't be const. > > > But Max() must call RollWindow(). So it can only be const too. > > If it needs to be const, it shouldn't update visible state. Yes, it's hard to reflect the realtime-clock properties of time_ms parameter. If, as documented something like realtime clock is used as a parameters for calls, then your mentioned sequence of calls is impossible. There's a way to fix all interface problems at the expense of efficiency and flexibility: just remove time_ms parameters and get realtime from system clock inside the functions. This way users physically can't make illegal call. I don't like this solution, since time functions are not very fast and it's impossible to test. Also, currently, if there's incorrect calls, there'll be failed assert, which will be easy to understand. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:56: mutable int64_t last_call_time_ms_; On 2017/08/22 12:35:38, kwiberg-webrtc wrote: > On 2017/08/22 12:03:56, ilnik wrote: > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > Is last_call_time_ms_ necessary? Couldn't you just use > samples_.back().first, > > if > > > it exists? (last_call_time_ms_ *is* necessary if you take my advice about > the > > > AdvanceTime() method.) > > > > It's necessary. We also need Max() to be called in correct temporal order. > > Otherwise we can't throw out any samples in Max(), and can never implement it > > efficiently. Even if no samples were added in between two Max calls: there may > > be samples added window_len_ms milliseconds ago. > > If Max() is in fact called in the correct temporal order, you don't need this > member; you can just trust that the timestamp argument is correct. But having > the last call time saved here does enable you to DCHECK that the caller is doing > the right thing, so I agree that it's useful to have it here. > > As an optimization, consider letting this member exist only if RTC_DCHECK_IS_ON. Done.
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 13:04:38, ilnik wrote: > On 2017/08/22 12:35:38, kwiberg-webrtc wrote: > > On 2017/08/22 12:03:56, ilnik wrote: > > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > > Hmm. It's slightly difficult to document Add() and Max() well, since they > > each > > > > do two things: advance the time and adding or finding the max. It might be > > > > easier to use this class if you had this set of public methods instead: > > > > > > > > void Add(const T& sample); // add new sample at the current time > > > > rtc::Optional<T> Max() const; // return max for the current time > > > > void AdvanceTime(int64_t current_time_ms); // advance the current time > > > > > > The time advancement is an internal thing only. The public documentation is > > > easy: one adds sample at a given time, the other gives maximum in the window > > > with a fixed end time. Implementation specific restriction is that time > should > > > never decrease. > > > > Well, you can say that it's "internal" and "implementation specific", but that > > doesn't change the fact that callers need to be aware of how it works in order > > to avoid making illegal calls; so it must be documented. I found the > > documentation on this point confusing until I'd read the implementation two or > > three times, since it tries to pretend that the current time isn't part of the > > state of the MovingMaxCounter. > > Callers need to be aware only of one condition: time should always increase in > all calls. More comments on that should be enough I think. Splitting Max and > AdvanceTime > Will not be clear to the user since AdvanceTime will have to be const too, as it > will be called from the same GetStats function. OK, so no separate AdvanceTime() method---that's fine. But I really don't like the idea of Max() both being const and modifying externally visible state---it violates the convention of what "const" is supposed to mean in C++, and makes the behavior highly nonintuitive. How bad would it be to not call RollWindow() in Max()? I'm guessing the net performance effect would be that we'd pay the cost of a RollWindow() call every time we call Max()? How bad would that be? > > > Additionally, Add() can skip advancement of a time, it will just increase > the > > > memory usage. > > > > Hmm, I don't quite get what you mean here. That you can call Add() several > times > > with the same timestamp? > > I mean, that RollWindow call can be skipped in Add(). Oh. Well, that's purely an implementation detail---even if you do that, Add() will still update the object's idea of the current time, by adding a new sample with a new timestamp. No one outside the class would be able to tell the difference. https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:48: void RollWindow(int64_t time_ms) const; On 2017/08/22 13:04:38, ilnik wrote: > On 2017/08/22 12:35:37, kwiberg-webrtc wrote: > > On 2017/08/22 12:03:56, ilnik wrote: > > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > > I don't really like that this is const---conceptually, it does change the > > > object > > > > (by updating its idea of the current time). Would it be terribly > > inconvenient > > > if > > > > it weren't const? > > > > > > The problem is that Max() is assumed to be const in existing code (and from > > the > > > external point of view it should be. No sequence of Max() calls will ever > > change > > > the result of any subsequent Add() or Max() operation). > > > > This is not true. Consider > > > > c.Add(1, 1); > > Foo(&c); // void Foo(const MovingMaxCounter*); > > c.Add(2, 2); > > > > This sequence of calls is supposed to be unconditionally safe, since Foo() > > shouldn't be modifying c. But if Foo() calls c->Max(3), this sequence of calls > > is in fact illegal. > > > > Like it or not, as this is currently implemented, MovingMaxCounter de facto > > keeps track of the most recent timestamp passed to it by an Add() or Max() > call, > > and that state is not invisible to callers. So Max() shouldn't be const. > > > > > But Max() must call RollWindow(). So it can only be const too. > > > > If it needs to be const, it shouldn't update visible state. > > Yes, it's hard to reflect the realtime-clock properties of time_ms parameter. > If, as documented something like realtime clock is used as a parameters for > calls, then your mentioned sequence of calls is impossible. I agree. > There's a way to fix all interface problems at the expense of efficiency and > flexibility: just remove time_ms parameters and get realtime from system clock > inside the functions. This way users physically can't make illegal call. I don't > like this solution, since time functions are not very fast and it's impossible > to test. It's not impossible to test---tests can replace the global clock with a fake. I do agree that there might potentially be unnecessary calls to the clock in production code, though. > Also, currently, if there's incorrect calls, there'll be failed assert, which > will be easy to understand. Yes. But they'll fail at run-time, which isn't as good as at compile-time. https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:70: #endif I think the compiler will complain about a misplaced parenthesis here if you compile with DCHECKs off. Initialize it on line 60 instead---there's already an #if guard there, and no pesky parentheses to worry about.
The CQ bit was checked by ilnik@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: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 13:28:40, kwiberg-webrtc wrote: > On 2017/08/22 13:04:38, ilnik wrote: > > On 2017/08/22 12:35:38, kwiberg-webrtc wrote: > > > On 2017/08/22 12:03:56, ilnik wrote: > > > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > > > Hmm. It's slightly difficult to document Add() and Max() well, since > they > > > each > > > > > do two things: advance the time and adding or finding the max. It might > be > > > > > easier to use this class if you had this set of public methods instead: > > > > > > > > > > void Add(const T& sample); // add new sample at the current time > > > > > rtc::Optional<T> Max() const; // return max for the current time > > > > > void AdvanceTime(int64_t current_time_ms); // advance the current > time > > > > > > > > The time advancement is an internal thing only. The public documentation > is > > > > easy: one adds sample at a given time, the other gives maximum in the > window > > > > with a fixed end time. Implementation specific restriction is that time > > should > > > > never decrease. > > > > > > Well, you can say that it's "internal" and "implementation specific", but > that > > > doesn't change the fact that callers need to be aware of how it works in > order > > > to avoid making illegal calls; so it must be documented. I found the > > > documentation on this point confusing until I'd read the implementation two > or > > > three times, since it tries to pretend that the current time isn't part of > the > > > state of the MovingMaxCounter. > > > > Callers need to be aware only of one condition: time should always increase in > > all calls. More comments on that should be enough I think. Splitting Max and > > AdvanceTime > > Will not be clear to the user since AdvanceTime will have to be const too, as > it > > will be called from the same GetStats function. > > OK, so no separate AdvanceTime() method---that's fine. But I really don't like > the idea of Max() both being const and modifying externally visible state---it > violates the convention of what "const" is supposed to mean in C++, and makes > the behavior highly nonintuitive. > > How bad would it be to not call RollWindow() in Max()? I'm guessing the net > performance effect would be that we'd pay the cost of a RollWindow() call every > time we call Max()? How bad would that be? It may be quite bad. I don't like that It completely destroys all performance guarantees we may give. Currently, this class works in constant time on average for every Add() and Max() call. And it's pretty smooth if samples are spaced regularly. It's because each sample will be removed only once. If instead we are skipping samples in Max(), we will skip each sample as many times as Max() will be called before the next Add(). And it may be arbitrary bad. Also, removing that RollWindow() in Max() somewhat makes |const| more justified, but doesn't remove restrictions that we can't call Max() from the past. I don't see any perfect solutions here: 1) We can leave it as it is. Ugly thing is const Max() which kind-of changes internal state. But if invariant on current_time_ms is respected by the user, it doesn't matter how many or in that order calls are made. 2) Remove current_time_ms parameters and get local time inside Add and Max. Less flexible, works slower. Good that no invariant have to be respected by user. At current load we are observing (<50 samples per second, 1 Max() per second) it doesn't matter, but if this class will be used in other places it may be an issue. 3) Remove RollWindow() in Max(). |const| is somewhat justified, but invariant still needs to be partially preserved by the user in sense that Max() can't be called with time less than the last Add() call had. Also there are workflows where this class works no better than a naive implementation using vector and storing all the samples. Not the problem for the current use in this CL, but this class will be unusable in general case. I personally, like the 1st solution the most. If we document the invariant more clear it will be fine, I hope. https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:70: #endif On 2017/08/22 13:28:41, kwiberg-webrtc wrote: > I think the compiler will complain about a misplaced parenthesis here if you > compile with DCHECKs off. > > Initialize it on line 60 instead---there's already an #if guard there, and no > pesky parentheses to worry about. It doesn't complain locally. Waiting for the trybots results now.
The CQ bit was checked by ilnik@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 checked by ilnik@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/2995143002/diff/100001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:70: #endif On 2017/08/22 13:28:41, kwiberg-webrtc wrote: > I think the compiler will complain about a misplaced parenthesis here if you > compile with DCHECKs off. > > Initialize it on line 60 instead---there's already an #if guard there, and no > pesky parentheses to worry about. Bots do not complain too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 14:11:34, ilnik wrote: > On 2017/08/22 13:28:40, kwiberg-webrtc wrote: > > On 2017/08/22 13:04:38, ilnik wrote: > > > On 2017/08/22 12:35:38, kwiberg-webrtc wrote: > > > > On 2017/08/22 12:03:56, ilnik wrote: > > > > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > > > > Hmm. It's slightly difficult to document Add() and Max() well, since > > they > > > > each > > > > > > do two things: advance the time and adding or finding the max. It > might > > be > > > > > > easier to use this class if you had this set of public methods > instead: > > > > > > > > > > > > void Add(const T& sample); // add new sample at the current time > > > > > > rtc::Optional<T> Max() const; // return max for the current time > > > > > > void AdvanceTime(int64_t current_time_ms); // advance the current > > time > > > > > > > > > > The time advancement is an internal thing only. The public documentation > > is > > > > > easy: one adds sample at a given time, the other gives maximum in the > > window > > > > > with a fixed end time. Implementation specific restriction is that time > > > should > > > > > never decrease. > > > > > > > > Well, you can say that it's "internal" and "implementation specific", but > > that > > > > doesn't change the fact that callers need to be aware of how it works in > > order > > > > to avoid making illegal calls; so it must be documented. I found the > > > > documentation on this point confusing until I'd read the implementation > two > > or > > > > three times, since it tries to pretend that the current time isn't part of > > the > > > > state of the MovingMaxCounter. > > > > > > Callers need to be aware only of one condition: time should always increase > in > > > all calls. More comments on that should be enough I think. Splitting Max and > > > AdvanceTime > > > Will not be clear to the user since AdvanceTime will have to be const too, > as > > it > > > will be called from the same GetStats function. > > > > OK, so no separate AdvanceTime() method---that's fine. But I really don't like > > the idea of Max() both being const and modifying externally visible state---it > > violates the convention of what "const" is supposed to mean in C++, and makes > > the behavior highly nonintuitive. > > > > How bad would it be to not call RollWindow() in Max()? I'm guessing the net > > performance effect would be that we'd pay the cost of a RollWindow() call > every > > time we call Max()? How bad would that be? > > It may be quite bad. I don't like that It completely destroys all performance > guarantees we may give. Currently, this class works in constant time on average > for every Add() and Max() call. And it's pretty smooth if samples are spaced > regularly. It's because each sample will be removed only once. If instead we > are skipping samples in Max(), we will skip each sample as many times as Max() > will be called before the next Add(). And it may be arbitrary bad. > > Also, removing that RollWindow() in Max() somewhat makes |const| more justified, > but doesn't remove restrictions that we can't call Max() from the past. I don't > see any perfect solutions here: > > 1) We can leave it as it is. Ugly thing is const Max() which kind-of changes > internal state. But if invariant on current_time_ms is respected by the user, it > doesn't matter how many or in that order calls are made. > > 2) Remove current_time_ms parameters and get local time inside Add and Max. Less > flexible, works slower. Good that no invariant have to be respected by user. At > current load we are observing (<50 samples per second, 1 Max() per second) it > doesn't matter, but if this class will be used in other places it may be an > issue. > > 3) Remove RollWindow() in Max(). |const| is somewhat justified, but invariant > still needs to be partially preserved by the user in sense that Max() can't be > called with time less than the last Add() call had. Also there are workflows > where this class works no better than a naive implementation using vector and > storing all the samples. Not the problem for the current use in this CL, but > this class will be unusable in general case. > > I personally, like the 1st solution the most. If we document the invariant more > clear it will be fine, I hope. Hmm. None of the options are particularly appealing. Why does Max() have to be const, exactly? Would it be possible to change things so that it can be non-const? https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:70: #endif On 2017/08/22 16:12:44, ilnik wrote: > On 2017/08/22 13:28:41, kwiberg-webrtc wrote: > > I think the compiler will complain about a misplaced parenthesis here if you > > compile with DCHECKs off. > > > > Initialize it on line 60 instead---there's already an #if guard there, and no > > pesky parentheses to worry about. > > Bots do not complain too. Agh, it's just me who can't read. The parentheses are balanced. Even so, you could avoid having an #if/#endif pair here by following my suggestion. Your call. https://codereview.webrtc.org/2995143002/diff/160001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2995143002/diff/160001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.h:156: rtc::MovingMaxCounter<int> interframe_delay_max_moving_ GUARDED_BY(crit_); What if you made this one mutable instead, and made MovingMaxCounter::Max() non-const? This class uses the counter responsibly---always passing the current time, calling Max() but not Add() from const methods---so its callers will not observe any const conventions being broken.
https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/60001/webrtc/rtc_base/moving_ma... webrtc/rtc_base/moving_max_counter.h:43: On 2017/08/22 21:40:46, kwiberg-webrtc wrote: > On 2017/08/22 14:11:34, ilnik wrote: > > On 2017/08/22 13:28:40, kwiberg-webrtc wrote: > > > On 2017/08/22 13:04:38, ilnik wrote: > > > > On 2017/08/22 12:35:38, kwiberg-webrtc wrote: > > > > > On 2017/08/22 12:03:56, ilnik wrote: > > > > > > On 2017/08/22 11:13:30, kwiberg-webrtc wrote: > > > > > > > Hmm. It's slightly difficult to document Add() and Max() well, since > > > they > > > > > each > > > > > > > do two things: advance the time and adding or finding the max. It > > might > > > be > > > > > > > easier to use this class if you had this set of public methods > > instead: > > > > > > > > > > > > > > void Add(const T& sample); // add new sample at the current time > > > > > > > rtc::Optional<T> Max() const; // return max for the current time > > > > > > > void AdvanceTime(int64_t current_time_ms); // advance the current > > > time > > > > > > > > > > > > The time advancement is an internal thing only. The public > documentation > > > is > > > > > > easy: one adds sample at a given time, the other gives maximum in the > > > window > > > > > > with a fixed end time. Implementation specific restriction is that > time > > > > should > > > > > > never decrease. > > > > > > > > > > Well, you can say that it's "internal" and "implementation specific", > but > > > that > > > > > doesn't change the fact that callers need to be aware of how it works in > > > order > > > > > to avoid making illegal calls; so it must be documented. I found the > > > > > documentation on this point confusing until I'd read the implementation > > two > > > or > > > > > three times, since it tries to pretend that the current time isn't part > of > > > the > > > > > state of the MovingMaxCounter. > > > > > > > > Callers need to be aware only of one condition: time should always > increase > > in > > > > all calls. More comments on that should be enough I think. Splitting Max > and > > > > AdvanceTime > > > > Will not be clear to the user since AdvanceTime will have to be const too, > > as > > > it > > > > will be called from the same GetStats function. > > > > > > OK, so no separate AdvanceTime() method---that's fine. But I really don't > like > > > the idea of Max() both being const and modifying externally visible > state---it > > > violates the convention of what "const" is supposed to mean in C++, and > makes > > > the behavior highly nonintuitive. > > > > > > How bad would it be to not call RollWindow() in Max()? I'm guessing the net > > > performance effect would be that we'd pay the cost of a RollWindow() call > > every > > > time we call Max()? How bad would that be? > > > > It may be quite bad. I don't like that It completely destroys all performance > > guarantees we may give. Currently, this class works in constant time on > average > > for every Add() and Max() call. And it's pretty smooth if samples are spaced > > regularly. It's because each sample will be removed only once. If instead we > > are skipping samples in Max(), we will skip each sample as many times as Max() > > will be called before the next Add(). And it may be arbitrary bad. > > > > Also, removing that RollWindow() in Max() somewhat makes |const| more > justified, > > but doesn't remove restrictions that we can't call Max() from the past. I > don't > > see any perfect solutions here: > > > > 1) We can leave it as it is. Ugly thing is const Max() which kind-of changes > > internal state. But if invariant on current_time_ms is respected by the user, > it > > doesn't matter how many or in that order calls are made. > > > > 2) Remove current_time_ms parameters and get local time inside Add and Max. > Less > > flexible, works slower. Good that no invariant have to be respected by user. > At > > current load we are observing (<50 samples per second, 1 Max() per second) it > > doesn't matter, but if this class will be used in other places it may be an > > issue. > > > > 3) Remove RollWindow() in Max(). |const| is somewhat justified, but invariant > > still needs to be partially preserved by the user in sense that Max() can't be > > called with time less than the last Add() call had. Also there are workflows > > where this class works no better than a naive implementation using vector and > > storing all the samples. Not the problem for the current use in this CL, but > > this class will be unusable in general case. > > > > I personally, like the 1st solution the most. If we document the invariant > more > > clear it will be fine, I hope. > > Hmm. None of the options are particularly appealing. > > Why does Max() have to be const, exactly? Would it be possible to change things > so that it can be non-const? Yes, I'll try to make it non const. Instead, as in your suggestion, MovingMaxCounter instance will be mutable. https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/100001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:70: #endif On 2017/08/22 21:40:46, kwiberg-webrtc wrote: > On 2017/08/22 16:12:44, ilnik wrote: > > On 2017/08/22 13:28:41, kwiberg-webrtc wrote: > > > I think the compiler will complain about a misplaced parenthesis here if you > > > compile with DCHECKs off. > > > > > > Initialize it on line 60 instead---there's already an #if guard there, and > no > > > pesky parentheses to worry about. > > > > Bots do not complain too. > > Agh, it's just me who can't read. The parentheses are balanced. > > Even so, you could avoid having an #if/#endif pair here by following my > suggestion. Your call. Yes, it might be more readable that way. Changing it. https://codereview.webrtc.org/2995143002/diff/160001/webrtc/video/receive_sta... File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/2995143002/diff/160001/webrtc/video/receive_sta... webrtc/video/receive_statistics_proxy.h:156: rtc::MovingMaxCounter<int> interframe_delay_max_moving_ GUARDED_BY(crit_); On 2017/08/22 21:40:47, kwiberg-webrtc wrote: > What if you made this one mutable instead, and made MovingMaxCounter::Max() > non-const? > > This class uses the counter responsibly---always passing the current time, > calling Max() but not Add() from const methods---so its callers will not observe > any const conventions being broken. Yes, this is a great idea. Done it.
Excellent. Just a few more comments. :-) https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:33: // Not thread-safe. Now that the mutable members are gone and you don't have const methods, there's nothing special about this class thread-safety-wise, so the note on line 33 isn't needed. https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:53: // thrown out - deque always contains non-increasing sequence of samples. I made an attempt to rephrase this: "This deque stores (timestamp, sample) pairs in chronological order; new pairs are only ever added at the end. However, because they can't affect the Max() calculation, pairs older than window_length_ms_ are discarded, and if an older pair has a sample that's smaller than that of a younger pair, the older pair is discarded. As a result, the sequence of timestamps is non-decreasing, and the sequence of samples is non-increasing." You can use that text, or keep the text you have now (if you replace "what can't" with "that can't")---your choice. https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:71: RollWindow(current_time_ms); Hmm. You repeat lines 67-71 exactly below in the Max() method. How about moving lines 67-70 into RollWindow()? https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:72: // Remove samples what will never be maximum in any window: newly added sample samples that will never https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:78: } Hmm. Why not samples_.back().second <= sample so that the sequence of values becomes strictly decreasing? That would eliminate the case where the deque contains (time, sample) pairs such as (0, 17), (1, 17), (2, 17), ..., (99, 17), and let it contain just (99, 17) instead.
https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:33: // Not thread-safe. On 2017/08/23 09:32:14, kwiberg-webrtc wrote: > Now that the mutable members are gone and you don't have const methods, there's > nothing special about this class thread-safety-wise, so the note on line 33 > isn't needed. Done. https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:53: // thrown out - deque always contains non-increasing sequence of samples. On 2017/08/23 09:32:14, kwiberg-webrtc wrote: > I made an attempt to rephrase this: > > "This deque stores (timestamp, sample) pairs in chronological order; new pairs > are only ever added at the end. However, because they can't affect the Max() > calculation, pairs older than window_length_ms_ are discarded, and if an older > pair has a sample that's smaller than that of a younger pair, the older pair is > discarded. As a result, the sequence of timestamps is non-decreasing, and the > sequence of samples is non-increasing." > > You can use that text, or keep the text you have now (if you replace "what > can't" with "that can't")---your choice. Done. https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:71: RollWindow(current_time_ms); On 2017/08/23 09:32:14, kwiberg-webrtc wrote: > Hmm. You repeat lines 67-71 exactly below in the Max() method. How about moving > lines 67-70 into RollWindow()? Done. https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:72: // Remove samples what will never be maximum in any window: newly added sample On 2017/08/23 09:32:14, kwiberg-webrtc wrote: > samples that will never Done. https://codereview.webrtc.org/2995143002/diff/180001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:78: } On 2017/08/23 09:32:13, kwiberg-webrtc wrote: > Hmm. Why not > > samples_.back().second <= sample > > so that the sequence of values becomes strictly decreasing? That would eliminate > the case where the deque contains (time, sample) pairs such as (0, 17), (1, 17), > (2, 17), ..., (99, 17), and let it contain just (99, 17) instead. You are right. It doesn't affect the results, but will reduce the memory usage. Changed that.
Excellent! lgtm for rtc_base Thanks for hanging in there. It can be frustrating when reviewers just won't shut up... :-) https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:55: // is non-decreasing, and the sequence of samples is decreasing. decreasing -> strictly decreasing https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:73: // contains decreasing sequence of values. contains a strictly decreasing sequence of values. https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:79: // will never be a maximum in any window. Due to the checks above, the already existing element will be at least as large, so the new sample will never be the maximum in any window.
https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... File webrtc/rtc_base/moving_max_counter.h (right): https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:55: // is non-decreasing, and the sequence of samples is decreasing. On 2017/08/23 10:33:33, kwiberg-webrtc wrote: > decreasing -> strictly decreasing Done. https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:73: // contains decreasing sequence of values. On 2017/08/23 10:33:33, kwiberg-webrtc wrote: > contains a strictly decreasing sequence of values. Done. https://codereview.webrtc.org/2995143002/diff/200001/webrtc/rtc_base/moving_m... webrtc/rtc_base/moving_max_counter.h:79: // will never be a maximum in any window. On 2017/08/23 10:33:33, kwiberg-webrtc wrote: > Due to the checks above, the already existing element will be at least as large, > so the new sample will never be the maximum in any window. Rephrased a little.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2995143002/#ps220001 (title: "Implement more Kwiberg@ 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": 220001, "attempt_start_ts": 1503487583887360,
"parent_rev": "1cc5fc3ebfacefe8326abb6c3367be6ddf524957", "commit_rev":
"a79cc28de1fd7f0fa8e331e9bf3d8bbe2a24fa23"}
Message was sent while issue was closed.
Description was changed from ========== Report max interframe delay over window insdead of interframe delay sum Maximum of interframe delay is calculated over moving window in ReceiveStatistics proxy now and reported via GetStats. Name of a metric is also changed. BUG=none ========== to ========== Report max interframe delay over window insdead of interframe delay sum Maximum of interframe delay is calculated over moving window in ReceiveStatistics proxy now and reported via GetStats. Name of a metric is also changed. BUG=none Review-Url: https://codereview.webrtc.org/2995143002 Cr-Commit-Position: refs/heads/master@{#19463} Committed: https://chromium.googlesource.com/external/webrtc/+/a79cc28de1fd7f0fa8e331e9b... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/a79cc28de1fd7f0fa8e331e9b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
