UMA histogram WebRTC.AecDelayBasedQuality updated

UMA histogram WebRTC.AecDelayBasedQuality updated

The UMA histogram is used to verify performance of the Finch experiment "UseDelayAgnosticAEC".

In webrtc:r8170 the Echo Delay Metrics queried through webrtc::EchoCancellation::GetEchoDelayMetrics() were changed from reset upon query to fixed aggregation window.
These metrics are used in GetStats() and further to calculate and log WebRTC.AecDelayBasedQuality.
Before the change we could not handle multiple clients, so we queried the metrics when a client called GetStats(). Now we can handle these two types of stats independently.

In webrtc:r8230 a new metric was added. This metric calculates the fraction of poor delays.
Before the change we calculated such a metric in chromium for logging WebRTC.AecDelayBasedQuality. Now we can query the metric directly and average several queries to match the desired aggregation length.

- Adds a fourth histogram bucket to track invalid data
- Adjusts the algorithm to log UMA histogram w.r.t. the new fraction_poor_delay metric
- Removes UMA histogram dependency on someone else calling GetStats()
- Changed GetAecStats() to take EchoCancellation instead of AudioProcessing.

BUG=450193
Committed: https://crrev.com/3d4c1dabb48e36492a7084513d3caf4e65189ca2
Cr-Commit-Position: refs/heads/master@{#315331}

[bjornv, 2015-02-07 17:43:14]

bjornv@chromium.org changed reviewers:
+ asvitkine@chromium.org, perkj@chromium.org

[bjornv, 2015-02-07 22:14:50]

Dear reviewers,

asvitkine: histogram.xml - Am I allowed to change the histogram size? I could do
without that invalid_state_bucket.

perkj: the rest

[perkj_chrome, 2015-02-09 07:53:14]

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:681:
echo_information_.get()->UpdateAecDelayStats(ap);
Is it safe to access echo_information_ on this thread? Looks like its created on
one thread and used on another.

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:227: void
EchoInformation::UpdateAecDelayStats(AudioProcessing* audio_processing) {
It looks like this argument should be the type of echo_cancellation() and be a
const pointer if possible.

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:228: // In WebRTC
three echo delay metrics are calculated and updated every second.
nit In WebRTC,  three echo ...

https://codereview.chromium.org/904203003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/904203003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:44877: +  <int value="0" label="Invalid
delays: Insufficient amount of data"/>
I don't think you can change the meaning of existent enum. You can probably add
to it but not change the existing. In that case you will have to deprecate this
and add a new instead.

[bjornv, 2015-02-09 11:11:16]

perkj, PTAL at this new patch set.

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:681:
echo_information_.get()->UpdateAecDelayStats(ap);
On 2015/02/09 07:53:13, perkj wrote:
> Is it safe to access echo_information_ on this thread? Looks like its created
on
> one thread and used on another. 

Then, isn't it unsafe to use audio_processing_ at all, since it is created on
main_thread, but used on capture_thread just like echo_information_

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:227: void
EchoInformation::UpdateAecDelayStats(AudioProcessing* audio_processing) {
On 2015/02/09 07:53:13, perkj wrote:
> It looks like this argument should be the type of echo_cancellation() and be a
> const pointer if possible.

Much better. Done

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:228: // In WebRTC
three echo delay metrics are calculated and updated every second.
On 2015/02/09 07:53:13, perkj wrote:
> nit In WebRTC,  three echo ...

Done.

https://codereview.chromium.org/904203003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/904203003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:44877: +  <int value="0" label="Invalid
delays: Insufficient amount of data"/>
On 2015/02/09 07:53:14, perkj wrote:
> I don't think you can change the meaning of existent enum. You can probably
add
> to it but not change the existing. In that case you will have to deprecate
this
> and add a new instead.

Changed order, but letting asvitkine to decide whether to add it at all or not.

[perkj_chrome, 2015-02-09 11:48:31]

lgtm

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/904203003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:681:
echo_information_.get()->UpdateAecDelayStats(ap);
On 2015/02/09 11:11:16, bjornv wrote:
> On 2015/02/09 07:53:13, perkj wrote:
> > Is it safe to access echo_information_ on this thread? Looks like its
created
> on
> > one thread and used on another. 
> 
> Then, isn't it unsafe to use audio_processing_ at all, since it is created on
> main_thread, but used on capture_thread just like echo_information_

true, and that seems to be the only place it is touched.

https://codereview.chromium.org/904203003/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/904203003/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:248: if
(!echo_cancellation->GetDelayMetrics(&dummy_median, &dummy_std,
Not from this cl I see but it looks quite weird to have a return value that is
false and at the same time trust an out parameter to that method.

[bjornv, 2015-02-09 14:31:39]

New patchsets have been uploaded after l-g-t-m from perkj@chromium.org

[bjornv, 2015-02-09 14:39:11]

perkj: I did two changes
1) Since echo_cancellation->GetDelayMetrics() returns int I did a proper check
in the if statement.
2) GetStats() calls GetAecStats() using audio_processing. Your suggestion of
using echo_cancellation at the other place made me change here as well.

PTAL. Since the diff is rather big compared to what you lgtm:ed I think you
should give a new stamp.

https://codereview.chromium.org/904203003/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/904203003/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:248: if
(!echo_cancellation->GetDelayMetrics(&dummy_median, &dummy_std,
On 2015/02/09 11:48:30, perkj wrote:
> Not from this cl I see but it looks quite weird to have a return value that is
> false and at the same time trust an out parameter to that method. 

Agree. In fact Karl would have killed me for not changing to use == instead.

[perkj_chrome, 2015-02-09 15:29:03]

still lgtm

[Alexei Svitkine (slow), 2015-02-09 15:40:37]

lgtm

[bjornv, 2015-02-09 17:22:38]

The CQ bit was checked by bjornv@chromium.org

[commit-bot: I haz the power, 2015-02-09 17:24:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904203003/60001

[commit-bot: I haz the power, 2015-02-09 17:28:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-09 17:28:41]

Patchset 4 (id:??) landed as
https://crrev.com/3d4c1dabb48e36492a7084513d3caf4e65189ca2
Cr-Commit-Position: refs/heads/master@{#315331}