diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index e92ae6dffd..745e45ce72 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -663,8 +663,6 @@ AudioProcessingImpl::AudioProcessingImpl( use_setup_specific_default_aec3_config_( UseSetupSpecificDefaultAec3Congfig()), gain_controller2_experiment_params_(GetGainController2ExperimentParams()), - use_denormal_disabler_( - !field_trial::IsEnabled("WebRTC-ApmDenormalDisablerKillSwitch")), transient_suppressor_vad_mode_( GetTransientSuppressorVadMode(gain_controller2_experiment_params_)), capture_runtime_settings_(RuntimeSettingQueueSize()), @@ -1159,7 +1157,7 @@ int AudioProcessingImpl::ProcessStream(const float* const* src, const StreamConfig& output_config, float* const* dest) { TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_StreamConfig"); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; RETURN_ON_ERR( HandleUnsupportedAudioFormats(src, input_config, output_config, dest)); MaybeInitializeCapture(input_config, output_config); @@ -1464,7 +1462,7 @@ int AudioProcessingImpl::ProcessStream(const int16_t* const src, MaybeInitializeCapture(input_config, output_config); MutexLock lock_capture(&mutex_capture_); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; if (aec_dump_) { RecordUnprocessedCaptureStream(src, input_config); @@ -1493,7 +1491,7 @@ int AudioProcessingImpl::ProcessStream(const int16_t* const src, int AudioProcessingImpl::ProcessCaptureStreamLocked() { EmptyQueuedRenderAudioLocked(); HandleCaptureRuntimeSettings(); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; // Ensure that not both the AEC and AECM are active at the same time. // TODO(peah): Simplify once the public API Enable functions for these @@ -1830,7 +1828,7 @@ int AudioProcessingImpl::AnalyzeReverseStream( const StreamConfig& reverse_config) { TRACE_EVENT0("webrtc", "AudioProcessing::AnalyzeReverseStream_StreamConfig"); MutexLock lock(&mutex_render_); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; RTC_DCHECK(data); for (size_t i = 0; i < reverse_config.num_channels(); ++i) { RTC_DCHECK(data[i]); @@ -1848,7 +1846,7 @@ int AudioProcessingImpl::ProcessReverseStream(const float* const* src, float* const* dest) { TRACE_EVENT0("webrtc", "AudioProcessing::ProcessReverseStream_StreamConfig"); MutexLock lock(&mutex_render_); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; RETURN_ON_ERR( HandleUnsupportedAudioFormats(src, input_config, output_config, dest)); @@ -1896,7 +1894,7 @@ int AudioProcessingImpl::ProcessReverseStream(const int16_t* const src, TRACE_EVENT0("webrtc", "AudioProcessing::ProcessReverseStream_AudioFrame"); MutexLock lock(&mutex_render_); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; RETURN_ON_ERR( HandleUnsupportedAudioFormats(src, input_config, output_config, dest)); @@ -1920,7 +1918,7 @@ int AudioProcessingImpl::ProcessRenderStreamLocked() { AudioBuffer* render_buffer = render_.render_audio.get(); // For brevity. HandleRenderRuntimeSettings(); - DenormalDisabler denormal_disabler(use_denormal_disabler_); + DenormalDisabler denormal_disabler; if (submodules_.render_pre_processor) { submodules_.render_pre_processor->Process(render_buffer); diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 8ee07edbe2..b4b3de2f2b 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -230,8 +230,6 @@ class AudioProcessingImpl : public AudioProcessing { static TransientSuppressor::VadMode GetTransientSuppressorVadMode( const absl::optional& experiment_params); - const bool use_denormal_disabler_; - const TransientSuppressor::VadMode transient_suppressor_vad_mode_; SwapQueue capture_runtime_settings_; diff --git a/system_wrappers/include/denormal_disabler.h b/system_wrappers/include/denormal_disabler.h index ca1e19ea68..bd3d401327 100644 --- a/system_wrappers/include/denormal_disabler.h +++ b/system_wrappers/include/denormal_disabler.h @@ -30,10 +30,12 @@ namespace webrtc { // } class DenormalDisabler { public: - // Ctor. If `enabled` is true and architecture and compiler are supported, - // stores the HW settings for denormals, disables denormals and sets - // `disabling_activated_` to true. Otherwise, only sets `disabling_activated_` - // to false. + // Ctor. If architecture and compiler are supported, stores the HW settings + // for denormals, disables denormals and sets `disabling_activated_` to true. + // Otherwise, only sets `disabling_activated_` to false. + DenormalDisabler(); + // Ctor. Same as above, but also requires `enabled` to be true to disable + // denormals. explicit DenormalDisabler(bool enabled); DenormalDisabler(const DenormalDisabler&) = delete; DenormalDisabler& operator=(const DenormalDisabler&) = delete; diff --git a/system_wrappers/source/denormal_disabler.cc b/system_wrappers/source/denormal_disabler.cc index fe8ec1afdc..bb9c05643c 100644 --- a/system_wrappers/source/denormal_disabler.cc +++ b/system_wrappers/source/denormal_disabler.cc @@ -73,6 +73,8 @@ constexpr bool DenormalsEnabled(int status_word) { } // namespace #if defined(WEBRTC_DENORMAL_DISABLER_SUPPORTED) +DenormalDisabler::DenormalDisabler() : DenormalDisabler(/*enabled=*/true) {} + DenormalDisabler::DenormalDisabler(bool enabled) : status_word_(enabled ? ReadStatusWord() : kUnspecifiedStatusWord), disabling_activated_(enabled && DenormalsEnabled(status_word_)) { @@ -94,6 +96,8 @@ DenormalDisabler::~DenormalDisabler() { } } #else +DenormalDisabler::DenormalDisabler() : DenormalDisabler(/*enabled=*/false) {} + DenormalDisabler::DenormalDisabler(bool enabled) : status_word_(kUnspecifiedStatusWord), disabling_activated_(false) {} diff --git a/system_wrappers/source/denormal_disabler_unittest.cc b/system_wrappers/source/denormal_disabler_unittest.cc index 1006c67c3c..a2849f853f 100644 --- a/system_wrappers/source/denormal_disabler_unittest.cc +++ b/system_wrappers/source/denormal_disabler_unittest.cc @@ -42,7 +42,7 @@ class DenormalDisablerParametrization : public ::testing::TestWithParam { // Checks that +inf and -inf are not zeroed regardless of whether // architecture and compiler are supported. -TEST_P(DenormalDisablerParametrization, InfNotZeroed) { +TEST_P(DenormalDisablerParametrization, InfNotZeroedExplicitlySetEnabled) { DenormalDisabler denormal_disabler(/*enabled=*/GetParam()); constexpr float kMax = std::numeric_limits::max(); for (float x : {-2.0f, 2.0f}) { @@ -54,7 +54,7 @@ TEST_P(DenormalDisablerParametrization, InfNotZeroed) { // Checks that a NaN is not zeroed regardless of whether architecture and // compiler are supported. -TEST_P(DenormalDisablerParametrization, NanNotZeroed) { +TEST_P(DenormalDisablerParametrization, NanNotZeroedExplicitlySetEnabled) { DenormalDisabler denormal_disabler(/*enabled=*/GetParam()); volatile float kNan = std::sqrt(-1.0f); EXPECT_TRUE(std::isnan(kNan)); @@ -67,6 +67,26 @@ INSTANTIATE_TEST_SUITE_P(DenormalDisabler, return info.param ? "enabled" : "disabled"; }); +// Checks that +inf and -inf are not zeroed regardless of whether +// architecture and compiler are supported. +TEST(DenormalDisabler, InfNotZeroed) { + DenormalDisabler denormal_disabler; + constexpr float kMax = std::numeric_limits::max(); + for (float x : {-2.0f, 2.0f}) { + SCOPED_TRACE(x); + volatile float multiplication = kMax * x; + EXPECT_TRUE(std::isinf(multiplication)); + } +} + +// Checks that a NaN is not zeroed regardless of whether architecture and +// compiler are supported. +TEST(DenormalDisabler, NanNotZeroed) { + DenormalDisabler denormal_disabler; + volatile float kNan = std::sqrt(-1.0f); + EXPECT_TRUE(std::isnan(kNan)); +} + // Checks that denormals are not zeroed if `DenormalDisabler` is disabled and // architecture and compiler are supported. TEST(DenormalDisabler, DoNotZeroDenormalsIfDisabled) { @@ -86,6 +106,20 @@ TEST(DenormalDisabler, DoNotZeroDenormalsIfDisabled) { // Checks that denormals are zeroed if `DenormalDisabler` is enabled if // architecture and compiler are supported. TEST(DenormalDisabler, ZeroDenormals) { + if (!DenormalDisabler::IsSupported()) { + GTEST_SKIP() << "Unsupported platform."; + } + DenormalDisabler denormal_disabler; + for (float x : kDenormalDivisors) { + SCOPED_TRACE(x); + EXPECT_FALSE(DivisionIsDenormal(-kSmallest, x)); + EXPECT_FALSE(DivisionIsDenormal(kSmallest, x)); + } +} + +// Checks that denormals are zeroed if `DenormalDisabler` is enabled if +// architecture and compiler are supported. +TEST(DenormalDisabler, ZeroDenormalsExplicitlyEnabled) { if (!DenormalDisabler::IsSupported()) { GTEST_SKIP() << "Unsupported platform."; } @@ -100,6 +134,21 @@ TEST(DenormalDisabler, ZeroDenormals) { // Checks that the `DenormalDisabler` dtor re-enables denormals if previously // enabled and architecture and compiler are supported. TEST(DenormalDisabler, RestoreDenormalsEnabled) { + if (!DenormalDisabler::IsSupported()) { + GTEST_SKIP() << "Unsupported platform."; + } + ASSERT_TRUE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0])) + << "Precondition not met: denormals must be enabled."; + { + DenormalDisabler denormal_disabler; + ASSERT_FALSE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0])); + } + EXPECT_TRUE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0])); +} + +// Checks that the `DenormalDisabler` dtor re-enables denormals if previously +// enabled and architecture and compiler are supported. +TEST(DenormalDisabler, RestoreDenormalsEnabledExplicitlyEnabled) { if (!DenormalDisabler::IsSupported()) { GTEST_SKIP() << "Unsupported platform."; } @@ -116,6 +165,22 @@ TEST(DenormalDisabler, RestoreDenormalsEnabled) { // architecture and compiler are supported and if previously disabled - i.e., // nested usage is supported. TEST(DenormalDisabler, ZeroDenormalsNested) { + if (!DenormalDisabler::IsSupported()) { + GTEST_SKIP() << "Unsupported platform."; + } + DenormalDisabler d1; + ASSERT_FALSE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0])); + { + DenormalDisabler d2; + ASSERT_FALSE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0])); + } + EXPECT_FALSE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0])); +} + +// Checks that the `DenormalDisabler` dtor keeps denormals disabled if +// architecture and compiler are supported and if previously disabled - i.e., +// nested usage is supported. +TEST(DenormalDisabler, ZeroDenormalsNestedExplicitlyEnabled) { if (!DenormalDisabler::IsSupported()) { GTEST_SKIP() << "Unsupported platform."; } @@ -131,6 +196,21 @@ TEST(DenormalDisabler, ZeroDenormalsNested) { // Checks that `DenormalDisabler` does not zero denormals if architecture and // compiler are not supported. TEST(DenormalDisabler, DoNotZeroDenormalsIfUnsupported) { + if (DenormalDisabler::IsSupported()) { + GTEST_SKIP() << "This test should only run on platforms without support " + "for DenormalDisabler."; + } + DenormalDisabler denormal_disabler; + for (float x : kDenormalDivisors) { + SCOPED_TRACE(x); + EXPECT_TRUE(DivisionIsDenormal(-kSmallest, x)); + EXPECT_TRUE(DivisionIsDenormal(kSmallest, x)); + } +} + +// Checks that `DenormalDisabler` does not zero denormals if architecture and +// compiler are not supported. +TEST(DenormalDisabler, DoNotZeroDenormalsIfUnsupportedExplicitlyEnabled) { if (DenormalDisabler::IsSupported()) { GTEST_SKIP() << "This test should only run on platforms without support " "for DenormalDisabler.";