Replace the old resampler with SincResampler in the voice engine signal path.
* The old resampler was found to have a wraparound bug.
* Remove support for the old resampler from PushResampler.
* Use PushResampler in AudioCodingModule.
* The old resampler must still be removed from the file utility.
BUG=webrtc:1867,webrtc:827
TESTED=unit tests, Chrome using apprtc and voe_cmd_test to verify wrap-around is corrected, voe_cmd_test running through all supported codec sample rates and channels to verify good quality audio
R=henrika@webrtc.org, turaj@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/1590004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@4156 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/common_audio/resampler/include/push_resampler.h b/webrtc/common_audio/resampler/include/push_resampler.h
index 0183f91..770a992 100644
--- a/webrtc/common_audio/resampler/include/push_resampler.h
+++ b/webrtc/common_audio/resampler/include/push_resampler.h
@@ -16,12 +16,10 @@
namespace webrtc {
-class Resampler;
class PushSincResampler;
-// Wraps the old resampler and new arbitrary rate conversion resampler. The
-// old resampler will be used whenever it supports the requested rates, and
-// otherwise the sinc resampler will be enabled.
+// Wraps PushSincResampler to provide stereo support.
+// TODO(ajm): add support for an arbitrary number of channels.
class PushResampler {
public:
PushResampler();
@@ -37,19 +35,15 @@
int Resample(const int16_t* src, int src_length, int16_t* dst,
int dst_capacity);
- bool use_sinc_resampler() const { return use_sinc_resampler_; }
-
private:
int ResampleSinc(const int16_t* src, int src_length, int16_t* dst,
int dst_capacity);
- scoped_ptr<Resampler> resampler_;
scoped_ptr<PushSincResampler> sinc_resampler_;
scoped_ptr<PushSincResampler> sinc_resampler_right_;
int src_sample_rate_hz_;
int dst_sample_rate_hz_;
int num_channels_;
- bool use_sinc_resampler_;
scoped_array<int16_t> src_left_;
scoped_array<int16_t> src_right_;
scoped_array<int16_t> dst_left_;
diff --git a/webrtc/common_audio/resampler/push_resampler.cc b/webrtc/common_audio/resampler/push_resampler.cc
index 6c59e0b..8fdf638 100644
--- a/webrtc/common_audio/resampler/push_resampler.cc
+++ b/webrtc/common_audio/resampler/push_resampler.cc
@@ -19,14 +19,11 @@
namespace webrtc {
PushResampler::PushResampler()
- // Requires valid values at construction, so give it something arbitrary.
- : resampler_(new Resampler(48000, 48000, kResamplerSynchronous)),
- sinc_resampler_(NULL),
+ : sinc_resampler_(NULL),
sinc_resampler_right_(NULL),
src_sample_rate_hz_(0),
dst_sample_rate_hz_(0),
num_channels_(0),
- use_sinc_resampler_(false),
src_left_(NULL),
src_right_(NULL),
dst_left_(NULL),
@@ -55,16 +52,6 @@
dst_sample_rate_hz_ = dst_sample_rate_hz;
num_channels_ = num_channels;
- const ResamplerType resampler_type =
- num_channels == 1 ? kResamplerSynchronous : kResamplerSynchronousStereo;
- if (resampler_->Reset(src_sample_rate_hz, dst_sample_rate_hz,
- resampler_type) == 0) {
- // The resampler supports these rates.
- use_sinc_resampler_ = false;
- return 0;
- }
-
- use_sinc_resampler_ = true;
const int src_size_10ms_mono = src_sample_rate_hz / 100;
const int dst_size_10ms_mono = dst_sample_rate_hz / 100;
sinc_resampler_.reset(new PushSincResampler(src_size_10ms_mono,
@@ -89,20 +76,6 @@
return -1;
}
- if (use_sinc_resampler_) {
- return ResampleSinc(src, src_length, dst, dst_capacity);
- }
-
- int resulting_length = 0;
- if (resampler_->Push(src, src_length, dst, dst_capacity,
- resulting_length) != 0) {
- return -1;
- }
- return resulting_length;
-}
-
-int PushResampler::ResampleSinc(const int16_t* src, int src_length,
- int16_t* dst, int dst_capacity) {
if (src_sample_rate_hz_ == dst_sample_rate_hz_) {
// The old resampler provides this memcpy facility in the case of matching
// sample rates, so reproduce it here for the sinc resampler.
diff --git a/webrtc/common_audio/resampler/push_resampler_unittest.cc b/webrtc/common_audio/resampler/push_resampler_unittest.cc
index 6b60d05..c40923b 100644
--- a/webrtc/common_audio/resampler/push_resampler_unittest.cc
+++ b/webrtc/common_audio/resampler/push_resampler_unittest.cc
@@ -15,93 +15,14 @@
namespace webrtc {
-typedef std::tr1::tuple<int, int, bool> PushResamplerTestData;
-class PushResamplerTest
- : public testing::TestWithParam<PushResamplerTestData> {
- public:
- PushResamplerTest()
- : input_rate_(std::tr1::get<0>(GetParam())),
- output_rate_(std::tr1::get<1>(GetParam())),
- use_sinc_resampler_(std::tr1::get<2>(GetParam())) {
- }
-
- virtual ~PushResamplerTest() {}
-
- protected:
- int input_rate_;
- int output_rate_;
- bool use_sinc_resampler_;
-};
-
-TEST_P(PushResamplerTest, SincResamplerOnlyUsedWhenNecessary) {
+TEST(PushResamplerTest, VerifiesInputParameters) {
PushResampler resampler;
- resampler.InitializeIfNeeded(input_rate_, output_rate_, 1);
- EXPECT_EQ(use_sinc_resampler_, resampler.use_sinc_resampler());
+ EXPECT_EQ(-1, resampler.InitializeIfNeeded(-1, 16000, 1));
+ EXPECT_EQ(-1, resampler.InitializeIfNeeded(16000, -1, 1));
+ EXPECT_EQ(-1, resampler.InitializeIfNeeded(16000, 16000, 0));
+ EXPECT_EQ(-1, resampler.InitializeIfNeeded(16000, 16000, 3));
+ EXPECT_EQ(0, resampler.InitializeIfNeeded(16000, 16000, 1));
+ EXPECT_EQ(0, resampler.InitializeIfNeeded(16000, 16000, 2));
}
-INSTANTIATE_TEST_CASE_P(
- PushResamplerTest, PushResamplerTest, testing::Values(
- // To 8 kHz
- std::tr1::make_tuple(8000, 8000, false),
- std::tr1::make_tuple(16000, 8000, false),
- std::tr1::make_tuple(32000, 8000, false),
- std::tr1::make_tuple(44100, 8000, true),
- std::tr1::make_tuple(48000, 8000, false),
- std::tr1::make_tuple(96000, 8000, false),
- std::tr1::make_tuple(192000, 8000, true),
-
- // To 16 kHz
- std::tr1::make_tuple(8000, 16000, false),
- std::tr1::make_tuple(16000, 16000, false),
- std::tr1::make_tuple(32000, 16000, false),
- std::tr1::make_tuple(44100, 16000, true),
- std::tr1::make_tuple(48000, 16000, false),
- std::tr1::make_tuple(96000, 16000, false),
- std::tr1::make_tuple(192000, 16000, false),
-
- // To 32 kHz
- std::tr1::make_tuple(8000, 32000, false),
- std::tr1::make_tuple(16000, 32000, false),
- std::tr1::make_tuple(32000, 32000, false),
- std::tr1::make_tuple(44100, 32000, true),
- std::tr1::make_tuple(48000, 32000, false),
- std::tr1::make_tuple(96000, 32000, false),
- std::tr1::make_tuple(192000, 32000, false),
-
- // To 44.1kHz
- std::tr1::make_tuple(8000, 44100, true),
- std::tr1::make_tuple(16000, 44100, true),
- std::tr1::make_tuple(32000, 44100, true),
- std::tr1::make_tuple(44100, 44100, false),
- std::tr1::make_tuple(48000, 44100, true),
- std::tr1::make_tuple(96000, 44100, true),
- std::tr1::make_tuple(192000, 44100, true),
-
- // To 48kHz
- std::tr1::make_tuple(8000, 48000, false),
- std::tr1::make_tuple(16000, 48000, false),
- std::tr1::make_tuple(32000, 48000, false),
- std::tr1::make_tuple(44100, 48000, true),
- std::tr1::make_tuple(48000, 48000, false),
- std::tr1::make_tuple(96000, 48000, false),
- std::tr1::make_tuple(192000, 48000, false),
-
- // To 96kHz
- std::tr1::make_tuple(8000, 96000, false),
- std::tr1::make_tuple(16000, 96000, false),
- std::tr1::make_tuple(32000, 96000, false),
- std::tr1::make_tuple(44100, 96000, true),
- std::tr1::make_tuple(48000, 96000, false),
- std::tr1::make_tuple(96000, 96000, false),
- std::tr1::make_tuple(192000, 96000, false),
-
- // To 192kHz
- std::tr1::make_tuple(8000, 192000, true),
- std::tr1::make_tuple(16000, 192000, false),
- std::tr1::make_tuple(32000, 192000, false),
- std::tr1::make_tuple(44100, 192000, true),
- std::tr1::make_tuple(48000, 192000, false),
- std::tr1::make_tuple(96000, 192000, false),
- std::tr1::make_tuple(192000, 192000, false)));
-
} // namespace webrtc
diff --git a/webrtc/modules/audio_coding/main/source/acm_resampler.cc b/webrtc/modules/audio_coding/main/source/acm_resampler.cc
index 2618649..034dbe5 100644
--- a/webrtc/modules/audio_coding/main/source/acm_resampler.cc
+++ b/webrtc/modules/audio_coding/main/source/acm_resampler.cc
@@ -12,19 +12,15 @@
#include <string.h>
-#include "webrtc/common_audio/resampler/include/resampler.h"
-#include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
-#include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
-#include "webrtc/system_wrappers/interface/trace.h"
+#include "webrtc/common_audio/resampler/include/push_resampler.h"
+#include "webrtc/system_wrappers/interface/logging.h"
namespace webrtc {
-ACMResampler::ACMResampler()
- : resampler_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()) {
+ACMResampler::ACMResampler() {
}
ACMResampler::~ACMResampler() {
- delete resampler_crit_sect_;
}
int16_t ACMResampler::Resample10Msec(const int16_t* in_audio,
@@ -32,42 +28,32 @@
int16_t* out_audio,
int32_t out_freq_hz,
uint8_t num_audio_channels) {
- CriticalSectionScoped cs(resampler_crit_sect_);
-
if (in_freq_hz == out_freq_hz) {
size_t length = static_cast<size_t>(in_freq_hz * num_audio_channels / 100);
memcpy(out_audio, in_audio, length * sizeof(int16_t));
return static_cast<int16_t>(in_freq_hz / 100);
}
- // |maxLen| is maximum number of samples for 10ms at 48kHz.
- int max_len = 480 * num_audio_channels;
- int length_in = (int16_t)(in_freq_hz / 100) * num_audio_channels;
- int out_len;
+ // |max_length| is the maximum number of samples for 10ms at 48kHz.
+ // TODO(turajs): is this actually the capacity of the |out_audio| buffer?
+ int max_length = 480 * num_audio_channels;
+ int in_length = in_freq_hz / 100 * num_audio_channels;
- int32_t ret;
- ResamplerType type;
- type = (num_audio_channels == 1) ? kResamplerSynchronous :
- kResamplerSynchronousStereo;
-
- ret = resampler_.ResetIfNeeded(in_freq_hz, out_freq_hz, type);
- if (ret < 0) {
- WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, 0,
- "Error in reset of resampler");
+ if (resampler_.InitializeIfNeeded(in_freq_hz, out_freq_hz,
+ num_audio_channels) != 0) {
+ LOG_FERR3(LS_ERROR, InitializeIfNeeded, in_freq_hz, out_freq_hz,
+ num_audio_channels);
return -1;
}
- ret = resampler_.Push(in_audio, length_in, out_audio, max_len, out_len);
- if (ret < 0) {
- WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, 0,
- "Error in resampler: resampler.Push");
+ int out_length = resampler_.Resample(in_audio, in_length, out_audio,
+ max_length);
+ if (out_length == -1) {
+ LOG_FERR4(LS_ERROR, Resample, in_audio, in_length, out_audio, max_length);
return -1;
}
- int16_t out_audio_len_smpl = (int16_t) out_len /
- num_audio_channels;
-
- return out_audio_len_smpl;
+ return out_length / num_audio_channels;
}
} // namespace webrtc
diff --git a/webrtc/modules/audio_coding/main/source/acm_resampler.h b/webrtc/modules/audio_coding/main/source/acm_resampler.h
index ddb0094..c23abb8 100644
--- a/webrtc/modules/audio_coding/main/source/acm_resampler.h
+++ b/webrtc/modules/audio_coding/main/source/acm_resampler.h
@@ -11,13 +11,11 @@
#ifndef WEBRTC_MODULES_AUDIO_CODING_MAIN_SOURCE_ACM_RESAMPLER_H_
#define WEBRTC_MODULES_AUDIO_CODING_MAIN_SOURCE_ACM_RESAMPLER_H_
-#include "webrtc/common_audio/resampler/include/resampler.h"
+#include "webrtc/common_audio/resampler/include/push_resampler.h"
#include "webrtc/typedefs.h"
namespace webrtc {
-class CriticalSectionWrapper;
-
class ACMResampler {
public:
ACMResampler();
@@ -30,9 +28,7 @@
uint8_t num_audio_channels);
private:
- // Use the Resampler class.
- Resampler resampler_;
- CriticalSectionWrapper* resampler_crit_sect_;
+ PushResampler resampler_;
};
} // namespace webrtc
diff --git a/webrtc/system_wrappers/interface/logging.h b/webrtc/system_wrappers/interface/logging.h
index 1085fef..1b89c23 100644
--- a/webrtc/system_wrappers/interface/logging.h
+++ b/webrtc/system_wrappers/interface/logging.h
@@ -150,6 +150,8 @@
<< ", " << #v2 << "=" << v2
#define LOG_FERR3(sev, func, v1, v2, v3) LOG_FERR2(sev, func, v1, v2) \
<< ", " << #v3 << "=" << v3
+#define LOG_FERR4(sev, func, v1, v2, v3, v4) LOG_FERR3(sev, func, v1, v2, v3) \
+ << ", " << #v4 << "=" << v4
#endif // LOG
diff --git a/webrtc/voice_engine/output_mixer_unittest.cc b/webrtc/voice_engine/output_mixer_unittest.cc
index 24d3917..006c45f 100644
--- a/webrtc/voice_engine/output_mixer_unittest.cc
+++ b/webrtc/voice_engine/output_mixer_unittest.cc
@@ -158,7 +158,13 @@
printf("(%d, %d Hz) -> (%d, %d Hz) ", // SNR reported on the same line later.
src_channels, src_sample_rate_hz, dst_channels, dst_sample_rate_hz);
EXPECT_EQ(0, RemixAndResample(src_frame_, &resampler, &dst_frame_));
- EXPECT_GT(ComputeSNR(golden_frame_, dst_frame_, max_delay), 39.0f);
+ if (src_sample_rate_hz == 96000 && dst_sample_rate_hz == 8000) {
+ // The sinc resampler gives poor SNR at this extreme conversion, but we
+ // expect to see this rarely in practice.
+ EXPECT_GT(ComputeSNR(golden_frame_, dst_frame_, max_delay), 14.0f);
+ } else {
+ EXPECT_GT(ComputeSNR(golden_frame_, dst_frame_, max_delay), 46.0f);
+ }
}
TEST_F(OutputMixerTest, RemixAndResampleCopyFrameSucceeds) {