Added histogram for the output level int the audio processing module

To verify the upcoming code changes it is required
that the level of the output in the audio processing
module is monitored. This CL adds that.

BUG=webrtc:6181, webrtc:6183, webrtc:6220
Review-Url: https://codereview.webrtc.org/2549143004
Cr-Commit-Position: refs/heads/master@{#15718}
Committed: https://chromium.googlesource.com/external/webrtc/+/1b08dc33ebcdb48f951258cf75d97dff0e60b4f1

[peah-webrtc, 2016-12-06 08:24:02]

Description was changed from

==========
Added another histogram

BUG=
==========

to

==========
To verify the upcoming code changes it is required
that the level of the output in the audio processing
module is monitored. This CL adds that.

BUG=webrtc:6181, webrtc:6183, webrtc:6220
==========

[peah-webrtc, 2016-12-06 08:27:14]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2016-12-06 15:55:14]

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

[peah-webrtc, 2016-12-06 15:55:14]

henrik.lundin@: PTAL

Thanks!

[hlundin-webrtc, 2016-12-06 16:07:10]

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

https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1236:
RTC_HISTOGRAM_COUNTS("WebRTC.Audio.ApmCaptureOutputLevelAverage",
Did you not want this to be RTC_HISTOGRAM_COUNTS_LINEAR?

https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1238:
RTC_HISTOGRAM_COUNTS("WebRTC.Audio.ApmCaptureOutputLevelPeak", levels.peak,
And this?

[hlundin-webrtc, 2016-12-06 16:08:00]

On 2016/12/06 16:07:10, hlundin-webrtc wrote:
>
https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/audio_processing_impl.cc (right):
> 
>
https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/audio_processing_impl.cc:1236:
> RTC_HISTOGRAM_COUNTS("WebRTC.Audio.ApmCaptureOutputLevelAverage",
> Did you not want this to be RTC_HISTOGRAM_COUNTS_LINEAR?
> 
>
https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/audio_processing_impl.cc:1238:
> RTC_HISTOGRAM_COUNTS("WebRTC.Audio.ApmCaptureOutputLevelPeak", levels.peak,
> And this?

Oh, and you should get the review process for histograms.xml in Chromium started
as well before you land this. Because any feedback you get there may apply here
as well.

[peah-webrtc, 2016-12-20 13:58:12]

Patchset #2 (id:40001) has been deleted

[peah-webrtc, 2016-12-20 14:02:39]

Thanks for the review!

I have updated the code. A CL is also created for adding this in histograms.xml
in Chromium.

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

https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1236:
RTC_HISTOGRAM_COUNTS("WebRTC.Audio.ApmCaptureOutputLevelAverage",
On 2016/12/06 16:07:10, hlundin-webrtc wrote:
> Did you not want this to be RTC_HISTOGRAM_COUNTS_LINEAR?

Fully true! Thanks! I also changed the ranges to match what is done for the
input.

Done.

https://codereview.webrtc.org/2549143004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1238:
RTC_HISTOGRAM_COUNTS("WebRTC.Audio.ApmCaptureOutputLevelPeak", levels.peak,
On 2016/12/06 16:07:10, hlundin-webrtc wrote:
> And this?

Done.

[hlundin-webrtc, 2016-12-20 14:41:27]

LG, but I have one suggestions.

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

https://codereview.webrtc.org/2549143004/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1138: if
(++capture_rms_interval_counter_ >= 1000) {
This feels a bit fragile now, with incrementing in one place, and resetting 100+
lines later. I suggest the following:

const bool log_rms = ++capture_rms_interval_counter_ >= 1000;
if (log_rms) {
  capture_rms_interval_counter_ = 0;
  // Do the input level logging...
}

...

And then use the same log_rms in the new logging below.

[peah-webrtc, 2016-12-20 14:46:47]

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

https://codereview.webrtc.org/2549143004/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1138: if
(++capture_rms_interval_counter_ >= 1000) {
On 2016/12/20 14:41:26, hlundin-webrtc wrote:
> This feels a bit fragile now, with incrementing in one place, and resetting
100+
> lines later. I suggest the following:
> 
> const bool log_rms = ++capture_rms_interval_counter_ >= 1000;
> if (log_rms) {
>   capture_rms_interval_counter_ = 0;
>   // Do the input level logging...
> }
> 
> ...
> 
> And then use the same log_rms in the new logging below.

Great point and suggestion!
Done.

[hlundin-webrtc, 2016-12-20 15:12:13]

LGTM, but don't commit until the corresponding Chromium CL is approved.

[peah-webrtc, 2016-12-20 20:36:36]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2016-12-20 20:36:44]

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-12-20 21:45:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482266196397980,
"parent_rev": "0838327ec96993a7ff3459bb3588203b6ccfc260", "commit_rev":
"1b08dc33ebcdb48f951258cf75d97dff0e60b4f1"}

[commit-bot: I haz the power, 2016-12-20 21:46:02]

Description was changed from

==========
To verify the upcoming code changes it is required
that the level of the output in the audio processing
module is monitored. This CL adds that.

BUG=webrtc:6181, webrtc:6183, webrtc:6220
==========

to

==========
To verify the upcoming code changes it is required
that the level of the output in the audio processing
module is monitored. This CL adds that.

BUG=webrtc:6181, webrtc:6183, webrtc:6220

Review-Url: https://codereview.webrtc.org/2549143004
Cr-Commit-Position: refs/heads/master@{#15718}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1b08dc33ebcdb48f951258cf7...
==========

[commit-bot: I haz the power, 2016-12-20 21:46:03]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/1b08dc33ebcdb48f951258cf7...