Refactor QualityScaler and MovingAverage

webrtc/modules/video_coding/utility/moving_average.cc

Index: webrtc/modules/video_coding/utility/moving_average.cc
diff --git a/webrtc/modules/video_coding/utility/moving_average.cc b/webrtc/modules/video_coding/utility/moving_average.cc
new file mode 100644
index 0000000000000000000000000000000000000000..381eae5d2b9e88431e59d90d31b740b26f1e74c7
--- /dev/null
+++ b/webrtc/modules/video_coding/utility/moving_average.cc
@@ -0,0 +1,45 @@
+/*
+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.

[sakal, 2016/09/05 12:28:10]

You probably mean 2016.

+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */

[sakal, 2016/09/05 12:28:11]

nit: add an empty line here

+#include "webrtc/modules/video_coding/utility/moving_average.h"

[sakal, 2016/09/05 12:28:10]

nit: add an empty line here

+#include <algorithm>
+
+namespace webrtc {
+
+MovingAverage::MovingAverage() : MovingAverage(1024) {}

[sakal, 2016/09/05 12:28:11]

I would leave out the default constructor. I don't feel like there is a sensible
default for the size of the ring buffer so I would leave it to the user to
decide.

+
+MovingAverage::MovingAverage(size_t s) : samples_(s, 0) {}

[sakal, 2016/09/05 12:28:11]

nit: samples(s) would be enough, values are initialized to 0 by default

+
+void MovingAverage::AddSample(int sample) {
+  sum_ += sample;
+  count_++;
+  samples_[count_ % samples_.size()] = sum_;
+}
+
+int MovingAverage::GetAverage() const {
+  return GetAverage(count_ + 1);

[sakal, 2016/09/05 12:28:10]

Why +1? I saw you commented it is an array index on the other CL. However, this
is 1 after adding one sample though?

[magjed_webrtc, 2016/09/05 15:01:52]

Yes, I'm also pretty sure this is a bug. |count_| is the number of samples
added, so there should be no +1. In size() you use it correctly without +1.
Actually, you should consider using size() here instead of |count_|.

+}
+
+int MovingAverage::GetAverage(size_t num_samples) const {

[sakal, 2016/09/05 12:28:11]

I would rather this method return rtc::Optional in case there are not enough
samples.

+  num_samples = std::min(num_samples, this->size());

[magjed_webrtc, 2016/09/05 15:01:52]

Is this check really needed if you use size() in the other GetAverage()
function?

+  if (num_samples == 0) return 0;
+  int sum = sum_ - samples_[(count_ - num_samples) % samples_.size()];

[magjed_webrtc, 2016/09/05 15:01:52]

nit: Use const.

+  return sum / num_samples;

[sakal, 2016/09/05 12:28:10]

This will always round down. Is this the desired behavior? Maybe also worth
considering returning double instead?

+}
+
+void MovingAverage::Reset() {
+  count_ = 0;
+  sum_ = 0;
+  std::fill(samples_.begin(), samples_.end(), 0);

[sakal, 2016/09/05 12:28:11]

nit: This is not necessarily required because we will overwrite the values
before we use them. So, just samples_[0] = 0; would be enough. I am okay with
keeping it if you feel like it though.

+}
+
+size_t MovingAverage::size() const {
+  return std::min(count_, samples_.size() - 1);
+}

[sakal, 2016/09/05 12:28:10]

nit: add an empty line here

+}  // namespace webrtc