Make use of new APM statistics interface.

Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525
Committed: https://crrev.com/7aba0297e65c3413c88a93c78435dc369e86fe46
Cr-Commit-Position: refs/heads/master@{#15065}

[ivoc, 2016-10-31 13:11:37]

Description was changed from

==========
Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics
function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525
==========

to

==========
Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics
function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525
==========

[ivoc, 2016-10-31 13:11:37]

ivoc@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[ivoc, 2016-10-31 13:14:04]

Hi Fredrik,

This small CL updates the GetStats() function on AudioSendStream to use the new
GetStatistics() function on APM instead of relying on VoE.

[the sun, 2016-11-01 08:35:23]

lgtm % hooking up relevant unit tests

[ivoc, 2016-11-01 13:17:37]

ivoc@webrtc.org changed reviewers:
+ ossu@webrtc.org

[ivoc, 2016-11-01 13:18:25]

Thanks for the review! Adding ossu to have a look at the changes to the
unittests.

[ossu, 2016-11-01 13:30:41]

On 2016/11/01 13:18:25, ivoc wrote:
> Thanks for the review! Adding ossu to have a look at the changes to the
> unittests.

Unit tests lgtm! The new setup should provide the same values as the old one.

[ivoc, 2016-11-02 16:22:45]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-11-02 16:22:46]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2463813002/#ps20001 (title:
"Updated unittests.")

[commit-bot: I haz the power, 2016-11-02 16:22:53]

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, 2016-11-02 16:30:22]

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

[commit-bot: I haz the power, 2016-11-02 16:30:23]

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

[ivoc, 2016-11-03 09:58:33]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-11-03 09:58:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2463813002/#ps40001 (title:
"Updated new default residual echo likelihood value (for inactive RED).")

[commit-bot: I haz the power, 2016-11-03 09:58:40]

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, 2016-11-03 10:06:34]

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

[commit-bot: I haz the power, 2016-11-03 10:06:35]

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

[ivoc, 2016-11-08 15:44:22]

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

[ivoc, 2016-11-08 15:53:56]

I made some changes to audio_processing_impl.cc to get the tests to pass on all
bots (see test results for the previous revision of this CL), so I added peah as
reviewer to have a look at the changes in that file. I'm not entirely sure if
this is a good way to fix this, so let me know if you have a better idea.

The issue is that the new AudioProcessing::Stat struct does some sanity checks
when setting values, such as checking that: minimum <= average <= maximum. It
seems like the stats that the AEC reports do not always pass these checks and
can hit the DCHECK on some bots. I don't really understand why this only happens
on these few bots, I was not able to reproduce this on my machine even though
the linux_rel bot is affected by this issue.

[ossu, 2016-11-08 16:17:36]

On 2016/11/08 15:53:56, ivoc wrote:
> I made some changes to audio_processing_impl.cc to get the tests to pass on
all
> bots (see test results for the previous revision of this CL), so I added peah
as
> reviewer to have a look at the changes in that file. I'm not entirely sure if
> this is a good way to fix this, so let me know if you have a better idea.
> 
> The issue is that the new AudioProcessing::Stat struct does some sanity checks
> when setting values, such as checking that: minimum <= average <= maximum. It
> seems like the stats that the AEC reports do not always pass these checks and
> can hit the DCHECK on some bots. I don't really understand why this only
happens
> on these few bots, I was not able to reproduce this on my machine even though
> the linux_rel bot is affected by this issue.

This looks like a big 'ole band-aid! I tried looking at the logs to see what was
going wrong, but I found nothing. Where do you see these values being weird?

[ivoc, 2016-11-08 16:23:26]

