Adding stats that can be used to compute output audio levels.

Adding stats that can be used to compute output audio levels as described here https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-totalaudioenergy.

BUG=webrtc:7982
Review-Url: https://codereview.webrtc.org/2964593002
Cr-Commit-Position: refs/heads/master@{#19027}
Committed: https://chromium.googlesource.com/external/webrtc/+/e76bd3aa43821ff10f5cef5854836ea782b2464f

[Zach Stein, 2017-06-29 21:55:34]

zstein@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Zach Stein, 2017-06-29 21:55:35]

Found an integer truncation issue.
Looking in to the 2 TODOs and trying to understand why the stat isn't showing up
in the statreport next.

[Zach Stein, 2017-06-30 23:14:56]

Description was changed from

==========
Adding stats that can be used to compute output audio levels.

Based on https://codereview.webrtc.org/2935693005/ with the following changes:
channel.cc: Cast the audio level to a double to avoid truncating division and
use INT16_MAX instead of SHRT_MAX.

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

to

==========
Adding stats that can be used to compute output audio levels.

Based on https://codereview.webrtc.org/2935693005/ with the following changes:
channel.cc: Cast the audio level to a double to avoid truncating division and
use INT16_MAX instead of SHRT_MAX.
rtcstats_objects.cc: Whitelist the new stats via WEBRTC_RTCSTATS_IMPL so they
show up in the final report.
Also compute stats for send streams (the energy and duration is computed in
TransmitMixer).

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

[Zach Stein, 2017-06-30 23:16:00]

Got the stats showing up in the report and roughed out an implementation for
send streams.

[the sun, 2017-07-03 08:14:57]

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

[the sun, 2017-07-03 08:16:27]

Do you really need double precision?

[hlundin-webrtc, 2017-07-03 13:23:03]

I'm not sure if double precision is strictly needed, but since this stat will
accumulate rather large numbers "forever", I'd bet on double to be safe(r).

See minor comments and questions inline.

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:700: // TODO(zstein): Use sample_rate_hz_?
What would you do with sample_rate_hz_?

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:476: double _totalOutputEnergy = 0;
Although the end result will be the same, I tend to prefer initializing doubles
with 0.0 instead of 0.

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:313: // TODO(zstein): Use sample_rate_hz_?
What would you do with sample_rate_hz_?

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.h:197: double _totalInputEnergy = 0;
0.0 here and below.

[Zach Stein, 2017-07-06 17:40:23]

On 2017/07/03 13:23:03, hlundin-webrtc (OOOtoJul3) wrote:
> I'm not sure if double precision is strictly needed, but since this stat will
> accumulate rather large numbers "forever", I'd bet on double to be safe(r).

I don't have a strong preference. I'm guessing Taylor chose to use a double
because that is what RTCMediaStreamTrackStats::audio_level uses.

[Zach Stein, 2017-07-06 17:41:38]

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:700: // TODO(zstein): Use sample_rate_hz_?
On 2017/07/03 13:23:03, hlundin-webrtc (OOOtoJul3) wrote:
> What would you do with sample_rate_hz_?

I wasn't sure if this method was intended to process 10ms of audio or if the
duration should be dynamic. Looking again, it looks like 10ms is guaranteed here
though, so I will remove this.

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:476: double _totalOutputEnergy = 0;
On 2017/07/03 13:23:03, hlundin-webrtc (OOOtoJul3) wrote:
> Although the end result will be the same, I tend to prefer initializing
doubles
> with 0.0 instead of 0.

Done.

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:312: // TODO(zstein): Extract helper to
share with voice_engine/channel.cc
I think we could add this computation to AudioLevel::ComputeLevel and expose
energy and duration from there if people prefer that approach.

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:313: // TODO(zstein): Use sample_rate_hz_?
On 2017/07/03 13:23:03, hlundin-webrtc (OOOtoJul3) wrote:
> What would you do with sample_rate_hz_?

Done.

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.h:197: double _totalInputEnergy = 0;
On 2017/07/03 13:23:03, hlundin-webrtc (OOOtoJul3) wrote:
> 0.0 here and below.

Done.

[Zach Stein, 2017-07-06 17:46:12]

zstein@webrtc.org changed reviewers:
+ hbos@webrtc.org, solenberg@webrtc.org

[Zach Stein, 2017-07-06 17:46:13]

PTAL

