From 2979f55f95ec71425a32fca31c50c9cbd71ad738 Mon Sep 17 00:00:00 2001 From: "henrik.lundin" Date: Fri, 5 May 2017 05:04:16 -0700 Subject: [PATCH] NetEq: Fix a bug in expand_rate and speech_expand_rate calculation After a Merge operation, the statistics for number of samples generated using Expand must be corrected, and the correction can in fact be negative. However, a bug was introduced in https://codereview.webrtc.org/1230503003 which uses a size_t to represent the correction, which leads to wrap-around of the negative value. This is not a problem in itself, since this value is added to another size_t, with the effect that the desired subtraction happens anyway. The actual problem arises if the statistics are polled/reset before a subtraction happens -- that is, between an Expand and a Merge operation. This will lead to an actual wrap-around of the stats value, and large expand_rate (16384) is reported. BUG=webrtc:7554 Review-Url: https://codereview.webrtc.org/2859483005 Cr-Commit-Position: refs/heads/master@{#18029} --- webrtc/modules/audio_coding/neteq/merge.cc | 1 + .../modules/audio_coding/neteq/neteq_impl.cc | 10 ++++++---- .../audio_coding/neteq/neteq_unittest.cc | 20 +++++++++---------- .../neteq/statistics_calculator.cc | 20 +++++++++++++++++++ .../neteq/statistics_calculator.h | 8 ++++++++ 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/webrtc/modules/audio_coding/neteq/merge.cc b/webrtc/modules/audio_coding/neteq/merge.cc index d50b66588a..9efbe4fb9c 100644 --- a/webrtc/modules/audio_coding/neteq/merge.cc +++ b/webrtc/modules/audio_coding/neteq/merge.cc @@ -158,6 +158,7 @@ size_t Merge::Process(int16_t* input, size_t input_length, // Return new added length. |old_length| samples were borrowed from // |sync_buffer_|. + RTC_DCHECK_GE(output_length, old_length); return output_length - old_length; } diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc index e119d9439c..2d91b8c1c5 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc @@ -1600,16 +1600,18 @@ void NetEqImpl::DoMerge(int16_t* decoded_buffer, size_t decoded_length, size_t new_length = merge_->Process(decoded_buffer, decoded_length, mute_factor_array_.get(), algorithm_buffer_.get()); - size_t expand_length_correction = new_length - - decoded_length / algorithm_buffer_->Channels(); + // Correction can be negative. + int expand_length_correction = + rtc::dchecked_cast(new_length) - + rtc::dchecked_cast(decoded_length / algorithm_buffer_->Channels()); // Update in-call and post-call statistics. if (expand_->MuteFactor(0) == 0) { // Expand generates only noise. - stats_.ExpandedNoiseSamples(expand_length_correction); + stats_.ExpandedNoiseSamplesCorrection(expand_length_correction); } else { // Expansion generates more than only noise. - stats_.ExpandedVoiceSamples(expand_length_correction); + stats_.ExpandedVoiceSamplesCorrection(expand_length_correction); } last_mode_ = kModeMerge; diff --git a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc index 03a788eed0..5399f2aae6 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc @@ -446,11 +446,11 @@ TEST_F(NetEqDecodingTest, MAYBE_TestBitExactness) { "09fa7646e2ad032a0b156177b95f09012430f81f", "759fef89a5de52bd17e733dc255c671ce86be909"); - const std::string network_stats_checksum = PlatformChecksum( - "f59b3dfdb9b1b8bbb61abedd7c8cf3fc47c21f5f", - "c8b2a93842e48d014f7e6efe10ae96cb3892b129", - "f59b3dfdb9b1b8bbb61abedd7c8cf3fc47c21f5f", - "f59b3dfdb9b1b8bbb61abedd7c8cf3fc47c21f5f"); + const std::string network_stats_checksum = + PlatformChecksum("f7c2158761a531dd2804d13da0480033faa7be12", + "8b5e3c8247dce48cb33923eaa1a502ca91429d5e", + "f7c2158761a531dd2804d13da0480033faa7be12", + "f7c2158761a531dd2804d13da0480033faa7be12"); const std::string rtcp_stats_checksum = PlatformChecksum( "b8880bf9fed2487efbddcb8d94b9937a29ae521d", @@ -483,11 +483,11 @@ TEST_F(NetEqDecodingTest, MAYBE_TestOpusBitExactness) { "6237dd113ad80d7764fe4c90b55b2ec035eae64e", "6237dd113ad80d7764fe4c90b55b2ec035eae64e"); - const std::string network_stats_checksum = PlatformChecksum( - "d8379381d5a619f0616bb3c0a8a9eea1704a8ab8", - "d8379381d5a619f0616bb3c0a8a9eea1704a8ab8", - "d8379381d5a619f0616bb3c0a8a9eea1704a8ab8", - "d8379381d5a619f0616bb3c0a8a9eea1704a8ab8"); + const std::string network_stats_checksum = + PlatformChecksum("0869a450a819b14bf2a91eb6f3629a3421d17606", + "0869a450a819b14bf2a91eb6f3629a3421d17606", + "0869a450a819b14bf2a91eb6f3629a3421d17606", + "0869a450a819b14bf2a91eb6f3629a3421d17606"); const std::string rtcp_stats_checksum = PlatformChecksum( "e37c797e3de6a64dda88c9ade7a013d022a2e1e0", diff --git a/webrtc/modules/audio_coding/neteq/statistics_calculator.cc b/webrtc/modules/audio_coding/neteq/statistics_calculator.cc index b47ca127db..c6d274f432 100644 --- a/webrtc/modules/audio_coding/neteq/statistics_calculator.cc +++ b/webrtc/modules/audio_coding/neteq/statistics_calculator.cc @@ -22,6 +22,16 @@ namespace webrtc { +namespace { +size_t AddIntToSizeTWithLowerCap(int a, size_t b) { + const size_t ret = b + a; + // If a + b is negative, resulting in a negative wrap, cap it to zero instead. + static_assert(sizeof(size_t) >= sizeof(int), + "int must not be wider than size_t for this to work"); + return (a < 0 && ret > b) ? 0 : ret; +} +} // namespace + // Allocating the static const so that it can be passed by reference to // RTC_DCHECK. const size_t StatisticsCalculator::kLenWaitingTimes; @@ -148,6 +158,16 @@ void StatisticsCalculator::ExpandedNoiseSamples(size_t num_samples) { expanded_noise_samples_ += num_samples; } +void StatisticsCalculator::ExpandedVoiceSamplesCorrection(int num_samples) { + expanded_speech_samples_ = + AddIntToSizeTWithLowerCap(num_samples, expanded_speech_samples_); +} + +void StatisticsCalculator::ExpandedNoiseSamplesCorrection(int num_samples) { + expanded_noise_samples_ = + AddIntToSizeTWithLowerCap(num_samples, expanded_noise_samples_); +} + void StatisticsCalculator::PreemptiveExpandedSamples(size_t num_samples) { preemptive_samples_ += num_samples; } diff --git a/webrtc/modules/audio_coding/neteq/statistics_calculator.h b/webrtc/modules/audio_coding/neteq/statistics_calculator.h index b2df865f63..fb869ab025 100644 --- a/webrtc/modules/audio_coding/neteq/statistics_calculator.h +++ b/webrtc/modules/audio_coding/neteq/statistics_calculator.h @@ -45,6 +45,14 @@ class StatisticsCalculator { // that the expansion produced only noise samples. void ExpandedNoiseSamples(size_t num_samples); + // Corrects the statistics for number of samples produced through non-noise + // expansion by adding |num_samples| (negative or positive) to the current + // value. The result is capped to zero to avoid negative values. + void ExpandedVoiceSamplesCorrection(int num_samples); + + // Same as ExpandedVoiceSamplesCorrection but for noise samples. + void ExpandedNoiseSamplesCorrection(int num_samples); + // Reports that |num_samples| samples were produced through preemptive // expansion. void PreemptiveExpandedSamples(size_t num_samples);