On 2016/11/08 16:17:36, ossu wrote:
> On 2016/11/08 15:53:56, ivoc wrote:
> > I made some changes to audio_processing_impl.cc to get the tests to pass on
> all
> > bots (see test results for the previous revision of this CL), so I added
peah
> as
> > reviewer to have a look at the changes in that file. I'm not entirely sure
if
> > this is a good way to fix this, so let me know if you have a better idea.
> > 
> > The issue is that the new AudioProcessing::Stat struct does some sanity
checks
> > when setting values, such as checking that: minimum <= average <= maximum.
It
> > seems like the stats that the AEC reports do not always pass these checks
and
> > can hit the DCHECK on some bots. I don't really understand why this only
> happens
> > on these few bots, I was not able to reproduce this on my machine even
though
> > the linux_rel bot is affected by this issue.
> 
> This looks like a big 'ole band-aid! I tried looking at the logs to see what
was
> going wrong, but I found nothing. Where do you see these values being weird?

It is definitely a band-aid :-) Maybe it's better to just remove the DCHECKS? Or
try to fix the issue inside of AEC (might affect the stats)? You can see the
values being weird here:
https://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/19964...
, search for "Check failed". It shows the following:

# Fatal error in
../../webrtc/modules/audio_processing/include/audio_processing.h, line 492
# last system error: 33
# Check failed: average >= minimum (-100 vs. 0)

[ossu, 2016-11-08 16:53:16]

On 2016/11/08 16:17:36, ossu wrote:
> On 2016/11/08 15:53:56, ivoc wrote:
> > I made some changes to audio_processing_impl.cc to get the tests to pass on
> all
> > bots (see test results for the previous revision of this CL), so I added
peah
> as
> > reviewer to have a look at the changes in that file. I'm not entirely sure
if
> > this is a good way to fix this, so let me know if you have a better idea.
> > 
> > The issue is that the new AudioProcessing::Stat struct does some sanity
checks
> > when setting values, such as checking that: minimum <= average <= maximum.
It
> > seems like the stats that the AEC reports do not always pass these checks
and
> > can hit the DCHECK on some bots. I don't really understand why this only
> happens
> > on these few bots, I was not able to reproduce this on my machine even
though
> > the linux_rel bot is affected by this issue.
> 
> This looks like a big 'ole band-aid! I tried looking at the logs to see what
was
> going wrong, but I found nothing. Where do you see these values being weird?

Ah, would you look at that!
So, if I trace it down, it looks like the EchoCancellation metrics are
explicitly set to kOffsetLevel (-100) for some cases. To me it looks like
whenever they cannot be properly initialized due to there not being enough data
available. (See: WebRtcAec_GetMetrics in echo_cancellation.cc).
In this case, it could be that himean hasn't been set, which causes average to
be set to -100.
In aec_core.cc:469, himean only gets set after at least two stat updates (with
one update, instant == average, which is not larger than average).

So maybe himean should be updated if metric->counter == 1 as well?

I believe if no updates have been made, then the checks in WebRtcAec_GetMetrics
are there to make sure all values are -100, in a round-about way. (to signify
invalid values, max is initialized to -100, min to 100 and average to -100).

[ivoc, 2016-11-09 10:20:43]