[Taylor Brandstetter, 2017-07-06 17:57:56]

lgtm for the code I own (with minor nits about comments/variable names). It
would be ideal if we could have some low-level tests that verify that the actual
values of the stats match what's expected (passing in some hard-coded audio
data), but it doesn't look like that level of testing currently exists.
hlundin@, thoughts?

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:282: // TODO(zstein): Consistent use of
input/output in names.
Can this be fixed before this CL lands? I'd consistently use "output" for the
receiver side and "input" for the sender side.

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/pc/rtcstats_integr...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/pc/rtcstats_integr...
webrtc/pc/rtcstats_integrationtest.cc:496: // Remote audio tracks are expected
to have audio energy/duration values
Since you got this to work for both remote and local tracks (right?), this
comment can be updated.

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:316: // thedifference between the results
from two getStats calls. To do this,
nit: space between "the" and "difference"

[Zach Stein, 2017-07-06 18:07:35]

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:282: // TODO(zstein): Consistent use of
input/output in names.
On 2017/07/06 17:57:56, Taylor Brandstetter wrote:
> Can this be fixed before this CL lands? I'd consistently use "output" for the
> receiver side and "input" for the sender side.

Done.

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/pc/rtcstats_integr...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/pc/rtcstats_integr...
webrtc/pc/rtcstats_integrationtest.cc:496: // Remote audio tracks are expected
to have audio energy/duration values
On 2017/07/06 17:57:56, Taylor Brandstetter wrote:
> Since you got this to work for both remote and local tracks (right?), this
> comment can be updated.

Done.

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/2964593002/diff/60001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:316: // thedifference between the results
from two getStats calls. To do this,
On 2017/07/06 17:57:56, Taylor Brandstetter wrote:
> nit: space between "the" and "difference"

Done.

[hlundin-webrtc, 2017-07-07 06:48:44]

lgtm

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2964593002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:700: // TODO(zstein): Use sample_rate_hz_?
On 2017/07/06 17:41:37, Zach Stein wrote:
> On 2017/07/03 13:23:03, hlundin-webrtc (OOOtoJul3) wrote:
> > What would you do with sample_rate_hz_?
> 
> I wasn't sure if this method was intended to process 10ms of audio or if the
> duration should be dynamic. Looking again, it looks like 10ms is guaranteed
here
> though, so I will remove this.

Acknowledged.

[hbos, 2017-07-07 12:52:25]

I didn't have time to fully review this today due to leaving early but it looked
good, just heads up about chromium whitelist, I'll revist on monday.

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/pc/rtcstats_integ...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/pc/rtcstats_integ...
webrtc/pc/rtcstats_integrationtest.cc:497: // be used to compute audio levels
between getStats calls.
nit: I think this comment is superfluous, we don't have them for all the other
TestMember...

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/stats/rtcstats_ob...
File webrtc/stats/rtcstats_objects.cc (right):

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/stats/rtcstats_ob...
webrtc/stats/rtcstats_objects.cc:353: "track",
Place RTCMediaStreamTrackStats, RTCStats, "track" on the same line followed by
one member per line. Style guide of this file with the weird macros. :)

The WEBRTC_RTCSTATS_IMPL defines all members of the stats objects so that they
are iterable ("for each stat in stats object"). As a result, not having a member
listed here will mean it is missed for example when exporting stats to Android
or other platforms that get stats by iterate all stats.

It's not meant to be a whitelist though (update CL description) and not listing
a stat here would be an error, whitelisted or not.

Speaking of whitelists, there is a test in chromium which tries to make sure we
don't expose any stats that have not been whitelisted with a chromium repo CL
(guard against non-standard stats), see javascript test:
https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/peerconnection_g...

If this CL lands without the chromium whitelist having been updated first the
roll into chromium will fail if the browser test detects the stat. Please make a
chromium CL whitelisting it, or if you don't work with chromium or don't know
what to do, I can whitelist it for you no problem.

[Taylor Brandstetter, 2017-07-07 16:15:16]

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/pc/rtcstats_integ...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/pc/rtcstats_integ...
webrtc/pc/rtcstats_integrationtest.cc:497: // be used to compute audio levels
between getStats calls.
On 2017/07/07 12:52:24, hbos wrote:
> nit: I think this comment is superfluous, we don't have them for all the other
> TestMember...

Yeah, may be now. I think I added it before because it was a special case (only
worked for remote tracks).

