APM: remove denormal disabler field trial
Always use the denormal disabler
Bug: chromium:1227566
Change-Id: I915567aac683a8cd23d6d09b75536c81fd4ee2a6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/288680
Reviewed-by: Per Åhgren <peah@webrtc.org>
Commit-Queue: Alessio Bazzica <alessiob@webrtc.org>
Reviewed-by: Henrik Andreassson <henrika@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38936}
diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc
index e92ae6d..745e45c 100644
--- a/modules/audio_processing/audio_processing_impl.cc
+++ b/modules/audio_processing/audio_processing_impl.cc
@@ -663,8 +663,6 @@
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 @@
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 @@
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::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 @@
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 @@
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 @@
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 @@
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 8ee07ed..b4b3de2 100644
--- a/modules/audio_processing/audio_processing_impl.h
+++ b/modules/audio_processing/audio_processing_impl.h
@@ -230,8 +230,6 @@
static TransientSuppressor::VadMode GetTransientSuppressorVadMode(
const absl::optional<GainController2ExperimentParams>& experiment_params);
- const bool use_denormal_disabler_;
-
const TransientSuppressor::VadMode transient_suppressor_vad_mode_;
SwapQueue<RuntimeSetting> capture_runtime_settings_;
diff --git a/system_wrappers/include/denormal_disabler.h b/system_wrappers/include/denormal_disabler.h
index ca1e19e..bd3d401 100644
--- a/system_wrappers/include/denormal_disabler.h
+++ b/system_wrappers/include/denormal_disabler.h
@@ -30,10 +30,12 @@
// }
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 fe8ec1a..bb9c056 100644
--- a/system_wrappers/source/denormal_disabler.cc
+++ b/system_wrappers/source/denormal_disabler.cc
@@ -73,6 +73,8 @@
} // 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 @@
}
}
#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 1006c67..a2849f8 100644
--- a/system_wrappers/source/denormal_disabler_unittest.cc
+++ b/system_wrappers/source/denormal_disabler_unittest.cc
@@ -42,7 +42,7 @@
// 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<float>::max();
for (float x : {-2.0f, 2.0f}) {
@@ -54,7 +54,7 @@
// 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 @@
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<float>::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) {
@@ -89,6 +109,20 @@
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.";
+ }
DenormalDisabler denormal_disabler(/*enabled=*/true);
for (float x : kDenormalDivisors) {
SCOPED_TRACE(x);
@@ -106,6 +140,21 @@
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.";
+ }
+ ASSERT_TRUE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0]))
+ << "Precondition not met: denormals must be enabled.";
+ {
DenormalDisabler denormal_disabler(/*enabled=*/true);
ASSERT_FALSE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0]));
}
@@ -119,6 +168,22 @@
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.";
+ }
DenormalDisabler d1(/*enabled=*/true);
ASSERT_FALSE(DivisionIsDenormal(kSmallest, kDenormalDivisors[0]));
{
@@ -135,6 +200,21 @@
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.";
+ }
DenormalDisabler denormal_disabler(/*enabled=*/true);
for (float x : kDenormalDivisors) {
SCOPED_TRACE(x);
