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/2006313009/ )
Reason for revert:
Relanding. The errors look like legit disk space problems so I'm going to watch the bots this time.
Original issue's description:
> 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
>
> Committed: https://crrev.com/6895d8ca788df29f4d3a4b97fe891f0fb3a6dbec
> Cr-Commit-Position: refs/heads/master@{#12909}
TBR=henrik.lundin@webrtc.org,kjellander@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/2014183003
Cr-Commit-Position: refs/heads/master@{#12920}
diff --git a/webrtc/common_audio/resampler/push_resampler.cc b/webrtc/common_audio/resampler/push_resampler.cc
index f654e9a..06fdfb8 100644
--- a/webrtc/common_audio/resampler/push_resampler.cc
+++ b/webrtc/common_audio/resampler/push_resampler.cc
@@ -12,6 +12,7 @@
#include <string.h>
+#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"
@@ -33,15 +34,22 @@
int PushResampler<T>::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;
@@ -70,6 +78,8 @@
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 4449f4c..58880cc 100644
--- a/webrtc/common_audio/resampler/push_resampler_unittest.cc
+++ b/webrtc/common_audio/resampler/push_resampler_unittest.cc
@@ -9,6 +9,7 @@
*/
#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.
@@ -17,12 +18,32 @@
TEST(PushResamplerTest, VerifiesInputParameters) {
PushResampler<int16_t> 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<int16_t> resampler;
+ EXPECT_DEATH(resampler.InitializeIfNeeded(-1, 16000, 1),
+ "src_sample_rate_hz");
+}
+
+TEST(PushResamplerTest, VerifiesBadInputParameters2) {
+ PushResampler<int16_t> resampler;
+ EXPECT_DEATH(resampler.InitializeIfNeeded(16000, -1, 1),
+ "dst_sample_rate_hz");
+}
+
+TEST(PushResamplerTest, VerifiesBadInputParameters3) {
+ PushResampler<int16_t> resampler;
+ EXPECT_DEATH(resampler.InitializeIfNeeded(16000, 16000, 0), "num_channels");
+}
+
+TEST(PushResamplerTest, VerifiesBadInputParameters4) {
+ PushResampler<int16_t> 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 d30a63c..eefe0a5 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,11 +309,14 @@
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_EQ(-1, acm_->PlayoutData10Ms(0, &audio_frame, &muted));
+ EXPECT_DEATH(acm_->PlayoutData10Ms(0, &audio_frame, &muted),
+ "dst_sample_rate_hz");
}
+#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 742e53e..8ccad33 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -2995,6 +2995,7 @@
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 4f86010..4534e12 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,20 +107,3 @@
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 605e553..37e12ce 100644
--- a/webrtc/voice_engine/utility.cc
+++ b/webrtc/voice_engine/utility.cc
@@ -10,6 +10,7 @@
#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"
@@ -52,21 +53,18 @@
if (resampler->InitializeIfNeeded(sample_rate_hz, dst_frame->sample_rate_hz_,
audio_ptr_num_channels) == -1) {
- 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);
+ 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;
}
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) {
- LOG(LS_ERROR) << "Resample failed: audio_ptr = " << audio_ptr
- << ", src_length = " << src_length
- << ", dst_frame->data_ = " << dst_frame->data_;
- assert(false);
+ FATAL() << "Resample failed: audio_ptr = " << audio_ptr
+ << ", src_length = " << src_length
+ << ", dst_frame->data_ = " << dst_frame->data_;
}
dst_frame->samples_per_channel_ = out_length / audio_ptr_num_channels;
@@ -84,8 +82,10 @@
const int16_t source[],
size_t source_channel,
size_t source_len) {
- assert(target_channel == 1 || target_channel == 2);
- assert(source_channel == 1 || source_channel == 2);
+ RTC_DCHECK_GE(target_channel, 1u);
+ RTC_DCHECK_LE(target_channel, 2u);
+ RTC_DCHECK_GE(source_channel, 1u);
+ RTC_DCHECK_LE(source_channel, 2u);
if (target_channel == 2 && source_channel == 1) {
// Convert source from mono to stereo.