[Zach Stein, 2017-07-07 21:21:01]

Description was changed from

==========
Adding stats that can be used to compute output audio levels.

Based on https://codereview.webrtc.org/2935693005/ with the following changes:
channel.cc: Cast the audio level to a double to avoid truncating division and
use INT16_MAX instead of SHRT_MAX.
rtcstats_objects.cc: Whitelist the new stats via WEBRTC_RTCSTATS_IMPL so they
show up in the final report.
Also compute stats for send streams (the energy and duration is computed in
TransmitMixer).

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

to

==========
Adding stats that can be used to compute output audio levels.

Based on https://codereview.webrtc.org/2935693005/ with the following changes:
channel.cc: Cast the audio level to a double to avoid truncating division and
use INT16_MAX instead of SHRT_MAX.
rtcstats_objects.cc: Include the new stats via WEBRTC_RTCSTATS_IMPL so they show
up in the final report.
Also compute stats for send streams (the energy and duration is computed in
TransmitMixer).

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

[Zach Stein, 2017-07-07 21:27:37]

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/pc/rtcstats_integ...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/pc/rtcstats_integ...
webrtc/pc/rtcstats_integrationtest.cc:497: // be used to compute audio levels
between getStats calls.
On 2017/07/07 16:15:15, Taylor Brandstetter wrote:
> On 2017/07/07 12:52:24, hbos wrote:
> > nit: I think this comment is superfluous, we don't have them for all the
other
> > TestMember...
> 
> Yeah, may be now. I think I added it before because it was a special case
(only
> worked for remote tracks).

Agreed.

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/stats/rtcstats_ob...
File webrtc/stats/rtcstats_objects.cc (right):

https://codereview.webrtc.org/2964593002/diff/100001/webrtc/stats/rtcstats_ob...
webrtc/stats/rtcstats_objects.cc:353: "track",
On 2017/07/07 12:52:24, hbos wrote:
> Place RTCMediaStreamTrackStats, RTCStats, "track" on the same line followed by
> one member per line. Style guide of this file with the weird macros. :)

Done. "git cl format" changed it and I didn't notice.

> The WEBRTC_RTCSTATS_IMPL defines all members of the stats objects so that they
> are iterable ("for each stat in stats object"). As a result, not having a
member
> listed here will mean it is missed for example when exporting stats to Android
> or other platforms that get stats by iterate all stats.
> 
> It's not meant to be a whitelist though (update CL description) and not
listing
> a stat here would be an error, whitelisted or not.

Got it, thanks. Description updated.

> Speaking of whitelists, there is a test in chromium which tries to make sure
we
> don't expose any stats that have not been whitelisted with a chromium repo CL
> (guard against non-standard stats), see javascript test:
>
https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/peerconnection_g...
> 
> If this CL lands without the chromium whitelist having been updated first the
> roll into chromium will fail if the browser test detects the stat. Please make
a
> chromium CL whitelisting it, or if you don't work with chromium or don't know
> what to do, I can whitelist it for you no problem.

Thanks for the heads up. I'll try to whitelist in Chromium before submitting.

[Zach Stein, 2017-07-08 00:05:40]

Turns out a little more work was needed to get the stats showing up in Java. I
also fixed a test I missed before.

[hbos, 2017-07-10 09:58:25]

lgtm % nits

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_receiv...
File webrtc/call/audio_receive_stream.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_receiv...
webrtc/call/audio_receive_stream.h:52: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
Ditto link.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_send_s...
File webrtc/call/audio_send_stream.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_send_s...
webrtc/call/audio_send_stream.h:50: // See description of "totalAudioEnergy" in
the WebRTC stats spec.
Ditto link.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:631: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
nit: Provide the w3c.github.io-link (not rawgit-link), e.g:
https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-totalaudioen...

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:633: float total_input_duration;
This should be double. The reason the other is float is probably because the old
getStats code use float and this file was initially written for that, but the
new (rtcstats_objects.h) supports and uses double, and the metrics are reported
as double before they get here.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:674: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
Ditto link.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:676: float total_output_duration;
Ditto double.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:705: // value needs to be of units "squared
sample value * time".
Ditto link.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:710: _totalOutputDuration += 0.01;
nit: Can you have a constant double kAudioSampleDurationSeconds = 0.01;
somewhere?

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:86: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
Ditto link.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.cc:317: // the value needs to be of units
"squared sample value * time".
Ditto link, ditto constant.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:79: // See description of
"totalAudioEnergy" in the WebRTC stats spec.
Ditto link.

