Refactor NACK bitrate allocation

webrtc/base/rate_statistics.cc

Index: webrtc/base/rate_statistics.cc
diff --git a/webrtc/base/rate_statistics.cc b/webrtc/base/rate_statistics.cc
index 1fd63cc6d2a071251edb0a9f4f3744fd700571f2..3122dbb3e690611bbaf80b39711e9c336d74b60a 100644
--- a/webrtc/base/rate_statistics.cc
+++ b/webrtc/base/rate_statistics.cc
@@ -61,8 +61,10 @@ void RateStatistics::Update(size_t count, int64_t now_ms) {
   ++num_samples_;
 }
 
-rtc::Optional<uint32_t> RateStatistics::Rate(int64_t now_ms) {
-  EraseOld(now_ms);
+rtc::Optional<uint32_t> RateStatistics::Rate(int64_t now_ms) const {
+  // Yeah, this const_cast ain't pretty, but the alternative is to declare most
+  // of the members as mutable...

[tommi, 2016/07/08 08:52:20]

there is one more alternative :)

+  const_cast<RateStatistics*>(this)->EraseOld(now_ms);

[tommi, 2016/07/07 12:48:47]

is it correct to call EraseOld in this method?
If so, then the data that EraseOld erases, should be mutable.  Does it belong
together in a nested, class perhaps (and accessed via a mutable member)?

[sprang_webrtc, 2016/07/07 13:46:51]

EraseOld() moves the averaging window. It may mutate every non-const member
apart form current_window_size_ms_. Note sure it's worth creating a wrapper
class for this.

[tommi, 2016/07/08 08:52:20]

ok. given this and that there's a comment that explains that despite being
const, the method modifies the state, I wonder if it should be const?  What
about making it non-const and explain why it's not const?  (I might be missing a
const context that this method needs to be callable from)

nit: can we change the name of the function to GetRate? 

[sprang_webrtc, 2016/07/08 11:46:58]

The big reason for this is that I'm deleting the old
webrtc/modules/rtp_rtcp/source/bitrate.cc 
That implementation had a few problems, one of them being that you needed to
register a module processing thread to actually update the state.

If you look at the first few patch sets, I indeed kept this as non-const method,
but that resulted in needing to change a number of the base rtp/rtcp interfaces,
breaking upstream code and other ugliness. Felt this was preferable...

 
   // If window is a single bucket or there is only one sample in a data set that
   // has not grown to the full window size, treat this as rate unavailable.
@@ -112,7 +114,7 @@ bool RateStatistics::SetWindowSize(int64_t window_size_ms, int64_t now_ms) {
   return true;
 }
 
-bool RateStatistics::IsInitialized() {
+bool RateStatistics::IsInitialized() const {
   return oldest_time_ != -max_window_size_ms_;
 }