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}

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);

modules/audio_processing/audio_processing_impl.h

@@ -230,8 +230,6 @@ class AudioProcessingImpl : public AudioProcessing {
   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_;

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,
+  // Ctor. If architecture and compiler are supported, stores the HW settings
-  // stores the HW settings for denormals, disables denormals and sets
+  // for denormals, disables denormals and sets `disabling_activated_` to true.
-  // `disabling_activated_` to true. Otherwise, only sets `disabling_activated_`
+  // Otherwise, only sets `disabling_activated_` to false.
-  // 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;

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) {}
 

system_wrappers/source/denormal_disabler_unittest.cc

@@ -42,7 +42,7 @@ class DenormalDisablerParametrization : public ::testing::TestWithParam<bool> {
 
 // 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 @@ 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<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) {
@@ -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.";