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}
diff --git a/webrtc/modules/audio_coding/neteq/merge.cc b/webrtc/modules/audio_coding/neteq/merge.cc
index d50b665..9efbe4f 100644
--- a/webrtc/modules/audio_coding/neteq/merge.cc
+++ b/webrtc/modules/audio_coding/neteq/merge.cc
@@ -158,6 +158,7 @@
// 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 e119d94..2d91b8c 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc
@@ -1600,16 +1600,18 @@
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<int>(new_length) -
+ rtc::dchecked_cast<int>(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 03a788e..5399f2a 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
@@ -446,11 +446,11 @@
"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 @@
"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 b47ca12..c6d274f 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 @@
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 b2df865..fb869ab 100644
--- a/webrtc/modules/audio_coding/neteq/statistics_calculator.h
+++ b/webrtc/modules/audio_coding/neteq/statistics_calculator.h
@@ -45,6 +45,14 @@
// 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);
