Compensate for the IntelligibilityEnhancer processing delay in high bands

webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc

Index: webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
diff --git a/webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc b/webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
index 6e641a23325984bee6daab921a67c9acc959a489..758d4bdba9e9a49fbf0e24e2bd76a97f40c09ef0 100644
--- a/webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
+++ b/webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
@@ -66,6 +66,22 @@ void GainApplier::Apply(const std::complex<float>* in_block,
   }
 }
 
+DelayBuffer::DelayBuffer(size_t delay, size_t num_channels)
+    : buffer(num_channels, std::vector<float>(delay, 0.f)), read_index(0u) {}
+
+DelayBuffer::~DelayBuffer() {}
+
+void DelayBuffer::Delay(float* const* data, size_t length) {
+  for (size_t i = 0u; i < length; ++i) {

[peah-webrtc, 2016/09/16 13:35:56]

I think this implementation is more expensive than needed. 
1) Since buffer.size() is 1 or 2, and each channel is stored in a linear manner,
it is better to flip the nesting of the loop and have the inner loop looping for
the samples.
2) When doing it in a loop with many iterations, the % operator is slower than
using an if statement. 
3) I know that the code is not written like that and that it handles an
arbitrary delay, but that is actually not really necessary, and it has a
potential impact on the complexity. It is a bit unclear to me but I think you
are using 256 point ffts, right? That should mean that the delay should be fixed
to 174 samples in 8 kHz and 94 samples otherwise, right? 
That should mean that the delay is fixed for all sample rates above 8 kHz, this
can be realize using memcpy's which are significantly faster.

[aluebs-webrtc, 2016/09/17 00:48:48]

1) This makes the code less readable, because it requires an intermediate
channel_index variable, but done.
2) This makes the code less readable, since a simple modulo operation requires
an if-statement, but done.
3) You are right that only fixed delays are possible in today's setup, but the
FFT depends on the sample rate, so the delay is 112 for 8kHz and 224 for the
rest. Anyway, I don't see why it is not possible to use memcpy for arbitrary
delays, it is done by the AudioRingBuffer I was using originally. But this would
require double the memory, which was your argument for building this DelayBuffer
class in the first place. So I am leaving it as is.

[peah-webrtc, 2016/09/17 21:31:29]

It may be less readable, but definitely faster.

You are right about the memcpys, they could always be used. But I don't think
the doubling of memory is needed though. But I'm fine with this implementation
though.

[aluebs-webrtc, 2016/09/19 19:17:14]

There is a tradeoff between memory footprint and speed. One extreme is to only
use one float to swap each at a time (what is implemented). The other is to
double the memory and ensure everything is memcpy-ed at once. And then there is
anything in between, using certain memory and doing the necessary amount of
memcpys.

+    for (size_t j = 0u; j < buffer.size(); ++j) {
+      float swap = data[j][i];
+      data[j][i] = buffer[j][read_index];
+      buffer[j][read_index] = swap;
+    }
+    read_index = (read_index + 1u) % buffer.size();
+  }
+}
+
 }  // namespace intelligibility
 
 }  // namespace webrtc