Add UMA logging for target audio bitrate

Add UMA logging for target audio bitrate

This CL logs the target audio bitrate to a UMA histogram called
WebRTC.Audio.TargetBitrateInKbps. It logs the rate when a codec is
created, and when the target is explicitly updated. Note that since
each codec implementation is free to change or ignore the target
value, there is no guarantee that the logged value will actually be
used as the target.

BUG=chromium:488124
Committed: https://crrev.com/6b4a564d21cac99edfa4ab1cb95d525d40855141
Cr-Commit-Position: refs/heads/master@{#9484}

[hlundin-webrtc, 2015-06-11 09:39:10]

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

[hlundin-webrtc, 2015-06-11 09:39:10]

henrik.lundin@webrtc.org changed required reviewers:
+ kwiberg@webrtc.org

[hlundin-webrtc, 2015-06-11 09:39:33]

Karl,

Please, take a look.

Thanks!

[kwiberg-webrtc, 2015-06-11 10:03:14]

lgtm

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right):

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:292:
RTC_HISTOGRAM_COUNTS_100("WebRTC.Audio.TargetBitrateInKbps",
Define a constant for this string? You use it in 3 places, and am I correct in
assuming that a misspelling wouldn't trigger a compile-time error?

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:295:
codec_manager_.CurrentEncoder()->SetTargetBitrate(bitrate_bps);
If this method would just return the bitrate the encoder actually decided to
use, we could log that too. Would that be useful?

[hlundin-webrtc, 2015-06-11 10:11:48]

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right):

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:292:
RTC_HISTOGRAM_COUNTS_100("WebRTC.Audio.TargetBitrateInKbps",
On 2015/06/11 10:03:13, kwiberg1 wrote:
> Define a constant for this string? You use it in 3 places, and am I correct in
> assuming that a misspelling wouldn't trigger a compile-time error?

Yes, that might be a good idea. Any suggestion on where to store it?

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:295:
codec_manager_.CurrentEncoder()->SetTargetBitrate(bitrate_bps);
On 2015/06/11 10:03:13, kwiberg1 wrote:
> If this method would just return the bitrate the encoder actually decided to
> use, we could log that too. Would that be useful?

That could be useful. But it would mean that it must be implemented by all
codecs, not only by those who support changing rates.

[kwiberg-webrtc, 2015-06-11 11:01:25]

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right):

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:292:
RTC_HISTOGRAM_COUNTS_100("WebRTC.Audio.TargetBitrateInKbps",
On 2015/06/11 10:11:48, hlundin-webrtc wrote:
> On 2015/06/11 10:03:13, kwiberg1 wrote:
> > Define a constant for this string? You use it in 3 places, and am I correct
in
> > assuming that a misspelling wouldn't trigger a compile-time error?
> 
> Yes, that might be a good idea. Any suggestion on where to store it?

I think my preferred solution would be for all of WebRTC to define all of these
constants in a central place---metrics.h, say. But something like
acm_common_defs.h might also work.

https://codereview.webrtc.org/1178053002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:295:
codec_manager_.CurrentEncoder()->SetTargetBitrate(bitrate_bps);
On 2015/06/11 10:11:48, hlundin-webrtc wrote:
> On 2015/06/11 10:03:13, kwiberg1 wrote:
> > If this method would just return the bitrate the encoder actually decided to
> > use, we could log that too. Would that be useful?
> 
> That could be useful. But it would mean that it must be implemented by all
> codecs, not only by those who support changing rates.

Yes---but the implementation would be trivial, so it could be worth it.

But we also set the bitrate when constructing encoders (see the codec_owner.cc
diff in this CL), so maybe a dedicated method just for inquiring about the
target bitrate would be better.

[hlundin-webrtc, 2015-06-16 11:36:31]

Define histogram name in a constant

[hlundin-webrtc, 2015-06-18 13:19:55]

Rebase

[hlundin-webrtc, 2015-06-18 13:23:04]

Using GetTargetBitrate

[hlundin-webrtc, 2015-06-18 13:26:38]

Karl,

Care to take another look? For reference, you reviewed patch set 1 last time.

Thanks!

[kwiberg-webrtc, 2015-06-18 14:35:52]

https://codereview.webrtc.org/1178053002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/acm_common_defs.h (right):

https://codereview.webrtc.org/1178053002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_common_defs.h:80:
"WebRTC.Audio.TargetBitrateInKbps";
This will make a new copy of the string in each translation unit that includes
the header.

[hlundin-webrtc, 2015-06-22 11:00:31]

PTAL again.

https://codereview.webrtc.org/1178053002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/acm_common_defs.h (right):

https://codereview.webrtc.org/1178053002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_common_defs.h:80:
"WebRTC.Audio.TargetBitrateInKbps";
On 2015/06/18 14:35:52, kwiberg1 wrote:
> This will make a new copy of the string in each translation unit that includes
> the header.

Changed to #define.

[kwiberg-webrtc, 2015-06-22 11:05:46]

lgtm

[hlundin-webrtc, 2015-06-22 12:32:19]

Fixing a problem that was revealed in tests, and silencing a few warnings

[hlundin-webrtc, 2015-06-22 12:33:13]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2015-06-22 12:33:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1178053002/#ps100001 (title:
"Fixing a problem that was revealed in tests, and silencing a few warnings")

[commit-bot: I haz the power, 2015-06-22 12:33:21]

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

[commit-bot: I haz the power, 2015-06-22 13:35:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-22 13:35:32]

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