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

Review URL: https://codereview.webrtc.org/1203803004

Cr-Commit-Position: refs/heads/master@{#9506}

webrtc/modules/audio_coding/main/acm2/acm_common_defs.h

@@ -76,9 +76,6 @@ struct WebRtcACMAudioBuff {
   uint32_t last_in_timestamp;
 };
 
-#define HISTOGRAM_NAME_AUDIO_TARGET_BITRATE_IN_KBPS \
-  "WebRTC.Audio.TargetBitrateInKbps"
-
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_CODING_MAIN_ACM2_ACM_COMMON_DEFS_H_

webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc

@@ -122,6 +122,14 @@ void ConvertEncodedInfoToFragmentationHeader(
 }
 }  // namespace
 
+void AudioCodingModuleImpl::ChangeLogger::MaybeLog(int value) {
+  if (value != last_value_ || first_time_) {
+    first_time_ = false;
+    last_value_ = value;
+    RTC_HISTOGRAM_COUNTS_100(histogram_name_, value);
+  }
+}
+
 AudioCodingModuleImpl::AudioCodingModuleImpl(
     const AudioCodingModule::Config& config)
     : acm_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
@@ -129,6 +137,7 @@ AudioCodingModuleImpl::AudioCodingModuleImpl(
       expected_codec_ts_(0xD87F3F9F),
       expected_in_ts_(0xD87F3F9F),
       receiver_(config),
+      bitrate_logger_("WebRTC.Audio.TargetBitrateInKbps"),
       previous_pltype_(255),
       aux_rtp_header_(NULL),
       receiver_initialized_(false),
@@ -185,6 +194,7 @@ int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) {
   encoded_info = audio_encoder->Encode(rtp_timestamp, input_data.audio,
                                        input_data.length_per_channel,
                                        sizeof(stream), stream);
+  bitrate_logger_.MaybeLog(audio_encoder->GetTargetBitrate() / 1000);
   if (encoded_info.encoded_bytes == 0 && !encoded_info.send_even_if_empty) {
     // Not enough data.
     return 0;
@@ -296,9 +306,6 @@ void AudioCodingModuleImpl::SetBitRate(int bitrate_bps) {
   CriticalSectionScoped lock(acm_crit_sect_);
   if (codec_manager_.CurrentEncoder()) {
     codec_manager_.CurrentEncoder()->SetTargetBitrate(bitrate_bps);
-    RTC_HISTOGRAM_COUNTS_100(
-        HISTOGRAM_NAME_AUDIO_TARGET_BITRATE_IN_KBPS,
-        codec_manager_.CurrentEncoder()->GetTargetBitrate() / 1000);
   }
 }
 

webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h

@@ -250,6 +250,22 @@ class AudioCodingModuleImpl : public AudioCodingModule {
     int16_t buffer[WEBRTC_10MS_PCM_AUDIO];
   };
 
+  // This member 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).
+  class ChangeLogger {
+   public:
+    explicit ChangeLogger(const std::string& histogram_name)
+        : histogram_name_(histogram_name) {}
+    // Logs the new value if it is different from the last logged value, or if
+    // this is the first call.
+    void MaybeLog(int value);
+
+   private:
+    int last_value_ = 0;
+    int first_time_ = true;
+    const std::string histogram_name_;
+  };
+
   int Add10MsDataInternal(const AudioFrame& audio_frame, InputData* input_data)
       EXCLUSIVE_LOCKS_REQUIRED(acm_crit_sect_);
   int Encode(const InputData& input_data)
@@ -285,6 +301,7 @@ class AudioCodingModuleImpl : public AudioCodingModule {
   uint32_t expected_in_ts_ GUARDED_BY(acm_crit_sect_);
   ACMResampler resampler_ GUARDED_BY(acm_crit_sect_);
   AcmReceiver receiver_;  // AcmReceiver has it's own internal lock.
+  ChangeLogger bitrate_logger_ GUARDED_BY(acm_crit_sect_);
   CodecManager codec_manager_ GUARDED_BY(acm_crit_sect_);
 
   // This is to keep track of CN instances where we can send DTMFs.

webrtc/modules/audio_coding/main/acm2/codec_manager.cc

@@ -13,8 +13,6 @@
 #include "webrtc/base/checks.h"
 #include "webrtc/engine_configurations.h"
 #include "webrtc/modules/audio_coding/main/acm2/acm_codec_database.h"
-#include "webrtc/modules/audio_coding/main/acm2/acm_common_defs.h"
-#include "webrtc/system_wrappers/interface/metrics.h"
 #include "webrtc/system_wrappers/interface/trace.h"
 
 namespace webrtc {
@@ -314,9 +312,6 @@ int CodecManager::RegisterEncoder(const CodecInst& send_codec) {
   // Check if a change in Rate is required.
   if (send_codec.rate != send_codec_inst_.rate) {
     codec_owner_.SpeechEncoder()->SetTargetBitrate(send_codec.rate);
-    RTC_HISTOGRAM_COUNTS_100(
-        HISTOGRAM_NAME_AUDIO_TARGET_BITRATE_IN_KBPS,
-        codec_owner_.SpeechEncoder()->GetTargetBitrate() / 1000);
     send_codec_inst_.rate = send_codec.rate;
   }
 

webrtc/modules/audio_coding/main/acm2/codec_owner.cc

@@ -21,8 +21,6 @@
 #include "webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h"
 #include "webrtc/modules/audio_coding/codecs/pcm16b/include/audio_encoder_pcm16b.h"
 #include "webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h"
-#include "webrtc/modules/audio_coding/main/acm2/acm_common_defs.h"
-#include "webrtc/system_wrappers/interface/metrics.h"
 
 namespace webrtc {
 namespace acm2 {
@@ -182,8 +180,6 @@ void CodecOwner::SetEncoders(const CodecInst& speech_inst,
                       &isac_is_encoder_);
   external_speech_encoder_ = nullptr;
   ChangeCngAndRed(cng_payload_type, vad_mode, red_payload_type);
-  RTC_HISTOGRAM_COUNTS_100(HISTOGRAM_NAME_AUDIO_TARGET_BITRATE_IN_KBPS,
-                           SpeechEncoder()->GetTargetBitrate() / 1000);
 }
 
 void CodecOwner::SetEncoders(AudioEncoderMutable* external_speech_encoder,
@@ -194,8 +190,6 @@ void CodecOwner::SetEncoders(AudioEncoderMutable* external_speech_encoder,
   speech_encoder_.reset();
   isac_is_encoder_ = false;
   ChangeCngAndRed(cng_payload_type, vad_mode, red_payload_type);
-  RTC_HISTOGRAM_COUNTS_100(HISTOGRAM_NAME_AUDIO_TARGET_BITRATE_IN_KBPS,
-                           SpeechEncoder()->GetTargetBitrate() / 1000);
 }
 
 void CodecOwner::ChangeCngAndRed(int cng_payload_type,

webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc

@@ -100,8 +100,6 @@ TEST_F(CodecOwnerTest, VerifyCngFrames) {
 
 TEST_F(CodecOwnerTest, ExternalEncoder) {
   MockAudioEncoderMutable external_encoder;
-  EXPECT_CALL(external_encoder, GetTargetBitrate())
-      .WillRepeatedly(Return(-1));  // Flag adaptive bitrate (doesn't matter).
   codec_owner_.SetEncoders(&external_encoder, -1, VADNormal, -1);
   const int kSampleRateHz = 8000;
   const int kPacketSizeSamples = kSampleRateHz / 100;