[Zach Stein, 2017-07-10 18:35:21]

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_receiv...
File webrtc/call/audio_receive_stream.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_receiv...
webrtc/call/audio_receive_stream.h:52: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link.

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_send_s...
File webrtc/call/audio_send_stream.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/call/audio_send_s...
webrtc/call/audio_send_stream.h:50: // See description of "totalAudioEnergy" in
the WebRTC stats spec.
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link.

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:631: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
On 2017/07/10 09:58:24, hbos wrote:
> nit: Provide the w3c.github.io-link (not rawgit-link), e.g:
>
https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-totalaudioen...

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:633: float total_input_duration;
On 2017/07/10 09:58:24, hbos wrote:
> This should be double. The reason the other is float is probably because the
old
> getStats code use float and this file was initially written for that, but the
> new (rtcstats_objects.h) supports and uses double, and the metrics are
reported
> as double before they get here.

I changed this to make it work with the changes in statscollector.cc, which pass
this value along to StatsReport::AddFloat. Doesn't look like StatsReport
supports doubles. Would you prefer I cast the values in stascollector.cc? This
seemed more consistent with the existing code to me.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:674: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link.

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:705: // value needs to be of units "squared
sample value * time".
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link.

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:710: _totalOutputDuration += 0.01;
On 2017/07/10 09:58:24, hbos wrote:
> nit: Can you have a constant double kAudioSampleDurationSeconds = 0.01;
> somewhere?

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:86: // See description of "totalAudioEnergy"
in the WebRTC stats spec.
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link.

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.cc:317: // the value needs to be of units
"squared sample value * time".
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link, ditto constant.

Link added. I think the sample duration should actually be dynamic here though
(also done).

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:79: // See description of
"totalAudioEnergy" in the WebRTC stats spec.
On 2017/07/10 09:58:24, hbos wrote:
> Ditto link.

Done.

[Zach Stein, 2017-07-10 19:19:06]

zstein@webrtc.org changed reviewers:
+ henrika@webrtc.org, ossu@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

[Zach Stein, 2017-07-10 19:19:07]

Still need reviewers for audio, voice_engine, and call. PTAL

[pbos-webrtc, 2017-07-10 22:13:17]

On 2017/07/10 19:19:07, Zach Stein wrote:
> Still need reviewers for audio, voice_engine, and call. PTAL

webrtc/call rs-lgtm

[hbos, 2017-07-11 08:01:01]

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:633: float total_input_duration;
On 2017/07/10 18:35:20, Zach Stein wrote:
> On 2017/07/10 09:58:24, hbos wrote:
> > This should be double. The reason the other is float is probably because the
> old
> > getStats code use float and this file was initially written for that, but
the
> > new (rtcstats_objects.h) supports and uses double, and the metrics are
> reported
> > as double before they get here.
> 
> I changed this to make it work with the changes in statscollector.cc, which
pass
> this value along to StatsReport::AddFloat. Doesn't look like StatsReport
> supports doubles. Would you prefer I cast the values in stascollector.cc? This
> seemed more consistent with the existing code to me.

Yes, cast it to float in statscollector.cc before AddFloat. That way other stats
consumers/rtcstatscollector.cc are not limited in precision due to limitations
of statscollector.cc.

[ossu, 2017-07-11 10:38:38]

lgtm % nits

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:476: double _totalOutputEnergy = 0.0;
I think these should be totalOutputEnergy_ etc. - with the underscore at the
end. It's the current style and plenty of other things here use that.

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:198: double _totalInputEnergy = 0.0;
Same here: since both styles are used in this file, try to use the current one
for new additions.

[Zach Stein, 2017-07-11 19:50:28]

zstein@webrtc.org changed reviewers:
+ niklas.enbom@webrtc.org

[Zach Stein, 2017-07-11 19:50:30]

