Added a new echo likelihood stat that reports the maximum value from a previous time period.

Added a new echo likelihood stat that reports the maximum value from a previous time period.

BUG=webrtc:6797
Review-Url: https://codereview.webrtc.org/2629563003
Cr-Commit-Position: refs/heads/master@{#16079}
Committed: https://chromium.googlesource.com/external/webrtc/+/4e477a1d7b419bfb4b4a4887641ab9ab2ad825c2

[ivoc, 2017-01-12 16:47:40]

Description was changed from

==========
Added a new echo likelihood stat that reports the maximum value from a previous
time period.

BUG=webrtc:6797
==========

to

==========
Added a new echo likelihood stat that reports the maximum value from a previous
time period.

BUG=webrtc:6797
==========

[ivoc, 2017-01-12 16:47:40]

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

[ivoc, 2017-01-12 16:55:29]

Please have a look at this CL that adds a new echo likelihood metric to GetStats
(the old one). This new stat is the maximum echo likelihood value from the last
10 seconds, and is intended to make it easier not to miss detected echo when
polling GetStats infrequently. It makes sense to make this time period
configurable in a follow-up CL.

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1588:
AudioProcessing::AudioProcessingStatistics::AudioProcessingStatistics() {
It was needed to move these to the cc file because the class is now too complex.

[hlundin-webrtc, 2017-01-13 09:06:22]

Looks good. A few comments.

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1588:
AudioProcessing::AudioProcessingStatistics::AudioProcessingStatistics() {
On 2017/01/12 16:55:29, ivoc wrote:
> It was needed to move these to the cc file because the class is now too
complex.

Acknowledged.

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:32: float
LikelihoodAggregator::max() const {
I think this should be named Max() to reflect that it is not "as cheap as a
getter".

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:2: * 
Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
2017

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:22:
explicit LikelihoodAggregator(size_t window_size);
What is the unit of the window size? Please, append the unit with the variable
name.

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:37: int
insertion_index_ = 0;
size_t

[ivoc, 2017-01-13 12:12:29]

Thanks for the comments! I also made the max operation more efficient in this
update. It should now be amortized O(1) for the Update function, and O(1) for
the Max() operation.

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:32: float
LikelihoodAggregator::max() const {
On 2017/01/13 09:06:22, hlundin-webrtc wrote:
> I think this should be named Max() to reflect that it is not "as cheap as a
> getter".

Done.

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:2: * 
Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
On 2017/01/13 09:06:22, hlundin-webrtc wrote:
> 2017

Oops, good point.

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:22:
explicit LikelihoodAggregator(size_t window_size);
On 2017/01/13 09:06:22, hlundin-webrtc wrote:
> What is the unit of the window size? Please, append the unit with the variable
> name.

There's one likelihood value per frame (10 ms of audio). I changed this to
window_size_frames.

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:37: int
insertion_index_ = 0;
On 2017/01/13 09:06:22, hlundin-webrtc wrote:
> size_t

Done.

[the sun, 2017-01-13 12:15:58]

The plumbing l-g-t-m. A few comments on the aggregator class. Not sure whether
adding the stats string is the right thing to do but I'm sure hbos@ can give
input on that.

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:20: class
LikelihoodAggregator {
Rename to e.g. MovingMax or SlidingMax to
1. Avoid the need for the above comment.
2. Indicate what it actually does
3. ...which may allow it to be reused in other contexts
because really, the class has nothing to do with echo likelihood values - it
just keeps track of the max among the last in a series of floats...

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:22:
explicit LikelihoodAggregator(size_t window_size);
On 2017/01/13 09:06:22, hlundin-webrtc wrote:
> What is the unit of the window size? Please, append the unit with the variable
> name.

How does that matter here?

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:27: void
Update(float value);
Update() to me sounds like it would do some computation, but the computation is
actually done in max(). How about renaming this to Add() (which would make part
of the comment unnecessary) and max() to GetMax() or CalcMax()?

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:35:
std::vector<float> recent_values_;
Given that the size is 1000 elements in this use case, I think you've made the
right call simply searching through the vector when the max is retrieved
(insertion is what needs to be efficient).

[peah-webrtc, 2017-01-13 12:49:01]

peah@webrtc.org changed reviewers:
+ peah@webrtc.org

[peah-webrtc, 2017-01-13 12:49:02]

Some quick drive-by comments.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:20: :
recent_values_(window_size_frames) {
I think that if you initialize the maximum_candidates_ to window_size_frames,
that would ensure that there is no reallocation in the deque, right?

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:30: while
(!maximum_candidates_.empty() &&
In it's current use in the residual echo detector,  window_size_frames is 1000,
right? 

Afaics, the worst case performance of this loop is that it can consist of
looping through window_size_frames values, so even though this way of finding
the maximum is more efficient on average data, it can be quite complex as this
happens for each frame, right?

Would it be possible to do this in a more efficient way? Considering the above,
as the Max() operation is called less frequently, wouldn't it be better to do a
linear search each time Max() is called?

An alternative is to have a leaking maximum statistics implementation such as 

if (value > max_value_) {
 max_value_ = value;
 counter = 0;
}

if (counter_ > 1000) {
 max_value_ *= 0.99f;
}
else {
 ++counter;
}

That solution is supercheap, and even though approximate, it is probably still
accurate enough as the value it measures is as well averaged over a long data
segment.

WDYT?

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:39:
maximum_candidates_.push_back(insertion_index_);
This is potentially an expensive operation as it may cause a reallocation. Would
it be possible to do it in a way that ensures that no reallocation is performed?

[peah-webrtc, 2017-01-13 12:49:15]

peah@webrtc.org changed reviewers:
- peah@webrtc.org

[ivoc, 2017-01-13 13:24:23]

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:20: class
LikelihoodAggregator {
On 2017/01/13 12:15:57, the sun wrote:
> Rename to e.g. MovingMax or SlidingMax to
> 1. Avoid the need for the above comment.
> 2. Indicate what it actually does
> 3. ...which may allow it to be reused in other contexts
> because really, the class has nothing to do with echo likelihood values - it
> just keeps track of the max among the last in a series of floats...

Great suggestion, done.

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:27: void
Update(float value);
On 2017/01/13 12:15:57, the sun wrote:
> Update() to me sounds like it would do some computation, but the computation
is
> actually done in max(). How about renaming this to Add() (which would make
part
> of the comment unnecessary) and max() to GetMax() or CalcMax()?

In the new implementation it actually does a bit more computation (although
still amortized O(1)), is it okay to keep the name as is?

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:35:
std::vector<float> recent_values_;
On 2017/01/13 12:15:57, the sun wrote:
> Given that the size is 1000 elements in this use case, I think you've made the
> right call simply searching through the vector when the max is retrieved
> (insertion is what needs to be efficient).

I initially thought so too, but it has now come to my attention that some apps
poll GetStats() rather frequently (roughly once per 100 ms), which would mean
that the Max() function should be efficient as well.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:20: :
recent_values_(window_size_frames) {
On 2017/01/13 12:49:02, peah-webrtc wrote:
> I think that if you initialize the maximum_candidates_ to window_size_frames,
> that would ensure that there is no reallocation in the deque, right?

I don't think that's entirely true. The values in the deque can drift from their
starting position in memory. If you consider the case where the likelihood
values are a decreasing sequence, the new values will constantly be pushed to
the back of the deque, while the old values will get removed from the front.
This effect can cause allocation of new blocks to be necessary.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:30: while
(!maximum_candidates_.empty() &&
On 2017/01/13 12:49:02, peah-webrtc wrote:
> In it's current use in the residual echo detector,  window_size_frames is
1000,
> right? 
> 
> Afaics, the worst case performance of this loop is that it can consist of
> looping through window_size_frames values, so even though this way of finding
> the maximum is more efficient on average data, it can be quite complex as this
> happens for each frame, right?
> 
> Would it be possible to do this in a more efficient way? Considering the
above,
> as the Max() operation is called less frequently, wouldn't it be better to do
a
> linear search each time Max() is called?
> 
> An alternative is to have a leaking maximum statistics implementation such as 
> 
> if (value > max_value_) {
>  max_value_ = value;
>  counter = 0;
> }
> 
> if (counter_ > 1000) {
>  max_value_ *= 0.99f;
> }
> else {
>  ++counter;
> }
> 
> That solution is supercheap, and even though approximate, it is probably still
> accurate enough as the value it measures is as well averaged over a long data
> segment.
> 
> WDYT?

You're right that when considering a single call to this function, it can loop
through all elements (O(N)), which is indeed pretty slow (this can only happen
once every N calls though). This function is pretty fast on average, but
sometimes it can be slow. 

Your solution looks like a better idea, thanks for the suggestion, I'll rework
the class to use this implementation.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:39:
maximum_candidates_.push_back(insertion_index_);
On 2017/01/13 12:49:02, peah-webrtc wrote:
> This is potentially an expensive operation as it may cause a reallocation.
Would
> it be possible to do it in a way that ensures that no reallocation is
performed?

In a deque it should not cause reallocation (so nothing has to be moved to a new
memory location), but it can cause allocation of a new block (a deque is stored
in multiple array-like blocks in memory). It may be possible to store
maximum_candidates_ in a ring-buffer as well, which would avoid any memory
operations.

[peah-webrtc, 2017-01-13 13:37:33]

peah@webrtc.org changed reviewers:
+ peah@webrtc.org

[peah-webrtc, 2017-01-13 13:37:34]

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:20: :
recent_values_(window_size_frames) {
On 2017/01/13 13:24:23, ivoc wrote:
> On 2017/01/13 12:49:02, peah-webrtc wrote:
> > I think that if you initialize the maximum_candidates_ to
window_size_frames,
> > that would ensure that there is no reallocation in the deque, right?
> 
> I don't think that's entirely true. The values in the deque can drift from
their
> starting position in memory. If you consider the case where the likelihood
> values are a decreasing sequence, the new values will constantly be pushed to
> the back of the deque, while the old values will get removed from the front.
> This effect can cause allocation of new blocks to be necessary.

Good point! I did not know that.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:30: while
(!maximum_candidates_.empty() &&
On 2017/01/13 13:24:23, ivoc wrote:
> On 2017/01/13 12:49:02, peah-webrtc wrote:
> > In it's current use in the residual echo detector,  window_size_frames is
> 1000,
> > right? 
> > 
> > Afaics, the worst case performance of this loop is that it can consist of
> > looping through window_size_frames values, so even though this way of
finding
> > the maximum is more efficient on average data, it can be quite complex as
this
> > happens for each frame, right?
> > 
> > Would it be possible to do this in a more efficient way? Considering the
> above,
> > as the Max() operation is called less frequently, wouldn't it be better to
do
> a
> > linear search each time Max() is called?
> > 
> > An alternative is to have a leaking maximum statistics implementation such
as 
> > 
> > if (value > max_value_) {
> >  max_value_ = value;
> >  counter = 0;
> > }
> > 
> > if (counter_ > 1000) {
> >  max_value_ *= 0.99f;
> > }
> > else {
> >  ++counter;
> > }
> > 
> > That solution is supercheap, and even though approximate, it is probably
still
> > accurate enough as the value it measures is as well averaged over a long
data
> > segment.
> > 
> > WDYT?
> 
> You're right that when considering a single call to this function, it can loop
> through all elements (O(N)), which is indeed pretty slow (this can only happen
> once every N calls though). This function is pretty fast on average, but
> sometimes it can be slow. 
> 
> Your solution looks like a better idea, thanks for the suggestion, I'll rework
> the class to use this implementation.

Acknowledged.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:39:
maximum_candidates_.push_back(insertion_index_);
On 2017/01/13 13:24:23, ivoc wrote:
> On 2017/01/13 12:49:02, peah-webrtc wrote:
> > This is potentially an expensive operation as it may cause a reallocation.
> Would
> > it be possible to do it in a way that ensures that no reallocation is
> performed?
> 
> In a deque it should not cause reallocation (so nothing has to be moved to a
new
> memory location), but it can cause allocation of a new block (a deque is
stored
> in multiple array-like blocks in memory). It may be possible to store
> maximum_candidates_ in a ring-buffer as well, which would avoid any memory
> operations.

Ah, I did not know that the deque was stored like that. Yes, using a ring-buffer
for that sounds like a good suggestion.

[peah-webrtc, 2017-01-13 13:37:43]

peah@webrtc.org changed reviewers:
- peah@webrtc.org

[hlundin-webrtc, 2017-01-13 13:38:32]

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

https://codereview.webrtc.org/2629563003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:22:
explicit LikelihoodAggregator(size_t window_size);
On 2017/01/13 12:15:57, the sun wrote:
> On 2017/01/13 09:06:22, hlundin-webrtc wrote:
> > What is the unit of the window size? Please, append the unit with the
variable
> > name.
> 
> How does that matter here?

Maybe it doesn't. I was biased by our offline discussions about window lengths
in seconds. Now that we are looking at samples, I agree that it is less
relevant.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:45: float
LikelihoodAggregator::Max() const {
Now it is very cheap again, so max() would be ok. Up to you...

[the sun, 2017-01-13 13:55:28]

Caveat: I've never used the C++ priority_queue, only my own heap impls.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:38:
std::deque<size_t> maximum_candidates_;
I think a priority_queue may serve you better - then you'd have 2*log2 behavior
in Update() instead of the current pushing/popping.

[ivoc, 2017-01-13 14:40:36]

I rewrote the class based on Per's suggestion. It's now a lot simpler and more
efficient, and it should be good enough for our purposes.

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.cc:45: float
LikelihoodAggregator::Max() const {
On 2017/01/13 13:38:32, hlundin-webrtc wrote:
> Now it is very cheap again, so max() would be ok. Up to you...

I lowercased it again :-)

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h
(right):

https://codereview.webrtc.org/2629563003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/likelihood_aggregator.h:38:
std::deque<size_t> maximum_candidates_;
On 2017/01/13 13:55:28, the sun wrote:
> I think a priority_queue may serve you better - then you'd have 2*log2
behavior
> in Update() instead of the current pushing/popping.

That sounds like a good suggestion, but I rewrote this class to Per's
suggestion, which is a much cheaper algorithm (although it doesn't produce exact
results).

[the sun, 2017-01-13 14:51:23]

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

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:13: #include
<algorithm>
Not needed

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:27: max_value_ *=
0.99f;
Pull the constant out into a constexpr and add rationale or function used to
compute it.

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:33: ++counter_;
Counter may wrap. If you really need the peak hold, don't increment once you've
reached window_size_.

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

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.h:24: // Add a new
likelihood to the buffer. This will automatically remove the
Comments need updating. Since it's a generic class, skip the talk about a
likelihood...

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.h:35: size_t
window_size_;
It's a good idea to always init everything, regardless if you also set it in the
ctor

[ivoc, 2017-01-13 15:08:42]

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

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:13: #include
<algorithm>
On 2017/01/13 14:51:23, the sun wrote:
> Not needed

Good point, removed.

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:27: max_value_ *=
0.99f;
On 2017/01/13 14:51:23, the sun wrote:
> Pull the constant out into a constexpr and add rationale or function used to
> compute it.

Done. This made me rethink the value and realize that it's too high.

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:33: ++counter_;
On 2017/01/13 14:51:23, the sun wrote:
> Counter may wrap. If you really need the peak hold, don't increment once
you've
> reached window_size_.

Done.

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

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.h:24: // Add a new
likelihood to the buffer. This will automatically remove the
On 2017/01/13 14:51:23, the sun wrote:
> Comments need updating. Since it's a generic class, skip the talk about a
> likelihood...

I removed the comments for Update() and max(), since if I remove the talk about
likelihoods there isn't anything important left :)

https://codereview.webrtc.org/2629563003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.h:35: size_t
window_size_;
On 2017/01/13 14:51:23, the sun wrote:
> It's a good idea to always init everything, regardless if you also set it in
the
> ctor

Since zero is an illegal value, I set it to one by default now.

[the sun, 2017-01-13 15:16:02]

lgtm % comments

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

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:34: counter_ = 0;
return;

otherwise you're effectively resetting counter_ to 1 (line 39)

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/residual_echo_detector.h (right):

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/residual_echo_detector.h:84: MovingMax
moving_max_;
How about naming the variable something with likelihood, so code is
self-commenting? "recent_likelihood_max"?

[hlundin-webrtc, 2017-01-13 15:27:47]

lgtm

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

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:20: // decay to 1%
of its former value after 90 updates.
This might be too fast, but I'm not sure.

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

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max_unittest.cc:16: // Test
if the aggregator correctly finds the maximum.
Add a test for the decay too.

[ivoc, 2017-01-13 15:48:52]

Thanks for the quick reviews!

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

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:20: // decay to 1%
of its former value after 90 updates.
On 2017/01/13 15:27:47, hlundin-webrtc wrote:
> This might be too fast, but I'm not sure.

Changed this back to 0.99 per offline discussion.

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:34: counter_ = 0;
On 2017/01/13 15:16:02, the sun wrote:
> return;
> 
> otherwise you're effectively resetting counter_ to 1 (line 39)

I shuffled things around so a return is not needed. However, I did need to
update the check for the counter, since it's value is compared before being
incremented.

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

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max_unittest.cc:16: // Test
if the aggregator correctly finds the maximum.
On 2017/01/13 15:27:47, hlundin-webrtc wrote:
> Add a test for the decay too.

Done.

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/residual_echo_detector.h (right):

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/residual_echo_detector.h:84: MovingMax
moving_max_;
On 2017/01/13 15:16:02, the sun wrote:
> How about naming the variable something with likelihood, so code is
> self-commenting? "recent_likelihood_max"?

Done.

[hlundin-webrtc, 2017-01-13 16:23:43]

lgtm

[the sun, 2017-01-13 21:23:26]

still lgtm

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

https://codereview.webrtc.org/2629563003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/echo_detector/moving_max.cc:34: counter_ = 0;
On 2017/01/13 15:48:52, ivoc wrote:
> On 2017/01/13 15:16:02, the sun wrote:
> > return;
> > 
> > otherwise you're effectively resetting counter_ to 1 (line 39)
> 
> I shuffled things around so a return is not needed. However, I did need to
> update the check for the counter, since it's value is compared before being
> incremented.

Much nicer, but I'm not sure I agree with the (window_size_ - 1)... (nit)

[ivoc, 2017-01-15 13:47:56]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2017-01-15 13:48:04]

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

[commit-bot: I haz the power, 2017-01-15 13:59:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-15 13:59:17]

Try jobs failed on following builders:
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5271)

[ivoc, 2017-01-15 15:41:54]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2017-01-15 15:41:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2629563003/#ps100001 (title:
"Small bugfix.")

[commit-bot: I haz the power, 2017-01-15 15:42:01]

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

[commit-bot: I haz the power, 2017-01-15 16:29:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484494914233300,
"parent_rev": "cc1e1aa42494bb8532d32ef4c5055d1ddcac0d8b", "commit_rev":
"4e477a1d7b419bfb4b4a4887641ab9ab2ad825c2"}

[commit-bot: I haz the power, 2017-01-15 16:29:53]

Description was changed from

==========
Added a new echo likelihood stat that reports the maximum value from a previous
time period.

BUG=webrtc:6797
==========

to

==========
Added a new echo likelihood stat that reports the maximum value from a previous
time period.

BUG=webrtc:6797

Review-Url: https://codereview.webrtc.org/2629563003
Cr-Commit-Position: refs/heads/master@{#16079}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4e477a1d7b419bfb4b4a48876...
==========

[commit-bot: I haz the power, 2017-01-15 16:29:55]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/4e477a1d7b419bfb4b4a48876...

[hbos, 2017-01-16 11:29:49]

I didn't review this because we had an email discussion about adding stats
instead, but here's a comment based on something I thought about when reviewing
https://codereview.chromium.org/2632003003/.

https://codereview.webrtc.org/2629563003/diff/100001/webrtc/api/statscollecto...
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/2629563003/diff/100001/webrtc/api/statscollecto...
webrtc/api/statscollector.cc:131: report->AddFloat(
This can leak the default residual_echo_likelihood_recent_max -1 to the stats,
there should be an "if (residual_echo_likelihood_recent_max >= 0.0f)" or if the
two are tied together there should be a RTC_DCHECK_GE but that check would get
hit before Chrome's implementation is able to land.

[hbos, 2017-01-16 11:36:54]

https://codereview.webrtc.org/2629563003/diff/100001/webrtc/api/statscollecto...
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/2629563003/diff/100001/webrtc/api/statscollecto...
webrtc/api/statscollector.cc:131: report->AddFloat(
On 2017/01/16 11:29:49, hbos wrote:
> This can leak the default residual_echo_likelihood_recent_max -1 to the stats,
> there should be an "if (residual_echo_likelihood_recent_max >= 0.0f)" or if
the
> two are tied together there should be a RTC_DCHECK_GE but that check would get
> hit before Chrome's implementation is able to land.

If the stat is optional, prefer rtc::Optional to -1.

[hlundin-webrtc, 2017-01-16 13:44:56]

https://codereview.webrtc.org/2629563003/diff/100001/webrtc/api/statscollecto...
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/2629563003/diff/100001/webrtc/api/statscollecto...
webrtc/api/statscollector.cc:131: report->AddFloat(
On 2017/01/16 11:36:54, hbos wrote:
> On 2017/01/16 11:29:49, hbos wrote:
> > This can leak the default residual_echo_likelihood_recent_max -1 to the
stats,
> > there should be an "if (residual_echo_likelihood_recent_max >= 0.0f)" or if
> the
> > two are tied together there should be a RTC_DCHECK_GE but that check would
get
> > hit before Chrome's implementation is able to land.
> 
> If the stat is optional, prefer rtc::Optional to -1.

I'm making a minimal fix for now in https://codereview.webrtc.org/2632243002/.