Add AudioEncoderOpus constructors that use field trials from Environment

Deprecate or remove other constructor

Bug: webrtc:343086059
Change-Id: I863a1df1b313f871a0b03763be1588e68ceb84a0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/355182
Reviewed-by: Jakob Ivarsson‎ <jakobi@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42545}

api/audio_codecs/opus/BUILD.gn

@@ -50,6 +50,8 @@ rtc_library("audio_encoder_opus") {
     "../../../modules/audio_coding:webrtc_opus",
     "../../../rtc_base:checks",
     "../../../rtc_base/system:rtc_export",
+    "../../environment",
+    "//third_party/abseil-cpp/absl/memory",
     "//third_party/abseil-cpp/absl/types:optional",
   ]
 }

api/audio_codecs/opus/audio_encoder_opus.cc

@@ -13,9 +13,11 @@
 #include <memory>
 #include <vector>
 
+#include "absl/memory/memory.h"
 #include "absl/types/optional.h"
 #include "api/audio_codecs/audio_codec_pair_id.h"
 #include "api/audio_codecs/audio_encoder.h"
+#include "api/audio_codecs/audio_encoder_factory.h"
 #include "api/audio_codecs/audio_format.h"
 #include "api/audio_codecs/opus/audio_encoder_opus_config.h"
 #include "api/field_trials_view.h"
@@ -48,7 +50,23 @@ std::unique_ptr<AudioEncoder> AudioEncoderOpus::MakeAudioEncoder(
     RTC_DCHECK_NOTREACHED();
     return nullptr;
   }
-  return AudioEncoderOpusImpl::MakeAudioEncoder(config, payload_type);
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+  // Use WrapUnique to call deprecated constructor.
+  return absl::WrapUnique(new AudioEncoderOpusImpl(config, payload_type));
+#pragma clang diagnostic pop
+}
+
+std::unique_ptr<AudioEncoder> AudioEncoderOpus::MakeAudioEncoder(
+    const Environment& env,
+    const AudioEncoderOpusConfig& config,
+    const AudioEncoderFactory::Options& options) {
+  if (!config.IsOk()) {
+    RTC_DCHECK_NOTREACHED();
+    return nullptr;
+  }
+  return std::make_unique<AudioEncoderOpusImpl>(env, config,
+                                                options.payload_type);
 }
 
 }  // namespace webrtc

api/audio_codecs/opus/audio_encoder_opus.h

@@ -17,8 +17,10 @@
 #include "absl/types/optional.h"
 #include "api/audio_codecs/audio_codec_pair_id.h"
 #include "api/audio_codecs/audio_encoder.h"
+#include "api/audio_codecs/audio_encoder_factory.h"
 #include "api/audio_codecs/audio_format.h"
 #include "api/audio_codecs/opus/audio_encoder_opus_config.h"
+#include "api/environment/environment.h"
 #include "api/field_trials_view.h"
 #include "rtc_base/system/rtc_export.h"
 
@@ -32,6 +34,10 @@ struct RTC_EXPORT AudioEncoderOpus {
       const SdpAudioFormat& audio_format);
   static void AppendSupportedEncoders(std::vector<AudioCodecSpec>* specs);
   static AudioCodecInfo QueryAudioEncoder(const AudioEncoderOpusConfig& config);
