Fixing an overflow issue in NetEq::BackgroundNoise

webrtc/modules/audio_coding/neteq/background_noise.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  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.
  */
 
 #include "webrtc/modules/audio_coding/neteq/background_noise.h"
 
 #include <assert.h>
 #include <string.h>  // memcpy
 
 #include <algorithm>  // min, max
+#include <limits>
 
 #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
 #include "webrtc/modules/audio_coding/neteq/audio_multi_vector.h"
 #include "webrtc/modules/audio_coding/neteq/cross_correlation.h"
 #include "webrtc/modules/audio_coding/neteq/post_decode_vad.h"
 
 namespace webrtc {
 
 // static
 const size_t BackgroundNoise::kMaxLpcOrder;
@@ skipping 67 matching lines @@
                                 fiter_output, lpc_coefficients,
                                 kMaxLpcOrder + 1, kResidualLength);
       int32_t residual_energy = WebRtcSpl_DotProductWithScale(fiter_output,
                                                               fiter_output,
                                                               kResidualLength,
                                                               0);
 
       // Check spectral flatness.
       // Comparing the residual variance with the input signal variance tells
       // if the spectrum is flat or not.
-      // If 20 * residual_energy >= sample_energy << 6, the spectrum is flat
+      // If 5 * residual_energy >= sample_energy << 4, the spectrum is flat
       // enough.  Also ensure that the energy is non-zero.
-      if ((residual_energy * 20 >= (sample_energy << 6)) &&
+      // If residual_energy is too large to fit in an int32_t after multiplying
+      // with 5, then simply skip the comparison, and consider the spectrum not
+      // flat enough.
+      constexpr int32_t kMaxEnergyToCompare =
+          std::numeric_limits<int32_t>::max() / 5;
+      if ((residual_energy <= kMaxEnergyToCompare) &&
+          (residual_energy * 5 >= (sample_energy << 4)) &&
           (sample_energy > 0)) {

[kwiberg-webrtc, 2017/02/28 10:31:35]

Hmm. Wouldn't it make more sense to have

  (residual_energy >= kMaxEnergyToCompare) || ...

since the original code enters the if block if residual_energy is *high* enough?

Alternatively, consider doing

  if (sample_energy > 0 && int64_t{5} * residual_energy >= 16 * sample_energy)

(Or use int64_t{16} if the rhs could overflow as well.)

Godbolt experiment here: https://godbolt.org/g/0DvNYi
Note that testing against kMaxEnergyToCompare seems to introduce a branch on all
of x86-64 clang, x86-64 gcc, arm64 gcc, and arm32 gcc. Doing 64-bit arithmetic
seems to result in a bunch more instructions on arm32 (all of them cheap
arithmetic things, though), but looks very efficient on the 64-bit platforms.

[hlundin-webrtc, 2017/02/28 12:15:32]

I went for the int64_t{x} suggestion. Thanks!

         // Spectrum is flat enough; save filter parameters.
         // |temp_signal| + |kVecLen| - |kMaxLpcOrder| points at the first of the
         // |kMaxLpcOrder| samples in the residual signal, which will form the
         // filter state for the next noise generation.
         SaveParameters(channel_ix, lpc_coefficients,
                        temp_signal + kVecLen - kMaxLpcOrder, sample_energy,
                        residual_energy);
       }
     } else {
       // Will only happen if post-decode VAD is disabled and |sample_energy| is
@@ skipping 128 matching lines @@
   // Add 13 to the |scale_shift_|, since the random numbers table is in
   // Q13.
   // TODO(hlundin): Move the "13" to where the |scale_shift_| is used?
   parameters.scale_shift =
       static_cast<int16_t>(13 + ((kLogResidualLength + norm_shift) / 2));
 
   initialized_ = true;
 }
 
 }  // namespace webrtc