Change Resampler to not change state on invalid Reset and ResetIfNeeded calls.
Adds a unittest to test this.
A Reset() with unsupported frequencies will fail, but currently leaves the resampler in an illegal state.
Subsequent calls to ResetIfNeeded(), which depends on the internal state, will then have unreliable behavior.
The following sequence of calls demonstrate this: It appears as though the resampler is successfully reinitialized to upsample from 44 kHz to 48 kHz, but will in fact crash on Push().
Resampler::Reset() with in=44000, out=32000 // Returns 0
Resampler::ResetIfNeeded() with in=44000, out=48000 // Returns -1
Resampler::ResetIfNeeded() with in=44000, out=48000 // Returns 0
Resampler::Push() with some data
Bug: webrtc:8426
Change-Id: Id1e0528ffcb7a86702d4c2f4c5103a1db419c07d
Reviewed-on: https://webrtc-review.googlesource.com/16424
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Henrik Lundin <henrik.lundin@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20474}diff --git a/common_audio/resampler/include/resampler.h b/common_audio/resampler/include/resampler.h
index 4839bd3..fec2c1a 100644
--- a/common_audio/resampler/include/resampler.h
+++ b/common_audio/resampler/include/resampler.h
@@ -64,6 +64,12 @@
kResamplerMode11To8
};
+ // Computes the resampler mode for a given sampling frequency pair.
+ // Returns -1 for unsupported frequency pairs.
+ static int ComputeResamplerMode(int in_freq_hz,
+ int out_freq_hz,
+ ResamplerMode* mode);
+
// Generic pointers since we don't know what states we'll need
void* state1_;
void* state2_;
diff --git a/common_audio/resampler/resampler.cc b/common_audio/resampler/resampler.cc
index 47666c8..1c210df 100644
--- a/common_audio/resampler/resampler.cc
+++ b/common_audio/resampler/resampler.cc
@@ -18,6 +18,7 @@
#include "common_audio/resampler/include/resampler.h"
#include "common_audio/signal_processing/include/signal_processing_library.h"
+#include "rtc_base/logging.h"
namespace webrtc {
@@ -83,9 +84,20 @@
int Resampler::Reset(int inFreq, int outFreq, size_t num_channels) {
if (num_channels != 1 && num_channels != 2) {
- return -1;
+ LOG(LS_WARNING)
+ << "Reset() called with unsupported channel count, num_channels = "
+ << num_channels;
+ return -1;
}
+ ResamplerMode mode;
+ if (ComputeResamplerMode(inFreq, outFreq, &mode) != 0) {
+ LOG(LS_WARNING) << "Reset() called with unsupported sample rates, inFreq = "
+ << inFreq << ", outFreq = " << outFreq;
+ return -1;
+ }
+ // Reinitialize internal state for the frequencies and sample rates.
num_channels_ = num_channels;
+ my_mode_ = mode;
if (state1_) {
free(state1_);
@@ -121,98 +133,17 @@
in_buffer_size_max_ = 0;
out_buffer_size_max_ = 0;
- // Start with a math exercise, Euclid's algorithm to find the gcd:
- int a = inFreq;
- int b = outFreq;
- int c = a % b;
- while (c != 0) {
- a = b;
- b = c;
- c = a % b;
- }
- // b is now the gcd;
-
// We need to track what domain we're in.
my_in_frequency_khz_ = inFreq / 1000;
my_out_frequency_khz_ = outFreq / 1000;
- // Scale with GCD
- inFreq = inFreq / b;
- outFreq = outFreq / b;
-
if (num_channels_ == 2) {
// Create two mono resamplers.
slave_left_ = new Resampler(inFreq, outFreq, 1);
slave_right_ = new Resampler(inFreq, outFreq, 1);
}
- if (inFreq == outFreq) {
- my_mode_ = kResamplerMode1To1;
- } else if (inFreq == 1) {
- switch (outFreq) {
- case 2:
- my_mode_ = kResamplerMode1To2;
- break;
- case 3:
- my_mode_ = kResamplerMode1To3;
- break;
- case 4:
- my_mode_ = kResamplerMode1To4;
- break;
- case 6:
- my_mode_ = kResamplerMode1To6;
- break;
- case 12:
- my_mode_ = kResamplerMode1To12;
- break;
- default:
- return -1;
- }
- } else if (outFreq == 1) {
- switch (inFreq) {
- case 2:
- my_mode_ = kResamplerMode2To1;
- break;
- case 3:
- my_mode_ = kResamplerMode3To1;
- break;
- case 4:
- my_mode_ = kResamplerMode4To1;
- break;
- case 6:
- my_mode_ = kResamplerMode6To1;
- break;
- case 12:
- my_mode_ = kResamplerMode12To1;
- break;
- default:
- return -1;
- }
- } else if ((inFreq == 2) && (outFreq == 3)) {
- my_mode_ = kResamplerMode2To3;
- } else if ((inFreq == 2) && (outFreq == 11)) {
- my_mode_ = kResamplerMode2To11;
- } else if ((inFreq == 4) && (outFreq == 11)) {
- my_mode_ = kResamplerMode4To11;
- } else if ((inFreq == 8) && (outFreq == 11)) {
- my_mode_ = kResamplerMode8To11;
- } else if ((inFreq == 3) && (outFreq == 2)) {
- my_mode_ = kResamplerMode3To2;
- } else if ((inFreq == 11) && (outFreq == 2)) {
- my_mode_ = kResamplerMode11To2;
- } else if ((inFreq == 11) && (outFreq == 4)) {
- my_mode_ = kResamplerMode11To4;
- } else if ((inFreq == 11) && (outFreq == 16)) {
- my_mode_ = kResamplerMode11To16;
- } else if ((inFreq == 11) && (outFreq == 32)) {
- my_mode_ = kResamplerMode11To32;
- } else if ((inFreq == 11) && (outFreq == 8)) {
- my_mode_ = kResamplerMode11To8;
- } else {
- return -1;
- }
-
- // Now create the states we need
+ // Now create the states we need.
switch (my_mode_) {
case kResamplerMode1To1:
// No state needed;
@@ -376,6 +307,92 @@
return 0;
}
+int Resampler::ComputeResamplerMode(int in_freq_hz,
+ int out_freq_hz,
+ ResamplerMode* mode) {
+ // Start with a math exercise, Euclid's algorithm to find the gcd:
+ int a = in_freq_hz;
+ int b = out_freq_hz;
+ int c = a % b;
+ while (c != 0) {
+ a = b;
+ b = c;
+ c = a % b;
+ }
+ // b is now the gcd;
+
+ // Scale with GCD
+ const int reduced_in_freq = in_freq_hz / b;
+ const int reduced_out_freq = out_freq_hz / b;
+
+ if (reduced_in_freq == reduced_out_freq) {
+ *mode = kResamplerMode1To1;
+ } else if (reduced_in_freq == 1) {
+ switch (reduced_out_freq) {
+ case 2:
+ *mode = kResamplerMode1To2;
+ break;
+ case 3:
+ *mode = kResamplerMode1To3;
+ break;
+ case 4:
+ *mode = kResamplerMode1To4;
+ break;
+ case 6:
+ *mode = kResamplerMode1To6;
+ break;
+ case 12:
+ *mode = kResamplerMode1To12;
+ break;
+ default:
+ return -1;
+ }
+ } else if (reduced_out_freq == 1) {
+ switch (reduced_in_freq) {
+ case 2:
+ *mode = kResamplerMode2To1;
+ break;
+ case 3:
+ *mode = kResamplerMode3To1;
+ break;
+ case 4:
+ *mode = kResamplerMode4To1;
+ break;
+ case 6:
+ *mode = kResamplerMode6To1;
+ break;
+ case 12:
+ *mode = kResamplerMode12To1;
+ break;
+ default:
+ return -1;
+ }
+ } else if ((reduced_in_freq == 2) && (reduced_out_freq == 3)) {
+ *mode = kResamplerMode2To3;
+ } else if ((reduced_in_freq == 2) && (reduced_out_freq == 11)) {
+ *mode = kResamplerMode2To11;
+ } else if ((reduced_in_freq == 4) && (reduced_out_freq == 11)) {
+ *mode = kResamplerMode4To11;
+ } else if ((reduced_in_freq == 8) && (reduced_out_freq == 11)) {
+ *mode = kResamplerMode8To11;
+ } else if ((reduced_in_freq == 3) && (reduced_out_freq == 2)) {
+ *mode = kResamplerMode3To2;
+ } else if ((reduced_in_freq == 11) && (reduced_out_freq == 2)) {
+ *mode = kResamplerMode11To2;
+ } else if ((reduced_in_freq == 11) && (reduced_out_freq == 4)) {
+ *mode = kResamplerMode11To4;
+ } else if ((reduced_in_freq == 11) && (reduced_out_freq == 16)) {
+ *mode = kResamplerMode11To16;
+ } else if ((reduced_in_freq == 11) && (reduced_out_freq == 32)) {
+ *mode = kResamplerMode11To32;
+ } else if ((reduced_in_freq == 11) && (reduced_out_freq == 8)) {
+ *mode = kResamplerMode11To8;
+ } else {
+ return -1;
+ }
+ return 0;
+}
+
// Synchronous resampling, all output samples are written to samplesOut
int Resampler::Push(const int16_t * samplesIn, size_t lengthIn,
int16_t* samplesOut, size_t maxLen, size_t& outLen) {
diff --git a/common_audio/resampler/resampler_unittest.cc b/common_audio/resampler/resampler_unittest.cc
index 4a53901..0300719 100644
--- a/common_audio/resampler/resampler_unittest.cc
+++ b/common_audio/resampler/resampler_unittest.cc
@@ -8,6 +8,8 @@
* be found in the AUTHORS file in the root of the source tree.
*/
+#include <array>
+
#include "common_audio/resampler/include/resampler.h"
#include "test/gtest.h"
@@ -50,6 +52,8 @@
virtual void SetUp();
virtual void TearDown();
+ void ResetIfNeededAndPush(int in_rate, int out_rate, int num_channels);
+
Resampler rs_;
int16_t data_in_[kDataSize];
int16_t data_out_[kDataSize];
@@ -64,6 +68,26 @@
void ResamplerTest::TearDown() {}
+void ResamplerTest::ResetIfNeededAndPush(int in_rate,
+ int out_rate,
+ int num_channels) {
+ std::ostringstream ss;
+ ss << "Input rate: " << in_rate << ", output rate: " << out_rate
+ << ", channel count: " << num_channels;
+ SCOPED_TRACE(ss.str());
+
+ if (ValidRates(in_rate, out_rate)) {
+ size_t in_length = static_cast<size_t>(in_rate / 100);
+ size_t out_length = 0;
+ EXPECT_EQ(0, rs_.ResetIfNeeded(in_rate, out_rate, num_channels));
+ EXPECT_EQ(0,
+ rs_.Push(data_in_, in_length, data_out_, kDataSize, out_length));
+ EXPECT_EQ(static_cast<size_t>(out_rate / 100), out_length);
+ } else {
+ EXPECT_EQ(-1, rs_.ResetIfNeeded(in_rate, out_rate, num_channels));
+ }
+}
+
TEST_F(ResamplerTest, Reset) {
// The only failure mode for the constructor is if Reset() fails. For the
// time being then (until an Init function is added), we rely on Reset()
@@ -134,5 +158,18 @@
}
}
+// Try multiple resets between a few supported and unsupported rates.
+TEST_F(ResamplerTest, MultipleResets) {
+ constexpr size_t kNumChanges = 5;
+ constexpr std::array<int, kNumChanges> kInRates = {
+ {8000, 44000, 44000, 32000, 32000}};
+ constexpr std::array<int, kNumChanges> kOutRates = {
+ {16000, 48000, 48000, 16000, 16000}};
+ constexpr std::array<int, kNumChanges> kNumChannels = {{2, 2, 2, 2, 1}};
+ for (size_t i = 0; i < kNumChanges; ++i) {
+ ResetIfNeededAndPush(kInRates[i], kOutRates[i], kNumChannels[i]);
+ }
+}
+
} // namespace
} // namespace webrtc