On 2016/11/08 16:53:16, ossu wrote:
> On 2016/11/08 16:17:36, ossu wrote:
> > On 2016/11/08 15:53:56, ivoc wrote:
> > > I made some changes to audio_processing_impl.cc to get the tests to pass
on
> > all
> > > bots (see test results for the previous revision of this CL), so I added
> peah
> > as
> > > reviewer to have a look at the changes in that file. I'm not entirely sure
> if
> > > this is a good way to fix this, so let me know if you have a better idea.
> > > 
> > > The issue is that the new AudioProcessing::Stat struct does some sanity
> checks
> > > when setting values, such as checking that: minimum <= average <= maximum.
> It
> > > seems like the stats that the AEC reports do not always pass these checks
> and
> > > can hit the DCHECK on some bots. I don't really understand why this only
> > happens
> > > on these few bots, I was not able to reproduce this on my machine even
> though
> > > the linux_rel bot is affected by this issue.
> > 
> > This looks like a big 'ole band-aid! I tried looking at the logs to see what
> was
> > going wrong, but I found nothing. Where do you see these values being weird?
> 
> Ah, would you look at that!
> So, if I trace it down, it looks like the EchoCancellation metrics are
> explicitly set to kOffsetLevel (-100) for some cases. To me it looks like
> whenever they cannot be properly initialized due to there not being enough
data
> available. (See: WebRtcAec_GetMetrics in echo_cancellation.cc).
> In this case, it could be that himean hasn't been set, which causes average to
> be set to -100.
> In aec_core.cc:469, himean only gets set after at least two stat updates (with
> one update, instant == average, which is not larger than average).
> 
> So maybe himean should be updated if metric->counter == 1 as well?
> 
> I believe if no updates have been made, then the checks in
WebRtcAec_GetMetrics
> are there to make sure all values are -100, in a round-about way. (to signify
> invalid values, max is initialized to -100, min to 100 and average to -100).

I agree with your analysis, but it's still unclear to me why this only happens
on a few of the bots. 
Let's wait for Per's input on if this should be changed/fixed inside of AEC or
if the DCHECKS should be removed and the stats should stay the way they are now.

[peah-webrtc, 2016-11-10 10:36:09]

https://codereview.webrtc.org/2463813002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2463813002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1502:
stats.a_nlp.Set(FixStatistic(metrics.a_nlp));
What you do here is basically to ensure that if any of the metrics.* struct
members is -100, then all is set to -100, right?

That makes sense and seems like the correct way to report the metrics.

It is, however, weird that the APM should ensure that the metrics in the AEC are
correctly reported. 
That constitutes an unneccessary dependency of APM on the implementation of the
AEC.

I think it would make more sense to move this correction into the AEC where the
metrics are computed. In that way APM would not need to be concerned about the
implementation of the AEC metrics.

WDYT?

[ossu, 2016-11-10 11:29:47]

https://codereview.webrtc.org/2463813002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2463813002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1502:
stats.a_nlp.Set(FixStatistic(metrics.a_nlp));
On 2016/11/10 10:36:09, peah-webrtc wrote:
> What you do here is basically to ensure that if any of the metrics.* struct
> members is -100, then all is set to -100, right?
> 
> That makes sense and seems like the correct way to report the metrics.
> 
> It is, however, weird that the APM should ensure that the metrics in the AEC
are
> correctly reported. 
> That constitutes an unneccessary dependency of APM on the implementation of
the
> AEC.
> 
> I think it would make more sense to move this correction into the AEC where
the
> metrics are computed. In that way APM would not need to be concerned about the
> implementation of the AEC metrics.
> 
> WDYT?

peah: Did you read my analysis of the problem in question? Do you see any
problems with it? I'd prefer it if the problem could be solved at the source,
rather than plastering it over afterwards.

[peah-webrtc, 2016-11-10 12:49:42]

On 2016/11/10 11:29:47, ossu wrote:
>
https://codereview.webrtc.org/2463813002/diff/60001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/audio_processing_impl.cc (right):
> 
>
https://codereview.webrtc.org/2463813002/diff/60001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/audio_processing_impl.cc:1502:
> stats.a_nlp.Set(FixStatistic(metrics.a_nlp));
> On 2016/11/10 10:36:09, peah-webrtc wrote:
> > What you do here is basically to ensure that if any of the metrics.* struct
> > members is -100, then all is set to -100, right?
> > 
> > That makes sense and seems like the correct way to report the metrics.
> > 
> > It is, however, weird that the APM should ensure that the metrics in the AEC
> are
> > correctly reported. 
> > That constitutes an unneccessary dependency of APM on the implementation of
> the
> > AEC.
> > 
> > I think it would make more sense to move this correction into the AEC where
> the
> > metrics are computed. In that way APM would not need to be concerned about
the
> > implementation of the AEC metrics.
> > 
> > WDYT?
> 
> peah: Did you read my analysis of the problem in question? Do you see any
> problems with it? I'd prefer it if the problem could be solved at the source,
> rather than plastering it over afterwards.

