From 6895d8ca788df29f4d3a4b97fe891f0fb3a6dbec Mon Sep 17 00:00:00 2001 From: kjellander Date: Thu, 26 May 2016 02:46:53 -0700 Subject: [PATCH] Revert of Adding a some checks and switching out a few assert for RTC_[D]CHECK. (patchset #1 id:1 of https://codereview.webrtc.org/2009253004/ ) Reason for revert: Fails unexpectedly on multiple commit bots: https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug%20%28Clang%29/builds/1748 https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug%20%28Clang%29/builds/1683 I filed https://bugs.chromium.org/p/chromium/issues/detail?id=614967 to track the problem. I'll reland if it doesn't solve the problem. Original issue's description: > Reland of Adding a some checks and switching out a few assert for RTC_[D]CHECK. (patchset #1 id:1 of https://codereview.webrtc.org/2006243002/ ) > > Original issue's description: > > Adding a some checks and switching out a few assert for RTC_[D]CHECK. > > These changes are around use of AudioFrame.data_ to help us catch issues earlier since assert() is left out in release builds, including builds with DCHECK enabled. I've also added a few full-on CHECKs to avoid reading past buffer boundaries or continuing on in a failed state. > > > > BUG=chromium:613482 > > NOTRY=true > > (using notry due to offline android_arm64_rel bot) > > > > Committed: https://crrev.com/d36df89d40bde3c62ee5cbff841933e50b3c007b > > Cr-Commit-Position: refs/heads/master@{#12870} > > TBR=henrik.lundin@webrtc.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=chromium:613482 > > Committed: https://crrev.com/ba189cc4f4f6fe311a815646059e8011ffa53894 > Cr-Commit-Position: refs/heads/master@{#12907} TBR=henrik.lundin@webrtc.org,tommi@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:613482 Review-Url: https://codereview.webrtc.org/2006313009 Cr-Commit-Position: refs/heads/master@{#12909} --- .../common_audio/resampler/push_resampler.cc | 16 ++-------- .../resampler/push_resampler_unittest.cc | 29 +++---------------- .../audio_coding_module_unittest_oldapi.cc | 5 +--- webrtc/voice_engine/channel.cc | 1 - .../auto_test/standard/external_media_test.cc | 17 +++++++++++ webrtc/voice_engine/utility.cc | 22 +++++++------- 6 files changed, 36 insertions(+), 54 deletions(-) diff --git a/webrtc/common_audio/resampler/push_resampler.cc b/webrtc/common_audio/resampler/push_resampler.cc index 06fdfb814d..f654e9a397 100644 --- a/webrtc/common_audio/resampler/push_resampler.cc +++ b/webrtc/common_audio/resampler/push_resampler.cc @@ -12,7 +12,6 @@ #include -#include "webrtc/base/checks.h" #include "webrtc/common_audio/include/audio_util.h" #include "webrtc/common_audio/resampler/include/resampler.h" #include "webrtc/common_audio/resampler/push_sinc_resampler.h" @@ -34,22 +33,15 @@ template int PushResampler::InitializeIfNeeded(int src_sample_rate_hz, int dst_sample_rate_hz, size_t num_channels) { - RTC_DCHECK_GT(src_sample_rate_hz, 0); - RTC_DCHECK_GT(dst_sample_rate_hz, 0); - RTC_DCHECK_GT(num_channels, 0u); - RTC_DCHECK_LE(num_channels, 2u); - if (src_sample_rate_hz == src_sample_rate_hz_ && dst_sample_rate_hz == dst_sample_rate_hz_ && - num_channels == num_channels_) { + num_channels == num_channels_) // No-op if settings haven't changed. return 0; - } - if (src_sample_rate_hz <= 0 || dst_sample_rate_hz <= 0 || num_channels <= 0 || - num_channels > 2) { + if (src_sample_rate_hz <= 0 || dst_sample_rate_hz <= 0 || + num_channels <= 0 || num_channels > 2) return -1; - } src_sample_rate_hz_ = src_sample_rate_hz; dst_sample_rate_hz_ = dst_sample_rate_hz; @@ -78,8 +70,6 @@ int PushResampler::Resample(const T* src, size_t src_length, T* dst, size_t dst_capacity) { const size_t src_size_10ms = src_sample_rate_hz_ * num_channels_ / 100; const size_t dst_size_10ms = dst_sample_rate_hz_ * num_channels_ / 100; - RTC_CHECK_EQ(src_length, src_size_10ms); - RTC_CHECK_GE(dst_capacity, dst_size_10ms); if (src_length != src_size_10ms || dst_capacity < dst_size_10ms) return -1; diff --git a/webrtc/common_audio/resampler/push_resampler_unittest.cc b/webrtc/common_audio/resampler/push_resampler_unittest.cc index 58880cc1b7..4449f4c633 100644 --- a/webrtc/common_audio/resampler/push_resampler_unittest.cc +++ b/webrtc/common_audio/resampler/push_resampler_unittest.cc @@ -9,7 +9,6 @@ */ #include "testing/gtest/include/gtest/gtest.h" -#include "webrtc/base/checks.h" // force defintion of RTC_DCHECK_IS_ON #include "webrtc/common_audio/resampler/include/push_resampler.h" // Quality testing of PushResampler is handled through output_mixer_unittest.cc. @@ -18,32 +17,12 @@ namespace webrtc { TEST(PushResamplerTest, VerifiesInputParameters) { PushResampler 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)); } -#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -TEST(PushResamplerTest, VerifiesBadInputParameters1) { - PushResampler resampler; - EXPECT_DEATH(resampler.InitializeIfNeeded(-1, 16000, 1), - "src_sample_rate_hz"); -} - -TEST(PushResamplerTest, VerifiesBadInputParameters2) { - PushResampler resampler; - EXPECT_DEATH(resampler.InitializeIfNeeded(16000, -1, 1), - "dst_sample_rate_hz"); -} - -TEST(PushResamplerTest, VerifiesBadInputParameters3) { - PushResampler resampler; - EXPECT_DEATH(resampler.InitializeIfNeeded(16000, 16000, 0), "num_channels"); -} - -TEST(PushResamplerTest, VerifiesBadInputParameters4) { - PushResampler resampler; - EXPECT_DEATH(resampler.InitializeIfNeeded(16000, 16000, 3), "num_channels"); -} -#endif - } // namespace webrtc diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc b/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc index eefe0a5420..d30a63c448 100644 --- a/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc +++ b/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc @@ -309,14 +309,11 @@ TEST_F(AudioCodingModuleTestOldApi, VerifyOutputFrame) { EXPECT_EQ(kSampleRateHz, audio_frame.sample_rate_hz_); } -#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) TEST_F(AudioCodingModuleTestOldApi, FailOnZeroDesiredFrequency) { AudioFrame audio_frame; bool muted; - EXPECT_DEATH(acm_->PlayoutData10Ms(0, &audio_frame, &muted), - "dst_sample_rate_hz"); + EXPECT_EQ(-1, acm_->PlayoutData10Ms(0, &audio_frame, &muted)); } -#endif // Checks that the transport callback is invoked once for each speech packet. // Also checks that the frame type is kAudioFrameSpeech. diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index 8ccad33161..742e53e836 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -2995,7 +2995,6 @@ uint32_t Channel::PrepareEncodeAndSend(int mixingFrequency) { if (_includeAudioLevelIndication) { size_t length = _audioFrame.samples_per_channel_ * _audioFrame.num_channels_; - RTC_CHECK_LE(length, sizeof(_audioFrame.data_)); if (is_muted && previous_frame_muted_) { rms_level_.ProcessMuted(length); } else { diff --git a/webrtc/voice_engine/test/auto_test/standard/external_media_test.cc b/webrtc/voice_engine/test/auto_test/standard/external_media_test.cc index 4534e128b3..4f86010a18 100644 --- a/webrtc/voice_engine/test/auto_test/standard/external_media_test.cc +++ b/webrtc/voice_engine/test/auto_test/standard/external_media_test.cc @@ -107,3 +107,20 @@ TEST_F(ExternalMediaTest, EXPECT_EQ(0, voe_xmedia_->SetExternalMixing(channel_, false)); ResumePlaying(); } + +TEST_F(ExternalMediaTest, + ExternalMixingResamplingToInvalidFrequenciesFails) { + const int kInvalidFrequencies[] = {-8000, -1}; + webrtc::AudioFrame frame; + PausePlaying(); + EXPECT_EQ(0, voe_xmedia_->SetExternalMixing(channel_, true)); + ResumePlaying(); + for (size_t i = 0; i < arraysize(kInvalidFrequencies); i++) { + int f = kInvalidFrequencies[i]; + EXPECT_EQ(-1, voe_xmedia_->GetAudioFrame(channel_, f, &frame)) + << "Resampling fails for freq=" << f; + } + PausePlaying(); + EXPECT_EQ(0, voe_xmedia_->SetExternalMixing(channel_, false)); + ResumePlaying(); +} diff --git a/webrtc/voice_engine/utility.cc b/webrtc/voice_engine/utility.cc index 37e12cea4f..605e55369e 100644 --- a/webrtc/voice_engine/utility.cc +++ b/webrtc/voice_engine/utility.cc @@ -10,7 +10,6 @@ #include "webrtc/voice_engine/utility.h" -#include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/common_audio/resampler/include/push_resampler.h" #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h" @@ -53,18 +52,21 @@ void RemixAndResample(const int16_t* src_data, if (resampler->InitializeIfNeeded(sample_rate_hz, dst_frame->sample_rate_hz_, audio_ptr_num_channels) == -1) { - FATAL() << "InitializeIfNeeded failed: sample_rate_hz = " << sample_rate_hz - << ", dst_frame->sample_rate_hz_ = " << dst_frame->sample_rate_hz_ - << ", audio_ptr_num_channels = " << audio_ptr_num_channels; + LOG(LS_ERROR) << "InitializeIfNeeded failed: sample_rate_hz = " + << sample_rate_hz << ", dst_frame->sample_rate_hz_ = " + << dst_frame->sample_rate_hz_ + << ", audio_ptr_num_channels = " << audio_ptr_num_channels; + assert(false); } const size_t src_length = samples_per_channel * audio_ptr_num_channels; int out_length = resampler->Resample(audio_ptr, src_length, dst_frame->data_, AudioFrame::kMaxDataSizeSamples); if (out_length == -1) { - FATAL() << "Resample failed: audio_ptr = " << audio_ptr - << ", src_length = " << src_length - << ", dst_frame->data_ = " << dst_frame->data_; + LOG(LS_ERROR) << "Resample failed: audio_ptr = " << audio_ptr + << ", src_length = " << src_length + << ", dst_frame->data_ = " << dst_frame->data_; + assert(false); } dst_frame->samples_per_channel_ = out_length / audio_ptr_num_channels; @@ -82,10 +84,8 @@ void MixWithSat(int16_t target[], const int16_t source[], size_t source_channel, size_t source_len) { - RTC_DCHECK_GE(target_channel, 1u); - RTC_DCHECK_LE(target_channel, 2u); - RTC_DCHECK_GE(source_channel, 1u); - RTC_DCHECK_LE(source_channel, 2u); + assert(target_channel == 1 || target_channel == 2); + assert(source_channel == 1 || source_channel == 2); if (target_channel == 2 && source_channel == 1) { // Convert source from mono to stereo.