Refactor RMSLevel and give it new functionality

webrtc/modules/audio_processing/rms_level.cc

Index: webrtc/modules/audio_processing/rms_level.cc
diff --git a/webrtc/modules/audio_processing/rms_level.cc b/webrtc/modules/audio_processing/rms_level.cc
index 957a7b5839b9d607f5120567d2b67939333cdc2e..532cdbb62f6e6a9d387175f8c00758033536b7be 100644
--- a/webrtc/modules/audio_processing/rms_level.cc
+++ b/webrtc/modules/audio_processing/rms_level.cc
@@ -11,52 +11,89 @@
 #include "webrtc/modules/audio_processing/rms_level.h"
 
 #include <math.h>
+#include <algorithm>
+#include <numeric>
 
 #include "webrtc/base/checks.h"
 
 namespace webrtc {
-
-static const float kMaxSquaredLevel = 32768 * 32768;
+namespace {
+static constexpr float kMaxSquaredLevel = 32768 * 32768;
+static constexpr int kMinLevelDb = 127;
+// kMinLevel is the level corresponding to kMinLevelDb, that is 10^(-127/10).
+static constexpr float kMinLevel = 1.995262314968883e-13f;

[peah-webrtc, 2016/11/29 08:57:13]

Can this be computed from kMinLevelDb using a constexpr?

[hlundin-webrtc, 2016/11/29 10:24:54]

Already tried, but failed. Cannot use static_assert either, since neither
std::pow, nor std::log10 can be used in a constexpr.

+}  // namespace
 
 RMSLevel::RMSLevel()
     : sum_square_(0),

[peah-webrtc, 2016/11/29 08:57:12]

0.f

-      sample_count_(0) {}
+      sample_count_(0),
+      max_mean_square_(0) {}

[peah-webrtc, 2016/11/29 08:57:13]

0.f

[peah-webrtc, 2016/11/29 08:57:13]

Would it make sense to do a call to Reset() inside the constructor instead of
the initializer lists?

[hlundin-webrtc, 2016/11/29 10:24:54]

Done.

 
-RMSLevel::~RMSLevel() {}
+RMSLevel::~RMSLevel() = default;
 
 void RMSLevel::Reset() {
   sum_square_ = 0;

[peah-webrtc, 2016/11/29 08:57:13]

0.f;

[hlundin-webrtc, 2016/11/29 10:24:54]

Done.

   sample_count_ = 0;
+  max_mean_square_ = 0;

[peah-webrtc, 2016/11/29 08:57:13]

0.f;

[hlundin-webrtc, 2016/11/29 10:24:54]

Done.

 }
 
-void RMSLevel::Process(const int16_t* data, size_t length) {
-  for (size_t i = 0; i < length; ++i) {
-    sum_square_ += data[i] * data[i];
+void RMSLevel::Process(rtc::ArrayView<const int16_t> data) {
+  if (data.empty()) {
+    return;
   }
-  sample_count_ += length;
+
+  const float sum_square =
+      std::accumulate(data.begin(), data.end(), 0.f,
+                      [](float a, int16_t b) { return a + b * b; });
+  RTC_DCHECK_GE(sum_square, 0.f);
+  sum_square_ += sum_square;
+  sample_count_ += data.size();
+
+  max_mean_square_ = std::max(max_mean_square_, sum_square / data.size());

[peah-webrtc, 2016/11/29 08:57:13]

Why not do the division only at the call of AverageAndPeak. That is cheaper,
harmonizes with how sum_square is computed, and does not require the comment on
line 90.

In order to really save computations, you could then compute the division once
by precomputing 1/sample_count_ and apply it both to sum_square_ and
max_sum_square_.

[hlundin-webrtc, 2016/11/29 10:24:54]

Changed the implementation to cope with variable block sizes (by resetting).

 }
 
 void RMSLevel::ProcessMuted(size_t length) {
   sample_count_ += length;
 }
 
-int RMSLevel::RMS() {
-  if (sample_count_ == 0 || sum_square_ == 0) {
-    Reset();
-    return kMinLevel;
+namespace {

[peah-webrtc, 2016/11/29 08:57:13]

Is there a special reason for having two anonymous code sections? Would it make
sense to combine them?

[hlundin-webrtc, 2016/11/29 10:24:54]

I thought it made sense to have the code close to where it was called from. Now
I consolidated the namespaces.

+// Calculates the normalized RMS value from a mean square value. The input
+// should be the sum of squared samples divided by the number of samples. The
+// value will be normalized to full range before computing the RMS, wich is
+// returned as a negated dBfs. That is, 0 is full amplitude while 127 is very
+// faint.
+int CalcRms(float mean_square) {

[peah-webrtc, 2016/11/29 08:57:12]

I would suggest avoiding the abbreviation Calc since this method name is already
so short, it already contains an abbreviation, and since the style guide
suggests that
(https://google.github.io/styleguide/cppguide.html#General_Naming_Rules).

What about CalculateRms or ComputeRms?

[hlundin-webrtc, 2016/11/29 10:24:54]

Done.

+  if (mean_square <= kMinLevel * kMaxSquaredLevel) {
+    // Very faint; simply return the minimum value.
+    return kMinLevelDb;
   }
-
   // Normalize by the max level.
-  float rms = sum_square_ / (sample_count_ * kMaxSquaredLevel);
+  const float mean_square_norm = mean_square / kMaxSquaredLevel;
+  RTC_DCHECK_GT(mean_square_norm, kMinLevel);
   // 20log_10(x^0.5) = 10log_10(x)
-  rms = 10 * log10(rms);
+  const float rms = 10 * log10(mean_square_norm);

[peah-webrtc, 2016/11/29 08:57:12]

10.f

[hlundin-webrtc, 2016/11/29 10:24:55]

Done.

   RTC_DCHECK_LE(rms, 0);

[peah-webrtc, 2016/11/29 08:57:12]

0.f

[hlundin-webrtc, 2016/11/29 10:24:54]

Done.

-  if (rms < -kMinLevel)
-    rms = -kMinLevel;
+  RTC_DCHECK_GT(rms, -kMinLevelDb);
+  // Return the negated value.
+  return static_cast<int>(-rms + 0.5);

[peah-webrtc, 2016/11/29 08:57:13]

0.5f

[hlundin-webrtc, 2016/11/29 10:24:54]

Done.

+}
+}  // namespace
+
+int RMSLevel::RMS() {
+  int rms =
+      (sample_count_ == 0) ? kMinLevelDb : CalcRms(sum_square_ / sample_count_);
+  Reset();
+  return rms;
+}
 
-  rms = -rms;
+RMSLevel::Levels RMSLevel::AverageAndPeak() {
+  // Note that max_mean_square_ is already divided by the number of samples.
+  Levels levels = (sample_count_ == 0)
+                      ? Levels{kMinLevelDb, kMinLevelDb}
+                      : Levels{CalcRms(sum_square_ / sample_count_),
+                               CalcRms(max_mean_square_)};
   Reset();
-  return static_cast<int>(rms + 0.5);
+  return levels;
 }
 
 }  // namespace webrtc