Sorry, I had not read your analysis. Reading it, I totally agree, and that is
kind of what I meant by that it should be corrected inside the AEC and not
inside APM.

The code for the stats is a bit messy though.
As you say, in aec_core.cc the metric values are initialized to -100 in order to
ensure initial updates to the first values arriving. However, in
echo_cancellation.cc, the WebRtcAec_GetMetrics call retrieves the collected
metrics from aec_core, analyses them, and then uses them to populate another
metric struct that is passed as the actual metric.

I don't know that code and I'm not sure where the correction of the metrics
value inside the AEC should be, but looking at the code it seems like that the
WebRtcAec_GetMetrics method in echo_cancellation.cc should produce proper metric
values, and that the fix should be somewhere there.

Does that make sense? Is that what you meant?

[ivoc, 2016-11-10 16:51:32]

I modified the way the stats are checked and set in echo_cancellation.cc to
prevent this from happening. Please have another look.

[ossu, 2016-11-11 10:24:07]

On 2016/11/10 16:51:32, ivoc wrote:
> I modified the way the stats are checked and set in echo_cancellation.cc to
> prevent this from happening. Please have another look.

Why didn't you change UpdateLogRatioMetric to always set himean? Concerns that
it might change the current behavior? Or did you try it and it didn't work?

[ossu, 2016-11-11 10:24:51]

On 2016/11/11 10:24:07, ossu wrote:
> On 2016/11/10 16:51:32, ivoc wrote:
> > I modified the way the stats are checked and set in echo_cancellation.cc to
> > prevent this from happening. Please have another look.
> 
> Why didn't you change UpdateLogRatioMetric to always set himean? Concerns that
> it might change the current behavior? Or did you try it and it didn't work?

Uhm, by "always set", I of course mean, set it for the first update as well, not
for every update.

[ivoc, 2016-11-11 13:58:35]

As we discussed earlier today, I now removed the attempts to fix the stats, and
just removed the DCHECKs. PTAL.

[ossu, 2016-11-11 14:00:58]

Yeah, makes sense. No point in changing behavior at this point. Still lgtm from
me!

[peah-webrtc, 2016-11-14 09:44:58]

On 2016/11/11 14:00:58, ossu wrote:
> Yeah, makes sense. No point in changing behavior at this point. Still lgtm
from
> me!

Nice!!!

lgtm

[ivoc, 2016-11-14 11:58:54]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-11-14 11:58:54]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2463813002/#ps100001 (title:
"Remove DCHECKs on values of stats.")

[commit-bot: I haz the power, 2016-11-14 11:59: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, 2016-11-14 12:21:00]

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

[commit-bot: I haz the power, 2016-11-14 12:21:01]

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

[ivoc, 2016-11-14 12:39:04]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2016-11-14 12:39:09]

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, 2016-11-14 12:52:09]

Description was changed from

==========
Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics
function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525
==========

to

==========
Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics
function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525
==========

[commit-bot: I haz the power, 2016-11-14 12:52:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-14 12:52:16]

Description was changed from

==========
Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics
function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525
==========

to

==========
Make use of new APM statistics interface.

Updates GetStats() function in AudioSendStream to use the new GetStatistics
function in APM instead of the corresponding VoE functions.

BUG=webrtc:6525

Committed: https://crrev.com/7aba0297e65c3413c88a93c78435dc369e86fe46
Cr-Commit-Position: refs/heads/master@{#15065}
==========

[commit-bot: I haz the power, 2016-11-14 12:52:18]

Patchset 6 (id:??) landed as
https://crrev.com/7aba0297e65c3413c88a93c78435dc369e86fe46
Cr-Commit-Position: refs/heads/master@{#15065}