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}
diff --git a/webrtc/modules/audio_coding/main/acm2/acm_common_defs.h b/webrtc/modules/audio_coding/main/acm2/acm_common_defs.h
index bee10ca..85a287e 100644
--- a/webrtc/modules/audio_coding/main/acm2/acm_common_defs.h
+++ b/webrtc/modules/audio_coding/main/acm2/acm_common_defs.h
@@ -76,9 +76,6 @@
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_
diff --git a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
index c01466f..9c31832 100644
--- a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
+++ b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
@@ -122,6 +122,14 @@
}
} // 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 @@
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 @@
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 @@
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);
}
}
diff --git a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h
index 6ef80bc..19ca01b 100644
--- a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h
+++ b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h
@@ -250,6 +250,22 @@
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 @@
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.
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc b/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
index d28dcc6..cad6ee9 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
+++ b/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 @@
// 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;
}
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_owner.cc b/webrtc/modules/audio_coding/main/acm2/codec_owner.cc
index 5c1e37b..4d214be 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_owner.cc
+++ b/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 @@
&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 @@
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,
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc b/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc
index b9d4cdd..a1366a9 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc
+++ b/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc
@@ -100,8 +100,6 @@
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;