Thanks for all the feedback.
Just looking for a reviewer for the changes under webrtc/voice_engine now.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:633: float total_input_duration;
On 2017/07/11 08:01:01, hbos wrote:
> On 2017/07/10 18:35:20, Zach Stein wrote:
> > On 2017/07/10 09:58:24, hbos wrote:
> > > This should be double. The reason the other is float is probably because
the
> > old
> > > getStats code use float and this file was initially written for that, but
> the
> > > new (rtcstats_objects.h) supports and uses double, and the metrics are
> > reported
> > > as double before they get here.
> > 
> > I changed this to make it work with the changes in statscollector.cc, which
> pass
> > this value along to StatsReport::AddFloat. Doesn't look like StatsReport
> > supports doubles. Would you prefer I cast the values in stascollector.cc?
This
> > seemed more consistent with the existing code to me.
> 
> Yes, cast it to float in statscollector.cc before AddFloat. That way other
stats
> consumers/rtcstatscollector.cc are not limited in precision due to limitations
> of statscollector.cc.

Done.

https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:676: float total_output_duration;
On 2017/07/10 09:58:24, hbos wrote:
> Ditto double.

Done.

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:476: double _totalOutputEnergy = 0.0;
On 2017/07/11 10:38:37, ossu wrote:
> I think these should be totalOutputEnergy_ etc. - with the underscore at the
> end. It's the current style and plenty of other things here use that.

Done.

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:198: double _totalInputEnergy = 0.0;
On 2017/07/11 10:38:37, ossu wrote:
> Same here: since both styles are used in this file, try to use the current one
> for new additions.

Done.

[Zach Stein, 2017-07-12 21:31:49]

zstein@webrtc.org changed reviewers:
+ henrikg@webrtc.org

[niklas.enbom, 2017-07-12 23:32:07]

The voice_engine code seems to be reviewed by a good set of reviewers already,
so this is just a RS lgtm.

On 2017/07/11 19:50:30, Zach Stein wrote:
> Thanks for all the feedback.
> Just looking for a reviewer for the changes under webrtc/voice_engine now.
> 
>
https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
> File webrtc/media/base/mediachannel.h (right):
> 
>
https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
> webrtc/media/base/mediachannel.h:633: float total_input_duration;
> On 2017/07/11 08:01:01, hbos wrote:
> > On 2017/07/10 18:35:20, Zach Stein wrote:
> > > On 2017/07/10 09:58:24, hbos wrote:
> > > > This should be double. The reason the other is float is probably because
> the
> > > old
> > > > getStats code use float and this file was initially written for that,
but
> > the
> > > > new (rtcstats_objects.h) supports and uses double, and the metrics are
> > > reported
> > > > as double before they get here.
> > > 
> > > I changed this to make it work with the changes in statscollector.cc,
which
> > pass
> > > this value along to StatsReport::AddFloat. Doesn't look like StatsReport
> > > supports doubles. Would you prefer I cast the values in stascollector.cc?
> This
> > > seemed more consistent with the existing code to me.
> > 
> > Yes, cast it to float in statscollector.cc before AddFloat. That way other
> stats
> > consumers/rtcstatscollector.cc are not limited in precision due to
limitations
> > of statscollector.cc.
> 
> Done.
> 
>
https://codereview.webrtc.org/2964593002/diff/140001/webrtc/media/base/mediac...
> webrtc/media/base/mediachannel.h:676: float total_output_duration;
> On 2017/07/10 09:58:24, hbos wrote:
> > Ditto double.
> 
> Done.
> 
>
https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/chan...
> File webrtc/voice_engine/channel.h (right):
> 
>
https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/chan...
> webrtc/voice_engine/channel.h:476: double _totalOutputEnergy = 0.0;
> On 2017/07/11 10:38:37, ossu wrote:
> > I think these should be totalOutputEnergy_ etc. - with the underscore at the
> > end. It's the current style and plenty of other things here use that.
> 
> Done.
> 
>
https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/tran...
> File webrtc/voice_engine/transmit_mixer.h (right):
> 
>
https://codereview.webrtc.org/2964593002/diff/160001/webrtc/voice_engine/tran...
> webrtc/voice_engine/transmit_mixer.h:198: double _totalInputEnergy = 0.0;
> On 2017/07/11 10:38:37, ossu wrote:
> > Same here: since both styles are used in this file, try to use the current
one
> > for new additions.
> 
> Done.

[Zach Stein, 2017-07-13 00:43:15]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-07-13 00:43:16]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
henrik.lundin@webrtc.org, hbos@webrtc.org, ossu@webrtc.org, pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2964593002/#ps200001 (title:
"Add bug link to TODO.")

[commit-bot: I haz the power, 2017-07-13 00:43:25]

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-07-13 00:49:33]

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

