From 93fb53acf57eef05edba416373f254430ebf1b22 Mon Sep 17 00:00:00 2001 From: "henrik.lundin" Date: Thu, 25 Jun 2015 13:03:06 -0700 Subject: [PATCH] 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} --- .../audio_coding/main/acm2/acm_common_defs.h | 3 --- .../main/acm2/audio_coding_module_impl.cc | 13 ++++++++++--- .../main/acm2/audio_coding_module_impl.h | 17 +++++++++++++++++ .../audio_coding/main/acm2/codec_manager.cc | 5 ----- .../audio_coding/main/acm2/codec_owner.cc | 6 ------ .../main/acm2/codec_owner_unittest.cc | 2 -- 6 files changed, 27 insertions(+), 19 deletions(-) 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 bee10caa7..85a287e12 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 @@ 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_ 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 c01466f8c..9c3183271 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 @@ 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); } } 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 6ef80bc66..19ca01bf9 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 @@ 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. diff --git a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc b/webrtc/modules/audio_coding/main/acm2/codec_manager.cc index d28dcc6ff..cad6ee908 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 @@ 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; } diff --git a/webrtc/modules/audio_coding/main/acm2/codec_owner.cc b/webrtc/modules/audio_coding/main/acm2/codec_owner.cc index 5c1e37b5b..4d214be24 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 @@ 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, 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 b9d4cdd7d..a1366a9b8 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, 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;