Adding a new ChangeLogger class to handle UMA logging of bitrates

Adding a new ChangeLogger class to handle UMA logging of bitrates

This change introduces the sub-class ChangeLogger in AudioCodingModuleImpl. The class writes values to the named UMA histogram, but only if the value has changed since the last time (and always for the first call). This is to avoid the problem with audio codecs being registered but never used. Before this change, these codecs' bitrate was also logged, even though they were never used.

BUG=chromium:488124
R=kwiberg@webrtc.org

Committed: https://crrev.com/93fb53acf57eef05edba416373f254430ebf1b22
Cr-Commit-Position: refs/heads/master@{#9506}

[hlundin-webrtc, 2015-06-24 14:30:11]

Karl,

Please, take a look at this CL.

Thanks!

[kwiberg-webrtc, 2015-06-25 09:42:44]

lgtm, but see comments

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

https://codereview.webrtc.org/1203803004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:189:
bitrate_logger_.MaybeLogValue();
Sorry to come with an alternate suggestion at this stage, but if you do want to
suppress logging of the same value more than once in a row (which I asked about
in another comment), a simpler approach would be to replace DelayedLogger with a
DedupLogger class, and call

  bitrate_logger_.MaybeLogValue(audio_encoder_->GetTargetBitrate() / 1000);

here. That way, you wouldn't have to log in all the places where the target
bitrate can change, which would eliminate the possibility of missing one of
them.

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

https://codereview.webrtc.org/1203803004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/delayed_logger.cc:18: : value_(0),
log_on_next_call_(false), histogram_name_(histogram_name) {
You don't have to initialize value_. (The only significant cost of doing so is
the small while it takes the reader to realize that it isn't necessary.)

https://codereview.webrtc.org/1203803004/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/delayed_logger.h (right):

https://codereview.webrtc.org/1203803004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/delayed_logger.h:31: void SetValue(int
value);
It isn't clear from this description that a second call will overwrite the value
set by the first. Maybe change the first sentence to

  "Sets a new value locally in the DelayedLogger object (overwriting any
previous value)."

https://codereview.webrtc.org/1203803004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/delayed_logger.h:35: void MaybeLogValue();
You don't suppress logging of a value that's identical to the previous logged
value. Is that what you want for this application?

[hlundin-webrtc, 2015-06-25 13:59:25]

PTAL again. Probably best diffed versus base.

Thanks!

[kwiberg-webrtc, 2015-06-25 14:33:10]

lgtm if you update the commit message

[hlundin-webrtc, 2015-06-25 17:11:25]

Fixing a typo

[hlundin-webrtc, 2015-06-25 17:11:50]

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

[hlundin-webrtc, 2015-06-25 17:11:50]

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/1203803004/#ps40001 (title:
"Fixing a typo")

[commit-bot: I haz the power, 2015-06-25 17:12:15]

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

[commit-bot: I haz the power, 2015-06-25 19:12:43]

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

[commit-bot: I haz the power, 2015-06-25 19:12:44]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hlundin-webrtc, 2015-06-25 19:21:07]

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

[commit-bot: I haz the power, 2015-06-25 19:21:28]

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

[commit-bot: I haz the power, 2015-06-25 20:03:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-25 20:03:19]

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