Adds UMA histogram for system delay jumps

Adds UMA histogram for system delay jumps

Sudden platform system delay jumps can hurt AEC and we have no stats that monitor these jumps. How often do they occur, and when they are reported are they accurate?

This CL logs all jumps in both the reported and actual delay.

The histogram has been tested with a chromium build where a fake jump of 200 ms was applied after 5 seconds and it was registered correctly in chrome://histograms

BUG=488124
R=henrik.lundin@webrtc.org, peah@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/1ca324f23770e17d19433ccd3a3b021870d709ca

[bjornv1, 2015-06-26 17:20:06]

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

[bjornv1, 2015-06-26 17:22:22]

Dear reviewers, PTAL at this UMA histogram CL.

Should we log the jump size or the actual delay after the jump?
Also, should we log the difference between the reported delay and system delay?

[hlundin-webrtc, 2015-06-29 08:53:19]

Mostly LG, but please, see comments.

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

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1010: 1001 -
kMinDiffDelayMs);
Do we need granularity of 1 ms?

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1015: const int
frames_per_ms = split_rate_ / 1000;
This should be an exact division (zero remainder), right? Please, use
rtc::CheckedDivExact (in checks.h).

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1016: const int
aec_system_delay_ms =
(I'm guessing this one is not expected to be an exact division.)

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1024: 1001 -
kMinDiffDelayMs);
Same here.

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_tests.gypi (right):

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_tests.gypi:114:
'<(webrtc_root)/system_wrappers/system_wrappers.gyp:system_wrappers_default',
Why did you have to add this?

[bjornv1, 2015-06-29 10:40:23]

PTAL

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

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1010: 1001 -
kMinDiffDelayMs);
On 2015/06/29 08:53:18, hlundin-webrtc wrote:
> Do we need granularity of 1 ms?

No we don't. 10 ms should be just fine.

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1015: const int
frames_per_ms = split_rate_ / 1000;
On 2015/06/29 08:53:18, hlundin-webrtc wrote:
> This should be an exact division (zero remainder), right? Please, use
> rtc::CheckedDivExact (in checks.h).

Done.

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1016: const int
aec_system_delay_ms =
On 2015/06/29 08:53:18, hlundin-webrtc wrote:
> (I'm guessing this one is not expected to be an exact division.)

No, this can end up with rounding.

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1024: 1001 -
kMinDiffDelayMs);
On 2015/06/29 08:53:18, hlundin-webrtc wrote:
> Same here.

Done.

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_tests.gypi (right):

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_tests.gypi:114:
'<(webrtc_root)/system_wrappers/system_wrappers.gyp:system_wrappers_default',
On 2015/06/29 08:53:18, hlundin-webrtc wrote:
> Why did you have to add this?

We need a default implementation of metrics.

[hlundin-webrtc, 2015-06-29 10:55:05]

LGTM with one nit.

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_tests.gypi (right):

https://codereview.webrtc.org/1213733004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_tests.gypi:114:
'<(webrtc_root)/system_wrappers/system_wrappers.gyp:system_wrappers_default',
On 2015/06/29 10:40:23, bjornv1 wrote:
> On 2015/06/29 08:53:18, hlundin-webrtc wrote:
> > Why did you have to add this?
> 
> We need a default implementation of metrics.

Acknowledged.

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

https://codereview.webrtc.org/1213733004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1014: const int
frames_per_ms = rtc::CheckedDivExact(split_rate_, 1000);
This is declared in webrtc/base/checks.h, but I didn't see it in the list of
includes. Please, include what you use.

[bjornv1, 2015-06-29 11:05:01]

Per, any additional comments?

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

https://codereview.webrtc.org/1213733004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:1014: const int
frames_per_ms = rtc::CheckedDivExact(split_rate_, 1000);
On 2015/06/29 10:55:05, hlundin-webrtc wrote:
> This is declared in webrtc/base/checks.h, but I didn't see it in the list of
> includes. Please, include what you use.

Done.

[peah-webrtc, 2015-06-29 12:05:42]

Since diff_stream_delay_ms is computed as
const int diff_stream_delay_ms = stream_delay_ms_ - last_stream_delay_ms_;

and the following delay change size comparison is onesided only the code will
only detect significant delay increases. The same holds for
diff_aec_system_delay_ms.

Perhaps this is intended but it could make sense to make detect significant
delay decreases as well by computing the absolute diffs.

If it is intended to have this as a one-sided comparison, it looks good to me.
Otherwise the code maybe should be change to take that into account.

[peah-webrtc, 2015-06-29 12:17:53]

lgtm

[peah-webrtc, 2015-06-29 12:18:02]

lgtm

[peah-webrtc, 2015-06-29 12:18:10]

lgtm

[bjornv1, 2015-06-29 12:57:48]

Committed patchset #3 (id:40001) manually as
1ca324f23770e17d19433ccd3a3b021870d709ca (presubmit successful).