+  static std::unique_ptr<AudioEncoder> MakeAudioEncoder(
+      const Environment& env,
+      const AudioEncoderOpusConfig& config,
+      const AudioEncoderFactory::Options& options);
   static std::unique_ptr<AudioEncoder> MakeAudioEncoder(
       const AudioEncoderOpusConfig& config,
       int payload_type,

modules/audio_coding/BUILD.gn

@@ -469,8 +469,11 @@ rtc_library("webrtc_opus") {
     ":audio_coding_opus_common",
     ":audio_network_adaptor",
     "../../api:array_view",
+    "../../api:field_trials_view",
     "../../api/audio_codecs:audio_codecs_api",
     "../../api/audio_codecs/opus:audio_encoder_opus_config",
+    "../../api/environment",
+    "../../api/transport:field_trial_based_config",
     "../../common_audio",
     "../../rtc_base:buffer",
     "../../rtc_base:checks",
@@ -483,6 +486,7 @@ rtc_library("webrtc_opus") {
     "../../rtc_base:stringutils",
     "../../rtc_base:timeutils",
     "../../system_wrappers:field_trial",
+    "//third_party/abseil-cpp/absl/memory",
     "//third_party/abseil-cpp/absl/strings",
     "//third_party/abseil-cpp/absl/strings:string_view",
     "//third_party/abseil-cpp/absl/types:optional",
@@ -1723,6 +1727,7 @@ if (rtc_include_tests) {
         "../../system_wrappers",
         "../../test:audio_codec_mocks",
         "../../test:audio_test_common",
+        "../../test:explicit_key_value_config",
         "../../test:field_trial",
         "../../test:fileutils",
         "../../test:rtc_expect_death",

modules/audio_coding/codecs/opus/audio_encoder_opus.cc

@@ -16,8 +16,11 @@
 #include <string>
 #include <utility>
 
+#include "absl/memory/memory.h"
 #include "absl/strings/match.h"
 #include "absl/strings/string_view.h"
+#include "api/field_trials_view.h"
+#include "api/transport/field_trial_based_config.h"
 #include "modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h"
 #include "modules/audio_coding/audio_network_adaptor/controller_manager.h"
 #include "modules/audio_coding/codecs/opus/audio_coder_opus_common.h"
@@ -31,7 +34,6 @@
 #include "rtc_base/string_encode.h"
 #include "rtc_base/string_to_number.h"
 #include "rtc_base/time_utils.h"
-#include "system_wrappers/include/field_trial.h"
 
 namespace webrtc {
 
@@ -163,14 +165,14 @@ int GetBitrateBps(const AudioEncoderOpusConfig& config) {
   return *config.bitrate_bps;
 }
 
-std::vector<float> GetBitrateMultipliers() {
+std::vector<float> GetBitrateMultipliers(const FieldTrialsView& field_trials) {
   constexpr char kBitrateMultipliersName[] =
       "WebRTC-Audio-OpusBitrateMultipliers";
   const bool use_bitrate_multipliers =
-      webrtc::field_trial::IsEnabled(kBitrateMultipliersName);
+      field_trials.IsEnabled(kBitrateMultipliersName);
   if (use_bitrate_multipliers) {
     const std::string field_trial_string =
-        webrtc::field_trial::FindFullName(kBitrateMultipliersName);
+        field_trials.Lookup(kBitrateMultipliersName);
     std::vector<std::string> pieces;
     rtc::tokenize(field_trial_string, '-', &pieces);
     if (pieces.size() < 2 || pieces[0] != "Enabled") {
@@ -227,16 +229,6 @@ AudioCodecInfo AudioEncoderOpusImpl::QueryAudioEncoder(
   return info;
 }
 
-std::unique_ptr<AudioEncoder> AudioEncoderOpusImpl::MakeAudioEncoder(
-    const AudioEncoderOpusConfig& config,
-    int payload_type) {
-  if (!config.IsOk()) {
-    RTC_DCHECK_NOTREACHED();
-    return nullptr;
-  }
-  return std::make_unique<AudioEncoderOpusImpl>(config, payload_type);
-}
-
 absl::optional<AudioEncoderOpusConfig> AudioEncoderOpusImpl::SdpToConfig(
     const SdpAudioFormat& format) {
   if (!absl::EqualsIgnoreCase(format.name, "opus") ||
@@ -345,9 +337,35 @@ class AudioEncoderOpusImpl::PacketLossFractionSmoother {
   rtc::ExpFilter smoother_;
 };
 
+std::unique_ptr<AudioEncoderOpusImpl> AudioEncoderOpusImpl::CreateForTesting(
+    const Environment& env,
+    const AudioEncoderOpusConfig& config,
+    int payload_type,
+    const AudioNetworkAdaptorCreator& audio_network_adaptor_creator,
+    std::unique_ptr<SmoothingFilter> bitrate_smoother) {
+  // Using `new` to access a non-public constructor.
+  return absl::WrapUnique(new AudioEncoderOpusImpl(
+      env.field_trials(), config, payload_type, audio_network_adaptor_creator,
+      std::move(bitrate_smoother)));
+}
+
+AudioEncoderOpusImpl::AudioEncoderOpusImpl(const Environment& env,
+                                           const AudioEncoderOpusConfig& config,
+                                           int payload_type)
+    : AudioEncoderOpusImpl(
+          env.field_trials(),
+          config,
+          payload_type,
+          [this](absl::string_view config_string, RtcEventLog* event_log) {
+            return DefaultAudioNetworkAdaptorCreator(config_string, event_log);
+          },
+          // We choose 5sec as initial time constant due to empirical data.
+          std::make_unique<SmoothingFilterImpl>(5'000)) {}
+
 AudioEncoderOpusImpl::AudioEncoderOpusImpl(const AudioEncoderOpusConfig& config,
                                            int payload_type)
     : AudioEncoderOpusImpl(
+          FieldTrialBasedConfig(),
           config,
           payload_type,
           [this](absl::string_view config_string, RtcEventLog* event_log) {
@@ -357,17 +375,17 @@ AudioEncoderOpusImpl::AudioEncoderOpusImpl(const AudioEncoderOpusConfig& config,
           std::make_unique<SmoothingFilterImpl>(5000)) {}
 
 AudioEncoderOpusImpl::AudioEncoderOpusImpl(
+    const FieldTrialsView& field_trials,
     const AudioEncoderOpusConfig& config,
     int payload_type,
     const AudioNetworkAdaptorCreator& audio_network_adaptor_creator,
     std::unique_ptr<SmoothingFilter> bitrate_smoother)
     : payload_type_(payload_type),
-      use_stable_target_for_adaptation_(!webrtc::field_trial::IsDisabled(
-          "WebRTC-Audio-StableTargetAdaptation")),
-      adjust_bandwidth_(
-          webrtc::field_trial::IsEnabled("WebRTC-AdjustOpusBandwidth")),
+      use_stable_target_for_adaptation_(
+          !field_trials.IsDisabled("WebRTC-Audio-StableTargetAdaptation")),
+      adjust_bandwidth_(field_trials.IsEnabled("WebRTC-AdjustOpusBandwidth")),
       bitrate_changed_(true),
-      bitrate_multipliers_(GetBitrateMultipliers()),
+      bitrate_multipliers_(GetBitrateMultipliers(field_trials)),
       packet_loss_rate_(0.0),
       inst_(nullptr),
       packet_loss_fraction_smoother_(new PacketLossFractionSmoother()),
@@ -384,10 +402,6 @@ AudioEncoderOpusImpl::AudioEncoderOpusImpl(
   SetProjectedPacketLossRate(packet_loss_rate_);
 }
 
-AudioEncoderOpusImpl::AudioEncoderOpusImpl(int payload_type,
-                                           const SdpAudioFormat& format)
-    : AudioEncoderOpusImpl(*SdpToConfig(format), payload_type) {}
-
 AudioEncoderOpusImpl::~AudioEncoderOpusImpl() {
   RTC_CHECK_EQ(0, WebRtcOpus_EncoderFree(inst_));
 }

modules/audio_coding/codecs/opus/audio_encoder_opus.h

@@ -21,6 +21,7 @@
 #include "api/audio_codecs/audio_encoder.h"
 #include "api/audio_codecs/audio_format.h"
 #include "api/audio_codecs/opus/audio_encoder_opus_config.h"
+#include "api/environment/environment.h"
 #include "common_audio/smoothing_filter.h"
 #include "modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h"
 #include "modules/audio_coding/codecs/opus/opus_interface.h"
@@ -49,16 +50,21 @@ class AudioEncoderOpusImpl final : public AudioEncoder {
       std::function<std::unique_ptr<AudioNetworkAdaptor>(absl::string_view,
                                                          RtcEventLog*)>;
 
-  AudioEncoderOpusImpl(const AudioEncoderOpusConfig& config, int payload_type);
-
-  // Dependency injection for testing.
-  AudioEncoderOpusImpl(
+  static std::unique_ptr<AudioEncoderOpusImpl> CreateForTesting(
+      const Environment& env,
       const AudioEncoderOpusConfig& config,
       int payload_type,
       const AudioNetworkAdaptorCreator& audio_network_adaptor_creator,
       std::unique_ptr<SmoothingFilter> bitrate_smoother);
 
-  AudioEncoderOpusImpl(int payload_type, const SdpAudioFormat& format);
+  AudioEncoderOpusImpl(const Environment& env,
+                       const AudioEncoderOpusConfig& config,
+                       int payload_type);
+
+  [[deprecated("bugs.webrtc.org/343086059")]] AudioEncoderOpusImpl(
+      const AudioEncoderOpusConfig& config,
+      int payload_type);
+
   ~AudioEncoderOpusImpl() override;
 
   AudioEncoderOpusImpl(const AudioEncoderOpusImpl&) = delete;
@@ -120,13 +126,19 @@ class AudioEncoderOpusImpl final : public AudioEncoder {
  private:
   class PacketLossFractionSmoother;
 
+  // TODO: bugs.webrtc.org/343086059 - Replace field_trials with Environment
+  // when public constructors that do not provide the Environment are removed.
+  AudioEncoderOpusImpl(
+      const FieldTrialsView& field_trials,
+      const AudioEncoderOpusConfig& config,
+      int payload_type,
+      const AudioNetworkAdaptorCreator& audio_network_adaptor_creator,
+      std::unique_ptr<SmoothingFilter> bitrate_smoother);
+
   static absl::optional<AudioEncoderOpusConfig> SdpToConfig(
       const SdpAudioFormat& format);
   static void AppendSupportedEncoders(std::vector<AudioCodecSpec>* specs);
   static AudioCodecInfo QueryAudioEncoder(const AudioEncoderOpusConfig& config);
-  static std::unique_ptr<AudioEncoder> MakeAudioEncoder(
-      const AudioEncoderOpusConfig&,
-      int payload_type);
 
   size_t Num10msFramesPerPacket() const;
   size_t SamplesPer10msFrame() const;

modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc

@@ -15,6 +15,7 @@
 #include <utility>
 
 #include "absl/strings/string_view.h"
+#include "api/environment/environment_factory.h"
 #include "common_audio/mocks/mock_smoothing_filter.h"
 #include "modules/audio_coding/audio_network_adaptor/mock/mock_audio_network_adaptor.h"
 #include "modules/audio_coding/codecs/opus/audio_encoder_opus.h"
@@ -22,17 +23,18 @@
 #include "modules/audio_coding/neteq/tools/audio_loop.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/fake_clock.h"
-#include "test/field_trial.h"
+#include "test/explicit_key_value_config.h"
 #include "test/gmock.h"
 #include "test/gtest.h"
+#include "test/scoped_key_value_config.h"
 #include "test/testsupport/file_utils.h"
 
 namespace webrtc {
+namespace {
+using test::ExplicitKeyValueConfig;
 using ::testing::NiceMock;
 using ::testing::Return;
 
-namespace {
-
 constexpr int kDefaultOpusPayloadType = 105;
 constexpr int kDefaultOpusRate = 32000;
 constexpr int kDefaultOpusPacSize = 960;
@@ -52,8 +54,10 @@ struct AudioEncoderOpusStates {
   AudioEncoderOpusConfig config;
 };
 
-std::unique_ptr<AudioEncoderOpusStates> CreateCodec(int sample_rate_hz,
-                                                    size_t num_channels) {
+std::unique_ptr<AudioEncoderOpusStates> CreateCodec(
+    int sample_rate_hz,
+    size_t num_channels,
+    const FieldTrialsView* field_trials = nullptr) {
   std::unique_ptr<AudioEncoderOpusStates> states =
       std::make_unique<AudioEncoderOpusStates>();
   states->mock_audio_network_adaptor = nullptr;
@@ -85,9 +89,9 @@ std::unique_ptr<AudioEncoderOpusStates> CreateCodec(int sample_rate_hz,
       new MockSmoothingFilter());
   states->mock_bitrate_smoother = bitrate_smoother.get();
 
-  states->encoder.reset(
-      new AudioEncoderOpusImpl(states->config, kDefaultOpusPayloadType, creator,
-                               std::move(bitrate_smoother)));
+  states->encoder = AudioEncoderOpusImpl::CreateForTesting(
+      CreateEnvironment(field_trials), states->config, kDefaultOpusPayloadType,
+      creator, std::move(bitrate_smoother));
   return states;
 }
 
@@ -264,9 +268,9 @@ TEST_P(AudioEncoderOpusTest,
 
 TEST_P(AudioEncoderOpusTest,
        InvokeAudioNetworkAdaptorOnReceivedUplinkBandwidth) {
-  test::ScopedFieldTrials override_field_trials(
+  ExplicitKeyValueConfig field_trials(
       "WebRTC-Audio-StableTargetAdaptation/Disabled/");
-  auto states = CreateCodec(sample_rate_hz_, 2);
+  auto states = CreateCodec(sample_rate_hz_, 2, &field_trials);
   states->encoder->EnableAudioNetworkAdaptor("", nullptr);
 
   auto config = CreateEncoderRuntimeConfig();
@@ -485,9 +489,9 @@ TEST_P(AudioEncoderOpusTest, EmptyConfigDoesNotAffectEncoderSettings) {
 }
 
 TEST_P(AudioEncoderOpusTest, UpdateUplinkBandwidthInAudioNetworkAdaptor) {
-  test::ScopedFieldTrials override_field_trials(
+  ExplicitKeyValueConfig field_trials(
       "WebRTC-Audio-StableTargetAdaptation/Disabled/");
-  auto states = CreateCodec(sample_rate_hz_, 2);
+  auto states = CreateCodec(sample_rate_hz_, 2, &field_trials);
   states->encoder->EnableAudioNetworkAdaptor("", nullptr);
   const size_t opus_rate_khz = rtc::CheckedDivExact(sample_rate_hz_, 1000);
   const std::vector<int16_t> audio(opus_rate_khz * 10 * 2, 0);
@@ -819,7 +823,16 @@ TEST_P(AudioEncoderOpusTest, OpusFlagDtxAsNonSpeech) {
 }
 
 TEST(AudioEncoderOpusTest, OpusDtxFilteringHighEnergyRefreshPackets) {
-  test::ScopedFieldTrials override_field_trials(
+  // TODO: bugs.webrtc.org/343086059 - Use `ExplicitKeyValueConfig` type to
+  // ensure global field trial string is not used when this field trial is
+  // queried from the passed Environment.
+  // There are currently two complications for that:
+  // - field trial is queried by WebRtcOpus_EncoderCreate that follows c-style
+  // interface, and thus is not ready to accept c++ interface FieldTrialsView
+  // - field trial is queried during `RecreateEncoderInstance`, i.e., opus
+  // encoder needs to save field trials passed at construction. That will be
+  // simpler once all public constructors accept webrtc::Environment.
+  test::ScopedKeyValueConfig field_trials(
       "WebRTC-Audio-OpusAvoidNoisePumpingDuringDtx/Enabled/");
   const std::string kInputFileName =
       webrtc::test::ResourcePath("audio_coding/testfile16kHz", "pcm");
@@ -828,7 +841,8 @@ TEST(AudioEncoderOpusTest, OpusDtxFilteringHighEnergyRefreshPackets) {
   config.dtx_enabled = true;
   config.sample_rate_hz = kSampleRateHz;
   constexpr int payload_type = 17;
-  const auto encoder = AudioEncoderOpus::MakeAudioEncoder(config, payload_type);
+  AudioEncoderOpusImpl encoder(CreateEnvironment(&field_trials), config,
+                               payload_type);
   test::AudioLoop audio_loop;
   constexpr size_t kMaxLoopLengthSaples = kSampleRateHz * 11.6f;
   constexpr size_t kInputBlockSizeSamples = kSampleRateHz / 100;
@@ -849,7 +863,7 @@ TEST(AudioEncoderOpusTest, OpusDtxFilteringHighEnergyRefreshPackets) {
     // Every second call to the encoder will generate an Opus packet.
     for (int j = 0; j < 2; j++) {
       auto next_frame = audio_loop.GetNextBlock();
-      info = encoder->Encode(rtp_timestamp, next_frame, &encoded);
+      info = encoder.Encode(rtp_timestamp, next_frame, &encoded);
       if (opus_entered_dtx) {
         size_t silence_frame_start = rtp_timestamp - timestamp_start_silence;
         silence_filled = silence_frame_start >= kSilenceDurationSamples;
@@ -890,7 +904,7 @@ TEST(AudioEncoderOpusTest, OpusDtxFilteringHighEnergyRefreshPackets) {
             silence.begin() + silence_frame_start,
             silence.begin() + silence_frame_start + kInputBlockSizeSamples,
             silence_frame.begin(), [gain](float s) { return gain * s; });
-        info = encoder->Encode(rtp_timestamp, silence_frame, &encoded);
+        info = encoder.Encode(rtp_timestamp, silence_frame, &encoded);
         rtp_timestamp += kInputBlockSizeSamples;
       }
       EXPECT_TRUE(info.encoded_bytes > 0 || last_packet_dtx_frame);