[commit-bot: I haz the power, 2017-07-13 00:49:35]

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

[Zach Stein, 2017-07-13 04:17:57]

Description was changed from

==========
Adding stats that can be used to compute output audio levels.

Based on https://codereview.webrtc.org/2935693005/ with the following changes:
channel.cc: Cast the audio level to a double to avoid truncating division and
use INT16_MAX instead of SHRT_MAX.
rtcstats_objects.cc: Include the new stats via WEBRTC_RTCSTATS_IMPL so they show
up in the final report.
Also compute stats for send streams (the energy and duration is computed in
TransmitMixer).

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

to

==========
Adding stats that can be used to compute output audio levels as described here
https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-totalaudioen....

BUG=webrtc:7982
==========

[Henrik Grunell WebRTC, 2017-07-13 08:28:23]

On 2017/07/11 19:50:30, Zach Stein wrote:
> Thanks for all the feedback.
> Just looking for a reviewer for the changes under webrtc/voice_engine now.

Has anyone properly reviewed the voice_engine code? Oskar?

There seems to be way too many reviewers on this CL. Please try stick to one
owner per component, state for each reviewer which files they are expected to
review. If you need to switch reviewer either remove the old, or clearly state
what the new reviewing expectations are. It can be tricky in vacation times! :)

[Zach Stein, 2017-07-13 18:02:17]

zstein@webrtc.org changed reviewers:
- henrika@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org

[Zach Stein, 2017-07-13 18:04:31]

On 2017/07/13 08:28:23, Henrik Grunell WebRTC wrote:
> On 2017/07/11 19:50:30, Zach Stein wrote:
> > Thanks for all the feedback.
> > Just looking for a reviewer for the changes under webrtc/voice_engine now.
> 
> Has anyone properly reviewed the voice_engine code? Oskar?
> 
> There seems to be way too many reviewers on this CL. Please try stick to one
> owner per component, state for each reviewer which files they are expected to
> review. If you need to switch reviewer either remove the old, or clearly state
> what the new reviewing expectations are. It can be tricky in vacation times!
:)

It hasn't been reviewed by an owner - could you take a look?

[Henrik Grunell WebRTC, 2017-07-14 10:54:14]

On 2017/07/13 18:04:31, Zach Stein wrote:
> On 2017/07/13 08:28:23, Henrik Grunell WebRTC wrote:
> > On 2017/07/11 19:50:30, Zach Stein wrote:
> > > Thanks for all the feedback.
> > > Just looking for a reviewer for the changes under webrtc/voice_engine now.
> > 
> > Has anyone properly reviewed the voice_engine code? Oskar?
> > 
> > There seems to be way too many reviewers on this CL. Please try stick to one
> > owner per component, state for each reviewer which files they are expected
to
> > review. If you need to switch reviewer either remove the old, or clearly
state
> > what the new reviewing expectations are. It can be tricky in vacation times!
> :)
> 
> It hasn't been reviewed by an owner - could you take a look?

I checked with Oskar; at least he has reviewed voice_engine properly. rs lgtm.

[Zach Stein, 2017-07-14 18:45:47]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-07-14 18:45:47]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org,
henrik.lundin@webrtc.org, niklas.enbom@webrtc.org, hbos@webrtc.org,
pbos@webrtc.org, deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2964593002/#ps240001 (title:
"Add test coverage in AudioSendStreamTest.")

[commit-bot: I haz the power, 2017-07-14 18:45:56]

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-07-14 19:17:53]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1500057947232140,
"parent_rev": "06f3aae345854ba9dcc5ae3b603de1f86505acf9", "commit_rev":
"e76bd3aa43821ff10f5cef5854836ea782b2464f"}

[commit-bot: I haz the power, 2017-07-14 19:17:58]

Description was changed from

==========
Adding stats that can be used to compute output audio levels as described here
https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-totalaudioen....

BUG=webrtc:7982
==========

to

==========
Adding stats that can be used to compute output audio levels as described here
https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-totalaudioen....

BUG=webrtc:7982

Review-Url: https://codereview.webrtc.org/2964593002
Cr-Commit-Position: refs/heads/master@{#19027}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e76bd3aa43821ff10f5cef585...
==========

[commit-bot: I haz the power, 2017-07-14 19:18:01]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/external/webrtc/+/e76bd3aa43821ff10f5cef585...