From 9da7c7480ba15080f14e91c955351e6b037507f4 Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Mon, 30 Oct 2017 10:05:10 +0100 Subject: [PATCH] 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 Reviewed-by: Henrik Lundin Cr-Commit-Position: refs/heads/master@{#20474} --- common_audio/resampler/include/resampler.h | 6 + common_audio/resampler/resampler.cc | 183 ++++++++++--------- common_audio/resampler/resampler_unittest.cc | 37 ++++ 3 files changed, 143 insertions(+), 83 deletions(-) diff --git a/common_audio/resampler/include/resampler.h b/common_audio/resampler/include/resampler.h index 4839bd32be..fec2c1a7bf 100644 --- a/common_audio/resampler/include/resampler.h +++ b/common_audio/resampler/include/resampler.h @@ -64,6 +64,12 @@ class Resampler { 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 47666c8afa..1c210dfcfc 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::ResetIfNeeded(int inFreq, int outFreq, size_t num_channels) { 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 @@ int Resampler::Reset(int inFreq, int outFreq, size_t num_channels) { 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 @@ int Resampler::Reset(int inFreq, int outFreq, size_t num_channels) { 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 4a53901a56..0300719637 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 + #include "common_audio/resampler/include/resampler.h" #include "test/gtest.h" @@ -50,6 +52,8 @@ class ResamplerTest : public testing::Test { 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::SetUp() { 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(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(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 @@ TEST_F(ResamplerTest, Stereo) { } } +// Try multiple resets between a few supported and unsupported rates. +TEST_F(ResamplerTest, MultipleResets) { + constexpr size_t kNumChanges = 5; + constexpr std::array kInRates = { + {8000, 44000, 44000, 32000, 32000}}; + constexpr std::array kOutRates = { + {16000, 48000, 48000, 16000, 16000}}; + constexpr std::array kNumChannels = {{2, 2, 2, 2, 1}}; + for (size_t i = 0; i < kNumChanges; ++i) { + ResetIfNeededAndPush(kInRates[i], kOutRates[i], kNumChannels[i]); + } +} + } // namespace } // namespace webrtc