Issue 2419563003: Add algorithm for Residual Echo Detector.

ivoc

Description was changed from ========== Add algorithm for Residual Echo Detector. This algorithm calculates an ...

4 years, 2 months ago (2016-10-13 15:54:42 UTC) #1

ivoc

ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org

4 years, 2 months ago (2016-10-13 15:54:42 UTC) #2

ivoc

Hi guys, this is the follow-up CL that adds the actual algorithm for the residual ...

4 years, 2 months ago (2016-10-13 15:55:40 UTC) #3

hlundin-webrtc

Where are the unit tests? See comments inline too. https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode12 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:12: ...

4 years, 2 months ago (2016-10-14 08:00:40 UTC) #4

peah-webrtc

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { Capital P ...

4 years, 2 months ago (2016-10-14 08:18:59 UTC) #5

kwiberg-webrtc

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { On 2016/10/14 ...

4 years, 2 months ago (2016-10-14 08:58:54 UTC) #6

hlundin-webrtc

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { On 2016/10/14 ...

4 years, 2 months ago (2016-10-14 09:15:29 UTC) #7

peah-webrtc

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { On 2016/10/14 ...

4 years, 2 months ago (2016-10-14 13:39:40 UTC) #8

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
power(const float* farend, size_t nr_samples) {
On 2016/10/14 09:15:28, hlundin-webrtc wrote:
> On 2016/10/14 08:58:54, kwiberg-webrtc wrote:
> > On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> > > With my suggestion in the previous CL to use ArrayView, you can use that
> here
> > as
> > > well. The code then simplifies to
> > > float Power(rtc::ArrayView<const float> signal) {
> > >   double result = 0.;
> > >   for (auto value : signal) {
> > >     result += value * value;
> > >   }
> > >   return static_cast<float>(result);
> > > }
> > > 
> > > If you want to go fancy, you can make use of ArrayView's iterators and
> invoke
> > > std::inner_product. The whole function then collapses to a one-liner:
> > > 
> > > float Power(rtc::ArrayView<const float> x) {
> > >   return static_cast<float>(rtc::inner_product(x.begin(), x.end(),
> x.begin(),
> > > 0.0));
> > > }
> > > 
> > > (I am, however, not sure that the intermediate and final sum will be
double.
> > You
> > > may want to verify that, if it is important.)
> > 
> > The docs suggest that, unless you ask it nicely for something else,
> > std::inner_product will do float * float -> float multiplications and double
+
> > double -> double additions, just like the manual computation you suggest.
> (Would
> > it make sense to do the multiplications in double too?)
> 
> But the current implementation does float * float -> double, then double +
> double, and finally cast to float. Does that make sense from a precision
> point-of-view, and, can you make std::inner_product do that?

In this case, nr_samples is limited to 160. I really doubt that it is worth
doing the processing in double format.

kwiberg-webrtc

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { On 2016/10/14 ...

4 years, 2 months ago (2016-10-14 20:33:33 UTC) #9

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
power(const float* farend, size_t nr_samples) {
On 2016/10/14 09:15:28, hlundin-webrtc wrote:
> On 2016/10/14 08:58:54, kwiberg-webrtc wrote:
> > On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> > > With my suggestion in the previous CL to use ArrayView, you can use that
> here
> > as
> > > well. The code then simplifies to
> > > float Power(rtc::ArrayView<const float> signal) {
> > >   double result = 0.;
> > >   for (auto value : signal) {
> > >     result += value * value;
> > >   }
> > >   return static_cast<float>(result);
> > > }
> > > 
> > > If you want to go fancy, you can make use of ArrayView's iterators and
> invoke
> > > std::inner_product. The whole function then collapses to a one-liner:
> > > 
> > > float Power(rtc::ArrayView<const float> x) {
> > >   return static_cast<float>(rtc::inner_product(x.begin(), x.end(),
> x.begin(),
> > > 0.0));
> > > }
> > > 
> > > (I am, however, not sure that the intermediate and final sum will be
double.
> > You
> > > may want to verify that, if it is important.)
> > 
> > The docs suggest that, unless you ask it nicely for something else,
> > std::inner_product will do float * float -> float multiplications and double
+
> > double -> double additions, just like the manual computation you suggest.
> (Would
> > it make sense to do the multiplications in double too?)
> 
> But the current implementation does float * float -> double, then double +
> double, and finally cast to float. Does that make sense from a precision
> point-of-view, and, can you make std::inner_product do that?

I was going to say that the current implementation does float * float -> float,
then converts the float to a double and does double += double. But I decided to
look it up, and found out about FLT_EVAL_METHOD:
https://en.wikipedia.org/wiki/C99#IEEE.C2.A0754_floating_point_support.
Basically, the compiler is allowed but not required to use a larger type for
intermediate results, and you can look at the value of FLT_EVAL_METHOD to see
what it claims to be doing.

That Wikipedia page suggests that at least gcc on x86-64 does *not* use a larger
type for intermediate values in float and double expressions, so if you want it,
it's probably best to explicitly cast one of the operands to double.

(Separately from this, the compiler is also allowed to use infinite precision
for intermediates if it wants to. I gather this is mostly used with "fused
multiply and add" instructions, which looks like a good choice for the compiler
to employ here.)

And yes, you should be able to make std::inner_product do that. You may
optionally pass it the addition and multiplication functions you want it to use,
so all you have to do is give it std::plus<double> and std::multiplies<double>
if you want double precision for all intermediate values.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
power(const float* farend, size_t nr_samples) {
On 2016/10/14 13:39:40, peah-webrtc wrote:
> On 2016/10/14 09:15:28, hlundin-webrtc wrote:
> > On 2016/10/14 08:58:54, kwiberg-webrtc wrote:
> > > On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> > > > With my suggestion in the previous CL to use ArrayView, you can use that
> > here
> > > as
> > > > well. The code then simplifies to
> > > > float Power(rtc::ArrayView<const float> signal) {
> > > >   double result = 0.;
> > > >   for (auto value : signal) {
> > > >     result += value * value;
> > > >   }
> > > >   return static_cast<float>(result);
> > > > }
> > > > 
> > > > If you want to go fancy, you can make use of ArrayView's iterators and
> > invoke
> > > > std::inner_product. The whole function then collapses to a one-liner:
> > > > 
> > > > float Power(rtc::ArrayView<const float> x) {
> > > >   return static_cast<float>(rtc::inner_product(x.begin(), x.end(),
> > x.begin(),
> > > > 0.0));
> > > > }
> > > > 
> > > > (I am, however, not sure that the intermediate and final sum will be
> double.
> > > You
> > > > may want to verify that, if it is important.)
> > > 
> > > The docs suggest that, unless you ask it nicely for something else,
> > > std::inner_product will do float * float -> float multiplications and
double
> +
> > > double -> double additions, just like the manual computation you suggest.
> > (Would
> > > it make sense to do the multiplications in double too?)
> > 
> > But the current implementation does float * float -> double, then double +
> > double, and finally cast to float. Does that make sense from a precision
> > point-of-view, and, can you make std::inner_product do that?
> 
> In this case, nr_samples is limited to 160. I really doubt that it is worth
> doing the processing in double format.

OK, doing it all in float would simplify things. :-) And using
std::inner_product becomes more attractive since you don't have to pass it extra
options.

hlundin-webrtc

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { On 2016/10/14 ...

4 years, 2 months ago (2016-10-14 21:15:46 UTC) #10

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
power(const float* farend, size_t nr_samples) {
On 2016/10/14 20:33:33, kwiberg-webrtc wrote:
> On 2016/10/14 13:39:40, peah-webrtc wrote:
> > On 2016/10/14 09:15:28, hlundin-webrtc wrote:
> > > On 2016/10/14 08:58:54, kwiberg-webrtc wrote:
> > > > On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> > > > > With my suggestion in the previous CL to use ArrayView, you can use
that
> > > here
> > > > as
> > > > > well. The code then simplifies to
> > > > > float Power(rtc::ArrayView<const float> signal) {
> > > > >   double result = 0.;
> > > > >   for (auto value : signal) {
> > > > >     result += value * value;
> > > > >   }
> > > > >   return static_cast<float>(result);
> > > > > }
> > > > > 
> > > > > If you want to go fancy, you can make use of ArrayView's iterators and
> > > invoke
> > > > > std::inner_product. The whole function then collapses to a one-liner:
> > > > > 
> > > > > float Power(rtc::ArrayView<const float> x) {
> > > > >   return static_cast<float>(rtc::inner_product(x.begin(), x.end(),
> > > x.begin(),
> > > > > 0.0));
> > > > > }
> > > > > 
> > > > > (I am, however, not sure that the intermediate and final sum will be
> > double.
> > > > You
> > > > > may want to verify that, if it is important.)
> > > > 
> > > > The docs suggest that, unless you ask it nicely for something else,
> > > > std::inner_product will do float * float -> float multiplications and
> double
> > +
> > > > double -> double additions, just like the manual computation you
suggest.
> > > (Would
> > > > it make sense to do the multiplications in double too?)
> > > 
> > > But the current implementation does float * float -> double, then double +
> > > double, and finally cast to float. Does that make sense from a precision
> > > point-of-view, and, can you make std::inner_product do that?
> > 
> > In this case, nr_samples is limited to 160. I really doubt that it is worth
> > doing the processing in double format.
> 
> OK, doing it all in float would simplify things. :-) And using
> std::inner_product becomes more attractive since you don't have to pass it
extra
> options.

It seems to me that storing the intermediate as float or double doesn't matter.
I don't have strong opinions whether to use std::inner_product or simply stick
to what you have, but passing special operators to std::inner_product is not
attractive.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:20: for (size_t i
= 0; i < nr_samples; i++) {
On 2016/10/14 08:18:58, peah-webrtc wrote:
> ++i is faster

Not for a simple scalar.
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement
But it doesn't hurt to be consistent and always use pre-increment.

ivoc

Thanks for all the comments! I tried to address all the feedback and added unittests. ...

4 years, 2 months ago (2016-10-17 16:05:09 UTC) #11

Thanks for all the comments! I tried to address all the feedback and added
unittests. PTAL

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:12: #include
"webrtc/modules/audio_processing/echo_detector/echo_detector.h"
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> Aways include the corresponding .h file before any other includes.
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:14: #include
<math.h>
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> System includes go before own project's includes (except echo_detector.h).
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
power(const float* farend, size_t nr_samples) {
On 2016/10/14 21:15:46, hlundin-webrtc wrote:
> On 2016/10/14 20:33:33, kwiberg-webrtc wrote:
> > On 2016/10/14 13:39:40, peah-webrtc wrote:
> > > On 2016/10/14 09:15:28, hlundin-webrtc wrote:
> > > > On 2016/10/14 08:58:54, kwiberg-webrtc wrote:
> > > > > On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> > > > > > With my suggestion in the previous CL to use ArrayView, you can use
> that
> > > > here
> > > > > as
> > > > > > well. The code then simplifies to
> > > > > > float Power(rtc::ArrayView<const float> signal) {
> > > > > >   double result = 0.;
> > > > > >   for (auto value : signal) {
> > > > > >     result += value * value;
> > > > > >   }
> > > > > >   return static_cast<float>(result);
> > > > > > }
> > > > > > 
> > > > > > If you want to go fancy, you can make use of ArrayView's iterators
and
> > > > invoke
> > > > > > std::inner_product. The whole function then collapses to a
one-liner:
> > > > > > 
> > > > > > float Power(rtc::ArrayView<const float> x) {
> > > > > >   return static_cast<float>(rtc::inner_product(x.begin(), x.end(),
> > > > x.begin(),
> > > > > > 0.0));
> > > > > > }
> > > > > > 
> > > > > > (I am, however, not sure that the intermediate and final sum will be
> > > double.
> > > > > You
> > > > > > may want to verify that, if it is important.)
> > > > > 
> > > > > The docs suggest that, unless you ask it nicely for something else,
> > > > > std::inner_product will do float * float -> float multiplications and
> > double
> > > +
> > > > > double -> double additions, just like the manual computation you
> suggest.
> > > > (Would
> > > > > it make sense to do the multiplications in double too?)
> > > > 
> > > > But the current implementation does float * float -> double, then double
+
> > > > double, and finally cast to float. Does that make sense from a precision
> > > > point-of-view, and, can you make std::inner_product do that?
> > > 
> > > In this case, nr_samples is limited to 160. I really doubt that it is
worth
> > > doing the processing in double format.
> > 
> > OK, doing it all in float would simplify things. :-) And using
> > std::inner_product becomes more attractive since you don't have to pass it
> extra
> > options.
> 
> It seems to me that storing the intermediate as float or double doesn't
matter.
> I don't have strong opinions whether to use std::inner_product or simply stick
> to what you have, but passing special operators to std::inner_product is not
> attractive.

Thanks for all the comments, this seems to be a popular topic for discussion :-)
I decided to use innter_product and do everything in floats.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:19: double result
= 0.;
On 2016/10/14 08:18:58, peah-webrtc wrote:
> I think you can use float for the power computation.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:37: // Variance
is >= 0, so this is okay.
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> DCHECK this assumption.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:50: void
CovarianceEstimator::UpdateCovarianceEstimate(float x,
On 2016/10/14 08:18:58, peah-webrtc wrote:
> I don't think that covariance is fully correct here. What is computed is the
> normalized covariance/cross-correlation. Would it be possibly to change the
> naming to reflect what it is being computed?

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:57: (1.f -
alpha_) * covariance_ + alpha_ * (x - x_mean) * (y - y_mean);
On 2016/10/14 08:18:58, peah-webrtc wrote:
> The style guide mentions that abbreviations should be avoided in naming. Would
> it be possible to rename PCC? What about CrossCorrelation or normalize
> cross-correlation?

I chose normalized_cross_correlation.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:61: float
CovarianceEstimator::PCCEstimate() {
On 2016/10/14 08:18:58, peah-webrtc wrote:
> Please change this method name to avoid the abbreviation.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:71:
CircularBuffer::~CircularBuffer() {}
On 2016/10/14 08:18:57, peah-webrtc wrote:
> Alternatively you can use = default; 

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:74:
buffer_[next_insertion_index_] = value;
On 2016/10/14 08:18:57, peah-webrtc wrote:
> Please add a DCHECK that next_insertion_index_ < buffer_.size()

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:75:
next_insertion_index_++;
On 2016/10/14 08:18:57, peah-webrtc wrote:
> ++next_insertion_index_ is faster.

For scalars I don't think it is, but you're right that it's probably better to
use it everywhere.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:76:
next_insertion_index_ %= size_;
On 2016/10/14 08:18:58, peah-webrtc wrote:
> buffer_.size() here and elsewhere.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:80:
RTC_DCHECK(delay >= 0 && delay < size_);
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> Separate into two DCHECKs to get better printouts. Also, use DCHECK_GE/LT.

Since I changed delay to size_t, the >=0 check doesn't seem useful anymore. I
did switch to LT for the < size() check.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:81: const int
index = (size_ + next_insertion_index_ - delay - 1) % size_;
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> size_t

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:82: return
buffer_[index];
On 2016/10/14 08:18:58, peah-webrtc wrote:
> Please add a DCHECK that index < buffer_.size() and that index >= 0

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:91: :
render_buffer_(render_buffer_size_),
On 2016/10/14 08:18:58, peah-webrtc wrote:
> Do you need to have these in the initializer list? Can't you initialize them
> inside at the declaration instead? That would shorten the code and also place
> the initialization closer to the constant they are initialized to (which makes
> it easier to see what they are initialized to).

Good idea!

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:96:
EchoDetector::~EchoDetector() {}
On 2016/10/14 08:18:58, peah-webrtc wrote:
> Alternatively you can use = default; 

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:98: void
EchoDetector::BufferFarend(const float* farend, size_t num_samples) {
On 2016/10/14 08:18:57, peah-webrtc wrote:
> After some discussions I think we will be changing farend to render and
nearend
> to capture for all other submodules. So probably it would make sense to do
that
> there as well.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:99: float pow =
power(farend, num_samples);
On 2016/10/14 08:18:57, peah-webrtc wrote:
> If you change the name of the power method to Power, you don't need to
> abbreviate the power with pow.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:106: const float
cap_pow = power(nearend, num_samples);
On 2016/10/14 08:18:57, peah-webrtc wrote:
> I think that along the style guide abbreviation rule it would be good not to
> abbreviate capture as cap. Same thing with render and ren

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:112: const float
latest_ren_pow = render_buffer_.GetValue(render_buffer_delay_);
On 2016/10/14 08:18:58, peah-webrtc wrote:
> This delay is only used to access the most recently added value in the render
> buffer, right? 
> Since the render buffer knows that the delay and the latest value that was
> added, it is a bit of double bookkeeping keeping track of that here as well. 
> What about adding a getter function to the render_buffer that does this? Or am
I
> missing something?

As discussed offline, I moved the buffer size into the circular buffer class,
and converted the other uses of the circular buffer to regular vectors.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:119: for (int
delay = 0; delay < lookback_periods_; delay++) {
On 2016/10/14 08:18:58, peah-webrtc wrote:
> Use covariance_.size()

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:125:
echo_likelihood =
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> No need for the local variable echo_likelihood. You could just as well update
> echo_likelihood_ here.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:131: void
EchoDetector::Initialize(int sample_rate_hz) {
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> sample_rate_hz not used? Delete it.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:144: float
EchoDetector::EchoLikelihood() {
On 2016/10/14 08:18:57, peah-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:20: class
MeanVarianceEstimator {
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> This and the two next classes are an internal matter for EchoDetector. Either
> make them internal to that one (hard to test, should only be done for more or
> less trivial help classes) or declare and implement them in their own files
> (preferred).

Ok, I will move each to its own file.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:23: float
SigmaEstimate();
On 2016/10/14 08:18:58, peah-webrtc wrote:
> const

Done, I went with std_deviation (without estimate) as a new name, to make it
more consistent with the new name of mean().

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:24: float
MeanEstimate();
On 2016/10/14 08:18:58, peah-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:33: static
constexpr float alpha_ = 0.01f;
On 2016/10/14 08:00:40, hlundin-webrtc wrote:
> static const members should go before variable members, and should be named
> kAlpha.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:45: float
PCCEstimate();
On 2016/10/14 08:00:40, hlundin-webrtc wrote:
> Comment, so that the reader understands what PCC means.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:51: float PCC_ =
0.f;
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> pcc_

I renamed this to normalized_cross_correlation_, following Per's suggestion.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:55: static
constexpr float alpha_ = 0.01f;
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> Same comments as above: move up and rename to kAlpha.

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:61:
CircularBuffer(int size);
On 2016/10/14 08:00:40, hlundin-webrtc wrote:
> size_t size

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:70: const int
size_;
On 2016/10/14 08:18:58, peah-webrtc wrote:
> You don't need the size_ constant. Use buffer_.size() instead.

I thought it would be more efficient to store it than to call the .size()
function repeatedly. Perhaps you're right that it's better to remove it (I guess
it is a premature optimization).

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:71: int
next_insertion_index_ = 0;
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> size_t

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:85: float
EchoLikelihood();
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> float echo_likelihood() const { return echo_likelihood_; }

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:88: static
constexpr int lookback_periods_ = 650;
On 2016/10/14 08:18:58, peah-webrtc wrote:
> size_t?

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:89: static
constexpr int render_buffer_size_ = 20;
On 2016/10/14 08:18:58, peah-webrtc wrote:
> size_t?

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:95: int
render_buffer_delay_ = 0;
On 2016/10/14 08:18:58, peah-webrtc wrote:
> size_t?

Done.

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:101:
CircularBuffer render_power_sigma_;
On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> render_power_std_dev_

Done.

hlundin-webrtc

Much improved! https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode18 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float power(const float* farend, size_t nr_samples) { ...

4 years, 2 months ago (2016-10-17 18:49:43 UTC) #12

Much improved!

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
power(const float* farend, size_t nr_samples) {
On 2016/10/17 16:05:07, ivoc wrote:
> On 2016/10/14 21:15:46, hlundin-webrtc wrote:
> > On 2016/10/14 20:33:33, kwiberg-webrtc wrote:
> > > On 2016/10/14 13:39:40, peah-webrtc wrote:
> > > > On 2016/10/14 09:15:28, hlundin-webrtc wrote:
> > > > > On 2016/10/14 08:58:54, kwiberg-webrtc wrote:
> > > > > > On 2016/10/14 08:00:39, hlundin-webrtc wrote:
> > > > > > > With my suggestion in the previous CL to use ArrayView, you can
use
> > that
> > > > > here
> > > > > > as
> > > > > > > well. The code then simplifies to
> > > > > > > float Power(rtc::ArrayView<const float> signal) {
> > > > > > >   double result = 0.;
> > > > > > >   for (auto value : signal) {
> > > > > > >     result += value * value;
> > > > > > >   }
> > > > > > >   return static_cast<float>(result);
> > > > > > > }
> > > > > > > 
> > > > > > > If you want to go fancy, you can make use of ArrayView's iterators
> and
> > > > > invoke
> > > > > > > std::inner_product. The whole function then collapses to a
> one-liner:
> > > > > > > 
> > > > > > > float Power(rtc::ArrayView<const float> x) {
> > > > > > >   return static_cast<float>(rtc::inner_product(x.begin(), x.end(),
> > > > > x.begin(),
> > > > > > > 0.0));
> > > > > > > }
> > > > > > > 
> > > > > > > (I am, however, not sure that the intermediate and final sum will
be
> > > > double.
> > > > > > You
> > > > > > > may want to verify that, if it is important.)
> > > > > > 
> > > > > > The docs suggest that, unless you ask it nicely for something else,
> > > > > > std::inner_product will do float * float -> float multiplications
and
> > > double
> > > > +
> > > > > > double -> double additions, just like the manual computation you
> > suggest.
> > > > > (Would
> > > > > > it make sense to do the multiplications in double too?)
> > > > > 
> > > > > But the current implementation does float * float -> double, then
double
> +
> > > > > double, and finally cast to float. Does that make sense from a
precision
> > > > > point-of-view, and, can you make std::inner_product do that?
> > > > 
> > > > In this case, nr_samples is limited to 160. I really doubt that it is
> worth
> > > > doing the processing in double format.
> > > 
> > > OK, doing it all in float would simplify things. :-) And using
> > > std::inner_product becomes more attractive since you don't have to pass it
> > extra
> > > options.
> > 
> > It seems to me that storing the intermediate as float or double doesn't
> matter.
> > I don't have strong opinions whether to use std::inner_product or simply
stick
> > to what you have, but passing special operators to std::inner_product is not
> > attractive.
> 
> Thanks for all the comments, this seems to be a popular topic for discussion
:-)
> I decided to use innter_product and do everything in floats.

Good call!

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:41: } else {
In these cases, where you have one large "good code path" and one small "error
code path", I prefer to exit early on error. That is:
if (buffer_size_ == 0) {
  LOG...;
  return 0.f;
}
// rest of code goes here...

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:43: return 0.f;
What will happens where this is used? Would it make sense to return an
Optional<float>, or will ut just mess up the calling code?

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.h (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.h:26: float
GetValue();
I think this should be called Pop. This makes it clearer that values are
removed, and not just viewed. It also aligns with the terminology of STL. (You
may want to rename Insert to Push, but I consider that optional, as Insert is
quite clear in this context.)

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.h:33: size_t
buffer_size_ = 0;
Please, comment that this is not the allocated size, but the number of elements
inserted.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
Power(const rtc::ArrayView<const float>& input) {
Pass the ArrayView by value, not as ref.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:67:
RTC_DCHECK_LT(next_insertion_index_, kLookbackFrames);
DCHECKing immediately after the modulo is maybe not the most important. Consider
moving this DCHECK to before the index is used in line 47 instead. This will
make sure that it is not tampered with _somewhere else_ before being used next
time.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:28: void
BufferRender(const rtc::ArrayView<const float>& render);
Pass ArrayView by value, not by reference.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:30: void
Process(const rtc::ArrayView<const float>& nearend);
Same here.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:45: // Circular
buffers containing delayed versions of the power, mean and sigma,
sigma -> standard deviation

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:1: /*
None of the tests in this file test the EchoDetector class. They should all be
moved to other test files, corresponding to the class they are actually testing.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:20:
TEST(EchoDetectorTests, CircularBufferTest) {
This tests the CircularBuffer class. Move to circular_buffer_unittest.cc.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:21:
CircularBuffer test_buffer(3);
Test a few other cases too, like reading from an empty buffer, inserting less
than max number of elements, etc.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:33:
TEST(EchoDetectorTests, MeanVarianceEstimatorInsertTwoValues) {
Move to mean_variance_estimator_unittest.cc.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:39:
EXPECT_GT(test_estimator.mean(), 0.f);
Can't you EXPECT_EQ instead?

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:40:
EXPECT_GT(test_estimator.std_deviation(), 0.f);
And here.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:50: for
(int i = 0; i < 10000; i++) {
size_t

Several places.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:64:
EXPECT_LT(std::abs(test_estimator.mean() - 3.f), 0.01f);
Use EXPECT_FLOAT_EQ if the values are close enough, or EXPECT_NEAR otherwise.
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

Here, and several places below.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:80:
TEST(EchoDetectorTests, NormalizedCovarianceEstimatorIdenticalSignalTest) {
Move to normalized_covariance_estimator_unittests.cc

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:81:
MeanVarianceEstimator mean_variance_estimator;
You don't need this object, right? The test will be cleaner if you just feed the
test_estimator directly with the input numbers.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:107:
NormalizedCovarianceEstimator test_estimator;
Same comments apply to this as to the previous test.

ivoc

I addressed the comments and added two new tests that do actually test the EchoDetector, ...

4 years, 2 months ago (2016-10-18 15:20:20 UTC) #13

I addressed the comments and added two new tests that do actually test the
EchoDetector, PTAL

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:41: } else {
On 2016/10/17 18:49:39, hlundin-webrtc wrote:
> In these cases, where you have one large "good code path" and one small "error
> code path", I prefer to exit early on error. That is:
> if (buffer_size_ == 0) {
>   LOG...;
>   return 0.f;
> }
> // rest of code goes here...

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:43: return 0.f;
On 2016/10/17 18:49:39, hlundin-webrtc wrote:
> What will happens where this is used? Would it make sense to return an
> Optional<float>, or will ut just mess up the calling code?

I thought this should not happen, but now I'm not sure if clock drift could
cause this to happen. I'll ask Per about it. Anyway, returning an optional is
probably a better idea than just returning zero, so I'll do that instead.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.h (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.h:26: float
GetValue();
On 2016/10/17 18:49:40, hlundin-webrtc wrote:
> I think this should be called Pop. This makes it clearer that values are
> removed, and not just viewed. It also aligns with the terminology of STL. (You
> may want to rename Insert to Push, but I consider that optional, as Insert is
> quite clear in this context.)

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.h:33: size_t
buffer_size_ = 0;
On 2016/10/17 18:49:40, hlundin-webrtc wrote:
> Please, comment that this is not the allocated size, but the number of
elements
> inserted.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:18: float
Power(const rtc::ArrayView<const float>& input) {
On 2016/10/17 18:49:40, hlundin-webrtc wrote:
> Pass the ArrayView by value, not as ref.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:67:
RTC_DCHECK_LT(next_insertion_index_, kLookbackFrames);
On 2016/10/17 18:49:40, hlundin-webrtc wrote:
> DCHECKing immediately after the modulo is maybe not the most important.
Consider
> moving this DCHECK to before the index is used in line 47 instead. This will
> make sure that it is not tampered with _somewhere else_ before being used next
> time.

Good idea.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:28: void
BufferRender(const rtc::ArrayView<const float>& render);
On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> Pass ArrayView by value, not by reference.

Oh, oops, I thought I changed that but it seems to have gotten lost in some
hasty gitting.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:30: void
Process(const rtc::ArrayView<const float>& nearend);
On 2016/10/17 18:49:40, hlundin-webrtc wrote:
> Same here.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:45: // Circular
buffers containing delayed versions of the power, mean and sigma,
On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> sigma -> standard deviation

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:1: /*
On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> None of the tests in this file test the EchoDetector class. They should all be
> moved to other test files, corresponding to the class they are actually
testing.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:20:
TEST(EchoDetectorTests, CircularBufferTest) {
On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> This tests the CircularBuffer class. Move to circular_buffer_unittest.cc.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:33:
TEST(EchoDetectorTests, MeanVarianceEstimatorInsertTwoValues) {
On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> Move to mean_variance_estimator_unittest.cc.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:39:
EXPECT_GT(test_estimator.mean(), 0.f);
On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> Can't you EXPECT_EQ instead?

I could, but then the value depends on the exact implementation of the
MeanVarianceEstimator, in particular the alpha value. I thought it would be
useful to have one test just to see that it changes in the right direction, as
well as to see if Clear() is doing its job. The other tests are already testing
if the value converges to the correct value, so I'm not sure if it adds value to
also do that here. If you don't think it's a useful test I can remove it and
move the Clear() part to one of the others.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:50: for
(int i = 0; i < 10000; i++) {
On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> size_t
> 
> Several places.

Done, but the style guide says not to use size_t everywhere:
https://google.github.io/styleguide/cppguide.html#Integer_Types

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:64:
EXPECT_LT(std::abs(test_estimator.mean() - 3.f), 0.01f);
On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> Use EXPECT_FLOAT_EQ if the values are close enough, or EXPECT_NEAR otherwise.
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> 
> Here, and several places below.

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:80:
TEST(EchoDetectorTests, NormalizedCovarianceEstimatorIdenticalSignalTest) {
On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> Move to normalized_covariance_estimator_unittests.cc

Done.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:81:
MeanVarianceEstimator mean_variance_estimator;
On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> You don't need this object, right? The test will be cleaner if you just feed
the
> test_estimator directly with the input numbers.

Ok, I can remove it and use the true values. This should make it a bit easier
for the NormalizedCovarianceEstimator as it no longer has to rely on estimates.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:107:
NormalizedCovarianceEstimator test_estimator;
On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> Same comments apply to this as to the previous test.

Done.

hlundin-webrtc

Only a few minor comments left. After that, lgtm. Well done! https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_processing/echo_detector/echo_detector.h File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): ...

4 years, 2 months ago (2016-10-18 20:52:22 UTC) #14

Only a few minor comments left. After that, lgtm. Well done!

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:28: void
BufferRender(const rtc::ArrayView<const float>& render);
On 2016/10/18 15:20:19, ivoc wrote:
> On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> > Pass ArrayView by value, not by reference.
> 
> Oh, oops, I thought I changed that but it seems to have gotten lost in some
> hasty gitting.

When the gittings get hasty, the hasty get gitting...

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:39:
EXPECT_GT(test_estimator.mean(), 0.f);
On 2016/10/18 15:20:19, ivoc wrote:
> On 2016/10/17 18:49:41, hlundin-webrtc wrote:
> > Can't you EXPECT_EQ instead?
> 
> I could, but then the value depends on the exact implementation of the
> MeanVarianceEstimator, in particular the alpha value. I thought it would be
> useful to have one test just to see that it changes in the right direction, as
> well as to see if Clear() is doing its job. The other tests are already
testing
> if the value converges to the correct value, so I'm not sure if it adds value
to
> also do that here. If you don't think it's a useful test I can remove it and
> move the Clear() part to one of the others.

This is fine.

https://codereview.webrtc.org/2419563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:50: for
(int i = 0; i < 10000; i++) {
On 2016/10/18 15:20:19, ivoc wrote:
> On 2016/10/17 18:49:42, hlundin-webrtc wrote:
> > size_t
> > 
> > Several places.
> 
> Done, but the style guide says not to use size_t everywhere:
> https://google.github.io/styleguide/cppguide.html#Integer_Types

I think the restriction applies to the fixed-width types, e.g., int16_t,
uint32_t. These should only be used when really needed and motivated. size_t
should be used whenever counting bytes, items, samples, etc. But in this case, I
got a bit carried away – the index variable is only used to specify the for
loop, and it is not used to index into an array or vector, so an int would have
been fine too. Trigger happy reviewer.

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right):

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:35:
LOG(LS_ERROR) << "Attempted to get value from buffer, but it was empty.";
Now you are logging the same error twice – here and where this is called from.
With the use of Optional, I would say that this is not an error in the
CircularBuffer per se, it just happened to be empty and thus return empty.

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:37: } else {
You don't need the else here, since the if path returns. Let the remainder of
the function be on "function-level indentation", i.e.,

rtc::Optional<float> CircularBuffer::Pop() {
  if (odd_things) {
    // Deal with it.
    return rtc::Optional<float>();
  }

  // Life is good. Do awesome stuff, but with less indentation.
  return rtc::Optional<float>(awesome_result);
}

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/echo_detector/normalized_covariance_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/normalized_covariance_estimator_unittest.cc:19:
for (size_t i = 0; i < 10000; i++) {
Now one of the for loops is indexed with an int, and the other with a size_t.
Align to either one of them.

ivoc

Based on discussions with Per earlier today I improved the handling of clock drift on ...

4 years, 2 months ago (2016-10-19 14:12:04 UTC) #15

Based on discussions with Per earlier today I improved the handling of clock
drift on the render side. It works by keeping track of the minimum of the render
buffer size over from the last N updates, and if that minimum is larger than
zero, drop the excess render frames. To do this I added an extra class to
efficiently calculate the minimum value of the last N values, and some
accompanying unittests. I also added two EchoDetector unittests simulating clock
drift.

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right):

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:35:
LOG(LS_ERROR) << "Attempted to get value from buffer, but it was empty.";
On 2016/10/18 20:52:22, hlundin-webrtc wrote:
> Now you are logging the same error twice – here and where this is called from.
> With the use of Optional, I would say that this is not an error in the
> CircularBuffer per se, it just happened to be empty and thus return empty. 

Good point, I will remove this one.

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:37: } else {
On 2016/10/18 20:52:22, hlundin-webrtc wrote:
> You don't need the else here, since the if path returns. Let the remainder of
> the function be on "function-level indentation", i.e.,
> 
> rtc::Optional<float> CircularBuffer::Pop() {
>   if (odd_things) {
>     // Deal with it.
>     return rtc::Optional<float>();
>   }
> 
>   // Life is good. Do awesome stuff, but with less indentation.
>   return rtc::Optional<float>(awesome_result);
> }
> 

Done.

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/echo_detector/normalized_covariance_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/normalized_covariance_estimator_unittest.cc:19:
for (size_t i = 0; i < 10000; i++) {
On 2016/10/18 20:52:22, hlundin-webrtc wrote:
> Now one of the for loops is indexed with an int, and the other with a size_t.
> Align to either one of them.

Done.

peah-webrtc

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_processing/echo_detector/circular_buffer.cc File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right): https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_processing/echo_detector/circular_buffer.cc#newcode20 webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:20: CircularBuffer::CircularBuffer(CircularBuffer&& other) = default; I don't think you need ...

4 years, 2 months ago (2016-10-19 14:45:26 UTC) #16

ivoc

I removed the shiny new class again because Per provided a better suggestion :-) PTAL. ...

4 years, 2 months ago (2016-10-20 14:04:36 UTC) #17

I removed the shiny new class again because Per provided a better suggestion :-)
PTAL.

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right):

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:20:
CircularBuffer::CircularBuffer(CircularBuffer&& other) = default;
On 2016/10/19 14:45:25, peah-webrtc wrote:
> I don't think you need this one. My guess is that it is only needed because
you
> initialize the variable using
> CircularBuffer render_buffer_ = CircularBuffer(kRenderBufferSize)
> 
> If you instead initialize it as CircularBuffer
render_buffer_(kRenderBufferSize)
> it should be fine without this one. (you may have to do it in the initializer
> list though).

Done.

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:23: void
CircularBuffer::Push(float value) {
On 2016/10/19 14:45:25, peah-webrtc wrote:
> For me it is very strange to have a circular buffer with a push and pop
> function. To me that functionality specifies a stack where the pushed element
> goes on top, and the element popped is the most recent element pushed. I think
> the code would benefit from another naming of this.

This class behaves like a queue, except that it has a maximum size. For queues
the push/pop naming is pretty common, see for example the c++ standard library:
http://www.cplusplus.com/reference/queue/queue/ 
So I think the names are appropriate here.

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:47:
std::vector<float> render_power_ = std::vector<float>(kLookbackFrames);
On 2016/10/19 14:45:25, peah-webrtc wrote:
> Can't you just write std::vector<float> render_power_(kLookbackFrames) ? (you
> may have to do it in the initializer list though).

Ok, I'll do that in the initializer list.

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:59:
MeanVarianceEstimator capture_variance_;
On 2016/10/19 14:45:25, peah-webrtc wrote:
> Since you use render_variance_ and capture_variance_ both for computing the
mean
> and variances, these variable names are misleading I think. What about the
names
> render_statistics_ and capture_statistics_?

Done.

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/mean_variance_estimator.cc
(right):

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/mean_variance_estimator.cc:19:
void MeanVarianceEstimator::UpdateEstimate(float value) {
On 2016/10/19 14:45:25, peah-webrtc wrote:
> This should be UpdateEstimates since it updates several estimates. However,
> since you anyway have the name Estimator in the class name. What about
changing
> the name to "Update" ?

Sounds good, I also renamed the corresponding function in
NormalizedCovarianceEstimator to Update.

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/mean_variance_estimator.h
(right):

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/mean_variance_estimator.h:26:
static constexpr float kAlpha = 0.01f;
On 2016/10/19 14:45:25, peah-webrtc wrote:
> I think it is better to move this into the .cc file instead as it is not used
in
> the .h file. This makes the code easier to read as the value constant is
closer
> to, and in the same files as, where it is used.

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.h (right):

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/circular_buffer.h:29: size_t
buffer_size() { return buffer_size_; }
On 2016/10/19 14:45:25, peah-webrtc wrote:
> I think you could rename buffer_size() to Size(). The class name states it is
a
> buffer, and the Size term should not be ambiguous.

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:37: if
(minimum_render_buffer_size_.GetMinimum() > 0 &&
On 2016/10/19 14:45:26, peah-webrtc wrote:
> Do you need a sliding window buffer for this?
> You basically want to know whether the render_buffer_.buffer_size() has been
> zero during a time period T, right? That should be sufficient by using a
counter
> that is reset to 0 each time buffer_size() is zero. If the counter reaches T,
> then clockdrift can be flagged. If it works, that solution is cheaper and
> requires less code.

That's a good idea. My solution is a bit too general here it seems, I was also
considering the case where there could be multiple excess render buffer calls
within one monitoring period, but I guess that's actually a pretty unrealistic
amount of clock drift and that shouldn't happen in practice. 

I'll use your solution instead (too bad, I kind of liked making this sliding
window class :-))

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:48: void
EchoDetector::Process(rtc::ArrayView<const float> nearend) {
On 2016/10/19 14:45:26, peah-webrtc wrote:
> I think you should use a name with something of capture rather than nearend.
> That matches better with the name render used in BufferRender

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:49: const float
capture_power = Power(nearend);
On 2016/10/19 14:45:26, peah-webrtc wrote:
> Either more this variable closer to where it is first used, or use the output
of
> Power directly in the UpdateEstimate call.

It is used in two places, to update the statistics and inside the for-loop on
line 78 (in this version). I'll move it closer to the update function.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:50: const float
capture_mean = capture_variance_.mean();
On 2016/10/19 14:45:26, peah-webrtc wrote:
> Move capture_mean closer to where it is used. Or is there a reason for why the
> value before the updated mean from the last frame is used?

That does seem to be a bug, I fixed it now, thanks.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:51: const float
capture_std_deviation = capture_variance_.std_deviation();
On 2016/10/19 14:45:26, peah-webrtc wrote:
> Same comment as for capture_mean.

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:53: const
rtc::Optional<float> buffered_render_power = render_buffer_.Pop();
On 2016/10/19 14:45:25, peah-webrtc wrote:
> I think the function  would benefit in readability if you separate the
different
> parts with blank lines.

I added a few blank lines and some comments.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:57: LOG(LS_ERROR)
<< "Clockdrift detected in residual echo detector. Ignoring "
On 2016/10/19 14:45:26, peah-webrtc wrote:
> This will flood the log with messages if Capture is started before Render. 

Good point, but isn't it typically the render side that starts first? Or can
both cases happen?

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:61: const float
latest_render_power = *buffered_render_power;
On 2016/10/19 14:45:26, peah-webrtc wrote:
> I think you can skip this local copy. Use *buffered_render_power directly
> instead.

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:74: const float
render_power = render_power_[read_index];
On 2016/10/19 14:45:25, peah-webrtc wrote:
> I think you can skip storing the buffer values in local variables before the
> function call. Make the function call with the values
render_power_[read_index],
> render_power_mean_[read_index] and render_power_std_dev_[read_index] instead.
I
> think the code will be easier to read that way.

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:49:
SlidingWindowMinimum(kRenderBufferSize);
On 2016/10/19 14:45:26, peah-webrtc wrote:
> Call constructor immediately to avoid the copy.

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:58:
std::vector<NormalizedCovarianceEstimator> covariance_ =
On 2016/10/19 14:45:26, peah-webrtc wrote:
> Since the vector store several covariance estimates, what about naming it
> covariances_ ?

Done.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/echo_detector/normalized_covariance_estimator.cc
(right):

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/normalized_covariance_estimator.cc:15:
void NormalizedCovarianceEstimator::UpdateCovarianceEstimate(float x,
On 2016/10/19 14:45:26, peah-webrtc wrote:
> What about just calling the method Update? The class name allready states that
> it is a covariance estimate that is computed.

Done.

peah-webrtc

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_processing/echo_detector/circular_buffer.cc File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right): https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_processing/echo_detector/circular_buffer.cc#newcode23 webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:23: void CircularBuffer::Push(float value) { On 2016/10/20 14:04:35, ivoc wrote: ...

4 years, 2 months ago (2016-10-20 15:08:24 UTC) #18

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right):

https://codereview.webrtc.org/2419563003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:23: void
CircularBuffer::Push(float value) {
On 2016/10/20 14:04:35, ivoc wrote:
> On 2016/10/19 14:45:25, peah-webrtc wrote:
> > For me it is very strange to have a circular buffer with a push and pop
> > function. To me that functionality specifies a stack where the pushed
element
> > goes on top, and the element popped is the most recent element pushed. I
think
> > the code would benefit from another naming of this.
> 
> This class behaves like a queue, except that it has a maximum size. For queues
> the push/pop naming is pretty common, see for example the c++ standard
library:
> http://www.cplusplus.com/reference/queue/queue/ 
> So I think the names are appropriate here.

I see. Good point.

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:57: LOG(LS_ERROR)
<< "Clockdrift detected in residual echo detector. Ignoring "
On 2016/10/20 14:04:35, ivoc wrote:
> On 2016/10/19 14:45:26, peah-webrtc wrote:
> > This will flood the log with messages if Capture is started before Render. 
> 
> Good point, but isn't it typically the render side that starts first? Or can
> both cases happen?

I don't think we can make any assumptions on that. I at least have no idea.

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:44: if
(render_buffer_zero_ >= kRenderBufferSize) {
you can add an else if here, as the latter if-statement will never be true if
the first statement is true.

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:46: << "a render
frame.";
Please add to the comment which frame you are ignoring, "the latest" or "the
most recent"

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:66: // This
should only happen in case of clock drift. For now we will just
Please update the comment to include that it can also happen initially, or when
there is a glitch, or alternatively change the comment so that it includes that
it could happen otherwise as well, but not in a as a normal case.

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:75: // Update the
render statistics, and store the .
The comment seems to have been ended in the middle.

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:100:
echo_likelihood_ = std::max(
Do you take any account of the reliability on the likelihood. I.e., in the
initial phase ,e.g., the first frame, it should not be reliable. The same
probably holds for when there has been clock-drift.

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:38: // TODO(ivoc):
Verify the size of this buffer.
Do these two constants need to be in the header file?

https://codereview.webrtc.org/2419563003/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:48: size_t
render_buffer_zero_ = 0;
I think this variable would benefit of being more specifically named, such as
frames_since_zero_buffer_length_
or zero_length_buffer_frame_counter_

As it is now, the name is not self-explanatory and you need the comment in order
to tell what the variable stores.

ivoc

https://codereview.chromium.org/2419563003/diff/100001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.chromium.org/2419563003/diff/100001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode57 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:57: LOG(LS_ERROR) << "Clockdrift detected in residual echo detector. Ignoring ...

4 years, 2 months ago (2016-10-21 12:21:15 UTC) #19

https://codereview.chromium.org/2419563003/diff/100001/webrtc/modules/audio_p...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.chromium.org/2419563003/diff/100001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:57: LOG(LS_ERROR)
<< "Clockdrift detected in residual echo detector. Ignoring "
On 2016/10/20 15:08:24, peah-webrtc wrote:
> On 2016/10/20 14:04:35, ivoc wrote:
> > On 2016/10/19 14:45:26, peah-webrtc wrote:
> > > This will flood the log with messages if Capture is started before Render.

> > 
> > Good point, but isn't it typically the render side that starts first? Or can
> > both cases happen?
> 
> I don't think we can make any assumptions on that. I at least have no idea.

I removed the log messages for now to avoid this. I will add how often this
happens to the logging in a follow-up CL.

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:44: if
(render_buffer_zero_ >= kRenderBufferSize) {
On 2016/10/20 15:08:24, peah-webrtc wrote:
> you can add an else if here, as the latter if-statement will never be true if
> the first statement is true.

Done.

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:46: << "a render
frame.";
On 2016/10/20 15:08:24, peah-webrtc wrote:
> Please add to the comment which frame you are ignoring, "the latest" or "the
> most recent"

It is actually the oldest frame in the buffer that is being discarded, since the
render buffer is a FIFO queue. I will add it to the log message.

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:66: // This
should only happen in case of clock drift. For now we will just
On 2016/10/20 15:08:24, peah-webrtc wrote:
> Please update the comment to include that it can also happen initially, or
when
> there is a glitch, or alternatively change the comment so that it includes
that
> it could happen otherwise as well, but not in a as a normal case.

Done.

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:75: // Update the
render statistics, and store the .
On 2016/10/20 15:08:24, peah-webrtc wrote:
> The comment seems to have been ended in the middle.

Oh, that was sloppy, I fixed it.

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:100:
echo_likelihood_ = std::max(
On 2016/10/20 15:08:24, peah-webrtc wrote:
> Do you take any account of the reliability on the likelihood. I.e., in the
> initial phase ,e.g., the first frame, it should not be reliable. The same
> probably holds for when there has been clock-drift.
> 

That's an interesting idea. I think one additional factor that could indicate
reduced reliability is when both standard deviations are very low. Since the
covariance is divided by the product of the these standard deviations, in cases
where they're both very low, we're providing a substantial gain. I think this
can be caused in cases where both sides are near silent. What do you think is
the best way to keep track of the reliability? Should it be another adaptively
estimated variable that starts at zero and is gradually increased/reduced
depending on things like clock drift, standard deviations etc?

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right):

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:38: // TODO(ivoc):
Verify the size of this buffer.
On 2016/10/20 15:08:24, peah-webrtc wrote:
> Do these two constants need to be in the header file?

Not anymore, I will move them to the cc file.

https://codereview.chromium.org/2419563003/diff/120001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.h:48: size_t
render_buffer_zero_ = 0;
On 2016/10/20 15:08:24, peah-webrtc wrote:
> I think this variable would benefit of being more specifically named, such as
> frames_since_zero_buffer_length_
> or zero_length_buffer_frame_counter_
> 
> As it is now, the name is not self-explanatory and you need the comment in
order
> to tell what the variable stores.

I changed it to frames_since_zero_buffer_size_.

ivoc

https://codereview.chromium.org/2419563003/diff/100001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.chromium.org/2419563003/diff/100001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode57 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:57: LOG(LS_ERROR) << "Clockdrift detected in residual echo detector. Ignoring ...

4 years, 2 months ago (2016-10-21 15:42:08 UTC) #20

hlundin-webrtc

Looks good. A few questions only. https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode47 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47: frames_since_zero_buffer_size_ = 0; ...

4 years, 2 months ago (2016-10-24 00:42:14 UTC) #21

ivoc

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode47 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47: frames_since_zero_buffer_size_ = 0; On 2016/10/24 00:42:14, hlundin-webrtc wrote: > ...

4 years, 1 month ago (2016-10-24 15:25:19 UTC) #22

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_p...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47:
frames_since_zero_buffer_size_ = 0;
On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> I don't understand the logic of frames_since_zero_buffer_size_. No matter
what,
> it will always come out as 1 when this method returns, and it is neither read
> nor written anywhere else. Confused.

The idea is to count how long ago it was since the buffer was zero. The
reasoning is that in the case of clockdrift, this buffer will never return to
zero (it will slowly fill up), so we can use this as a signal to detect that. 

I don't think it will come out as 1 no matter what: if
frames_since_zero_buffer_size_ is < kRenderBufferSize and the buffer is not
zero, neither the if nor the else-if will be run, and it will simply be
incremented below.

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_p...
File webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc
(right):

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:70: for
(int i = 0; i < 1000; i++) {
On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> What if you ran this for much longer? Would it still pass? Probably not a good
> idea for the committed test, but you can test it locally.

I tried this with 100000 loop iterations locally, and the test still passes with
the same likelihood (but it takes 5.7s, which indicates that the RED takes about
57ns per iteration in debug mode on my machine).

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:87:
EXPECT_GT(echo_detector.echo_likelihood(), 0.8f);
On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> Out of curiosity, why does this test flag echo with "only" 80% certainty,
while
> the one below is 99%?

Clock drift is harder to correct for on the render side, this due to the fact
that calls to BufferRender() and Process() can be jittery, so they don't
necessarily alternate nicely. To deal with this, the BufferRender() function
stores the value in a queue, and processing is only done on the Process()
function, which takes a single value from the queue. For clockdrift on the other
side, the situation is simple, because the render buffer will be empty when
Process() is called too often, so we know immediately that this is clockdrift
and we can just ignore the excess call. The problem on the render side is that
excess calls can be either due to jitter (in which case it should just be
buffered) or due to clockdrift (in which case we want to discard it). To deal
with this we now count how long it has been until the render buffer was empty,
it should be empty within a reasonable amount of time, otherwise it indicates
clockdrift. This means that there is a larger delay before clockdrift is
detected and corrected on this side than on the other side, which leads to worse
performance.

I guess this could be corrected for by introducing a sufficiently long delay on
both sides, but I'm not sure if that's desirable.

https://codereview.chromium.org/2419563003/diff/140001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:99: for
(int i = 0; i < 1000; i++) {
On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> Same question as above about much longer test.

Also tried with 100000 iterations, same result.

hlundin-webrtc

LGTM. https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode47 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47: frames_since_zero_buffer_size_ = 0; On 2016/10/24 15:25:19, ivoc wrote: ...

4 years, 1 month ago (2016-10-27 13:05:48 UTC) #23

LGTM.

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47:
frames_since_zero_buffer_size_ = 0;
On 2016/10/24 15:25:19, ivoc wrote:
> On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> > I don't understand the logic of frames_since_zero_buffer_size_. No matter
> what,
> > it will always come out as 1 when this method returns, and it is neither
read
> > nor written anywhere else. Confused.
> 
> The idea is to count how long ago it was since the buffer was zero. The
> reasoning is that in the case of clockdrift, this buffer will never return to
> zero (it will slowly fill up), so we can use this as a signal to detect that. 
> 
> I don't think it will come out as 1 no matter what: if
> frames_since_zero_buffer_size_ is < kRenderBufferSize and the buffer is not
> zero, neither the if nor the else-if will be run, and it will simply be
> incremented below.

Oh, it was an if--elseif statement. I read it as if-else. Mea culpa.

You may want to reconsider the name of the state variable, or comment more in
detail what it is actually counting.

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:70: for
(int i = 0; i < 1000; i++) {
On 2016/10/24 15:25:19, ivoc wrote:
> On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> > What if you ran this for much longer? Would it still pass? Probably not a
good
> > idea for the committed test, but you can test it locally.
> 
> I tried this with 100000 loop iterations locally, and the test still passes
with
> the same likelihood (but it takes 5.7s, which indicates that the RED takes
about
> 57ns per iteration in debug mode on my machine).

Acknowledged.

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:87:
EXPECT_GT(echo_detector.echo_likelihood(), 0.8f);
On 2016/10/24 15:25:19, ivoc wrote:
> On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> > Out of curiosity, why does this test flag echo with "only" 80% certainty,
> while
> > the one below is 99%?
> 
> Clock drift is harder to correct for on the render side, this due to the fact
> that calls to BufferRender() and Process() can be jittery, so they don't
> necessarily alternate nicely. To deal with this, the BufferRender() function
> stores the value in a queue, and processing is only done on the Process()
> function, which takes a single value from the queue. For clockdrift on the
other
> side, the situation is simple, because the render buffer will be empty when
> Process() is called too often, so we know immediately that this is clockdrift
> and we can just ignore the excess call. The problem on the render side is that
> excess calls can be either due to jitter (in which case it should just be
> buffered) or due to clockdrift (in which case we want to discard it). To deal
> with this we now count how long it has been until the render buffer was empty,
> it should be empty within a reasonable amount of time, otherwise it indicates
> clockdrift. This means that there is a larger delay before clockdrift is
> detected and corrected on this side than on the other side, which leads to
worse
> performance.
> 
> I guess this could be corrected for by introducing a sufficiently long delay
on
> both sides, but I'm not sure if that's desirable.

OK. Sounds good. You may want to put some of this information (preferably
condensed) into a comment here.

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:99: for
(int i = 0; i < 1000; i++) {
On 2016/10/24 15:25:19, ivoc wrote:
> On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> > Same question as above about much longer test.
> 
> Also tried with 100000 iterations, same result.

Acknowledged.

ivoc

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_processing/echo_detector/echo_detector.cc#newcode47 webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47: frames_since_zero_buffer_size_ = 0; On 2016/10/27 13:05:48, hlundin-webrtc wrote: > ...

4 years, 1 month ago (2016-10-27 13:55:29 UTC) #24

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right):

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector.cc:47:
frames_since_zero_buffer_size_ = 0;
On 2016/10/27 13:05:48, hlundin-webrtc wrote:
> On 2016/10/24 15:25:19, ivoc wrote:
> > On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> > > I don't understand the logic of frames_since_zero_buffer_size_. No matter
> > what,
> > > it will always come out as 1 when this method returns, and it is neither
> read
> > > nor written anywhere else. Confused.
> > 
> > The idea is to count how long ago it was since the buffer was zero. The
> > reasoning is that in the case of clockdrift, this buffer will never return
to
> > zero (it will slowly fill up), so we can use this as a signal to detect
that. 
> > 
> > I don't think it will come out as 1 no matter what: if
> > frames_since_zero_buffer_size_ is < kRenderBufferSize and the buffer is not
> > zero, neither the if nor the else-if will be run, and it will simply be
> > incremented below.
> 
> Oh, it was an if--elseif statement. I read it as if-else. Mea culpa.
> 
> You may want to reconsider the name of the state variable, or comment more in
> detail what it is actually counting.

Added more comments in the header, because naming stuff is hard.

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc
(right):

https://codereview.webrtc.org/2419563003/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/echo_detector/echo_detector_unittest.cc:87:
EXPECT_GT(echo_detector.echo_likelihood(), 0.8f);
On 2016/10/27 13:05:48, hlundin-webrtc wrote:
> On 2016/10/24 15:25:19, ivoc wrote:
> > On 2016/10/24 00:42:14, hlundin-webrtc wrote:
> > > Out of curiosity, why does this test flag echo with "only" 80% certainty,
> > while
> > > the one below is 99%?
> > 
> > Clock drift is harder to correct for on the render side, this due to the
fact
> > that calls to BufferRender() and Process() can be jittery, so they don't
> > necessarily alternate nicely. To deal with this, the BufferRender() function
> > stores the value in a queue, and processing is only done on the Process()
> > function, which takes a single value from the queue. For clockdrift on the
> other
> > side, the situation is simple, because the render buffer will be empty when
> > Process() is called too often, so we know immediately that this is
clockdrift
> > and we can just ignore the excess call. The problem on the render side is
that
> > excess calls can be either due to jitter (in which case it should just be
> > buffered) or due to clockdrift (in which case we want to discard it). To
deal
> > with this we now count how long it has been until the render buffer was
empty,
> > it should be empty within a reasonable amount of time, otherwise it
indicates
> > clockdrift. This means that there is a larger delay before clockdrift is
> > detected and corrected on this side than on the other side, which leads to
> worse
> > performance.
> > 
> > I guess this could be corrected for by introducing a sufficiently long delay
> on
> > both sides, but I'm not sure if that's desirable.
> 
> OK. Sounds good. You may want to put some of this information (preferably
> condensed) into a comment here.

Done.

peah-webrtc

lgtm with one suggestion https://codereview.webrtc.org/2419563003/diff/160001/webrtc/modules/audio_processing/echo_detector/circular_buffer.cc File webrtc/modules/audio_processing/echo_detector/circular_buffer.cc (right): https://codereview.webrtc.org/2419563003/diff/160001/webrtc/modules/audio_processing/echo_detector/circular_buffer.cc#newcode28 webrtc/modules/audio_processing/echo_detector/circular_buffer.cc:28: RTC_DCHECK_LE(buffer_size_, buffer_.size()); It is quite ...

4 years, 1 month ago (2016-10-27 14:27:09 UTC) #25

ivoc

The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, peah@webrtc.org Link to ...

4 years, 1 month ago (2016-10-28 13:35:07 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2419563003/180001

4 years, 1 month ago (2016-10-28 13:35:15 UTC) #29

commit-bot: I haz the power

Description was changed from ========== Add algorithm for Residual Echo Detector. This algorithm calculates an ...

4 years, 1 month ago (2016-10-28 14:04:05 UTC) #30

commit-bot: I haz the power

Description was changed from ========== Add algorithm for Residual Echo Detector. This algorithm calculates an ...

4 years, 1 month ago (2016-10-28 14:04:15 UTC) #32

commit-bot: I haz the power

4 years, 1 month ago (2016-10-28 14:04:17 UTC) #33

Message was sent while issue was closed.

Patchset 10 (id:??) landed as
https://crrev.com/af27ed01d73eef0622869ecf9f0b6dbf8b80bd14
Cr-Commit-Position: refs/heads/master@{#14824}

Issue 2419563003: Add algorithm for Residual Echo Detector. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Review comments. #

Patch Set 3 : Added unittests and some small changes. #

Patch Set 4 : Addressed Henrik's comments. #

Patch Set 5 : More comments from Henrik. #

Patch Set 6 : Added better handling of clock drift on render side. #

Patch Set 7 : Per's comments. #

Patch Set 8 : More comments by Per. #

Patch Set 9 : Added comments. #

Patch Set 10 : Merged residual_echo_detector and echo_detector. #

Messages