From eaea3e26d7cb649871a7d96d8bebc9c8bcb9f676 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 25 Jun 2024 12:26:50 +0200 Subject: [PATCH] Extend AudioEncoderFactoryTemplate to pass Environment to AudioEncoder factory traits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:343086059 Change-Id: I1b8f066708ad630b9911bbcaacdc34542dd247ee Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/355480 Commit-Queue: Danil Chapovalov Reviewed-by: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#42531} --- .../audio_encoder_factory_template.h | 82 +++++++++++- ...audio_encoder_factory_template_unittest.cc | 121 ++++++++++++++++++ 2 files changed, 196 insertions(+), 7 deletions(-) diff --git a/api/audio_codecs/audio_encoder_factory_template.h b/api/audio_codecs/audio_encoder_factory_template.h index d3940aae85..14b1c5ea17 100644 --- a/api/audio_codecs/audio_encoder_factory_template.h +++ b/api/audio_codecs/audio_encoder_factory_template.h @@ -55,6 +55,65 @@ struct Helper<> { } }; +// Use ranked overloads (abseil.io/tips/229) for dispatching. +struct Rank0 {}; +struct Rank1 : Rank0 {}; + +template (), + std::declval(), + std::declval())), + std::unique_ptr>>> +absl::Nullable> CreateEncoder( + Rank1, + const Environment& env, + const typename Trait::Config& config, + const AudioEncoderFactory::Options& options) { + return Trait::MakeAudioEncoder(env, config, options); +} + +template (), + int{}, + std::declval>())), + std::unique_ptr>>> +absl::Nullable> CreateEncoder( + Rank0, + const Environment& env, + const typename Trait::Config& config, + const AudioEncoderFactory::Options& options) { + return Trait::MakeAudioEncoder(config, options.payload_type, + options.codec_pair_id); +} + +template (), + int{}, + std::declval>())), + std::unique_ptr>>> +absl::Nullable> LegacyCreateEncoder( + Rank1, + const typename Trait::Config& config, + int payload_type, + absl::optional codec_pair_ids) { + return Trait::MakeAudioEncoder(config, payload_type, codec_pair_ids); +} + +template +absl::Nullable> LegacyCreateEncoder( + Rank0, + const typename Trait::Config& config, + int payload_type, + absl::optional codec_pair_ids) { + RTC_CHECK_NOTREACHED(); +} + // Inductive case: Called with n + 1 template parameters; calls subroutines // with n template parameters. template @@ -80,7 +139,8 @@ struct Helper { absl::optional codec_pair_id) { auto opt_config = T::SdpToConfig(format); if (opt_config) { - return T::MakeAudioEncoder(*opt_config, payload_type, codec_pair_id); + return LegacyCreateEncoder(Rank1{}, *opt_config, payload_type, + codec_pair_id); } else { return Helper::MakeAudioEncoder(payload_type, format, codec_pair_id); @@ -92,10 +152,7 @@ struct Helper { const SdpAudioFormat& format, const AudioEncoderFactory::Options& options) { if (auto opt_config = T::SdpToConfig(format); opt_config.has_value()) { - // TODO: bugs.webrtc.org/343086059 - propagate `env` to the individual - // encoder factory functions. - return T::MakeAudioEncoder(*opt_config, options.payload_type, - options.codec_pair_id); + return CreateEncoder(Rank1{}, env, *opt_config, options); } return Helper::CreateAudioEncoder(env, format, options); } @@ -151,8 +208,13 @@ class AudioEncoderFactoryT : public AudioEncoderFactory { // AudioCodecInfo QueryAudioEncoder(const ConfigType& config); // // // Creates an AudioEncoder for the specified format. Used to implement -// // AudioEncoderFactory::MakeAudioEncoder(). -// std::unique_ptr MakeAudioEncoder( +// // AudioEncoderFactory::Create. +// std::unique_ptr MakeAudioEncoder( +// const Environment& env, +// const ConfigType& config, +// const AudioEncoderFactory::Options& options); +// or +// std::unique_ptr MakeAudioEncoder( // const ConfigType& config, // int payload_type, // absl::optional codec_pair_id); @@ -160,6 +222,12 @@ class AudioEncoderFactoryT : public AudioEncoderFactory { // ConfigType should be a type that encapsulates all the settings needed to // create an AudioEncoder. T::Config (where T is the encoder struct) should // either be the config type, or an alias for it. +// When both MakeAudioEncoder signatures are present, 1st one is preferred. +// +// Note: AudioEncoderFactory::MakeAudioEncoder factory can't use 1st signature, +// and thus uses 2nd signature when available, crashes otherwise. +// TODO: bugs.webrtc.org/343086059 - Remove the note above when MakeAudioEncoder +// is removed. // // Whenever it tries to do something, the new factory will try each of the // encoders in the order they were specified in the template argument list, diff --git a/api/audio_codecs/test/audio_encoder_factory_template_unittest.cc b/api/audio_codecs/test/audio_encoder_factory_template_unittest.cc index edfd456a1b..fdf04de926 100644 --- a/api/audio_codecs/test/audio_encoder_factory_template_unittest.cc +++ b/api/audio_codecs/test/audio_encoder_factory_template_unittest.cc @@ -37,8 +37,10 @@ namespace webrtc { namespace { using ::testing::IsNull; +using ::testing::NiceMock; using ::testing::Pointer; using ::testing::Property; +using ::testing::Return; struct BogusParams { static SdpAudioFormat AudioFormat() { return {"bogus", 8000, 1}; } @@ -87,6 +89,125 @@ struct AudioEncoderFakeApi { } }; +// Trait to pass as template parameter to `CreateAudioEncoderFactory` with +// all the functions except the functions to create the audio encoder. +struct BaseAudioEncoderApi { + // Create Encoders with different sample rates depending if it is created + // through V1 or V2 method so that a test may detect which method was used. + static constexpr int kV1SameRate = 10'000; + static constexpr int kV2SameRate = 20'000; + + struct Config {}; + + static SdpAudioFormat AudioFormat() { return {"fake", 16'000, 2, {}}; } + static AudioCodecInfo CodecInfo() { return {16'000, 2, 23456}; } + + static absl::optional SdpToConfig( + const SdpAudioFormat& audio_format) { + return Config(); + } + + static void AppendSupportedEncoders(std::vector* specs) { + specs->push_back({AudioFormat(), CodecInfo()}); + } + + static AudioCodecInfo QueryAudioEncoder(const Config&) { return CodecInfo(); } +}; + +struct AudioEncoderApiWithV1Make : BaseAudioEncoderApi { + static std::unique_ptr MakeAudioEncoder( + const Config&, + int payload_type, + absl::optional codec_pair_id) { + auto encoder = std::make_unique>(); + ON_CALL(*encoder, SampleRateHz).WillByDefault(Return(kV1SameRate)); + return encoder; + } +}; + +struct AudioEncoderApiWithV2Make : BaseAudioEncoderApi { + static std::unique_ptr MakeAudioEncoder( + const Environment& env, + const Config& config, + const AudioEncoderFactory::Options& options) { + auto encoder = std::make_unique>(); + ON_CALL(*encoder, SampleRateHz).WillByDefault(Return(kV2SameRate)); + return encoder; + } +}; + +struct AudioEncoderApiWithBothV1AndV2Make : BaseAudioEncoderApi { + static std::unique_ptr MakeAudioEncoder( + const Config&, + int payload_type, + absl::optional codec_pair_id) { + auto encoder = std::make_unique>(); + ON_CALL(*encoder, SampleRateHz).WillByDefault(Return(kV1SameRate)); + return encoder; + } + + static std::unique_ptr MakeAudioEncoder( + const Environment& env, + const Config& config, + const AudioEncoderFactory::Options& options) { + auto encoder = std::make_unique>(); + ON_CALL(*encoder, SampleRateHz).WillByDefault(Return(kV2SameRate)); + return encoder; + } +}; + +TEST(AudioEncoderFactoryTemplateTest, + UsesV1MakeAudioEncoderWhenV2IsNotAvailable) { + const Environment env = CreateEnvironment(); + auto factory = CreateAudioEncoderFactory(); + + EXPECT_THAT( + factory->MakeAudioEncoder(17, BaseAudioEncoderApi::AudioFormat(), {}), + Pointer(Property(&AudioEncoder::SampleRateHz, + BaseAudioEncoderApi::kV1SameRate))); + + EXPECT_THAT(factory->Create(env, BaseAudioEncoderApi::AudioFormat(), {}), + Pointer(Property(&AudioEncoder::SampleRateHz, + BaseAudioEncoderApi::kV1SameRate))); +} + +TEST(AudioEncoderFactoryTemplateTest, + PreferV2MakeAudioEncoderWhenBothAreAvailable) { + const Environment env = CreateEnvironment(); + auto factory = + CreateAudioEncoderFactory(); + + EXPECT_THAT(factory->Create(env, BaseAudioEncoderApi::AudioFormat(), {}), + Pointer(Property(&AudioEncoder::SampleRateHz, + BaseAudioEncoderApi::kV2SameRate))); + + // For backward compatibility legacy AudioEncoderFactory::MakeAudioEncoder + // still can be used, and uses older signature of the Trait::MakeAudioEncoder. + EXPECT_THAT( + factory->MakeAudioEncoder(17, BaseAudioEncoderApi::AudioFormat(), {}), + Pointer(Property(&AudioEncoder::SampleRateHz, + BaseAudioEncoderApi::kV1SameRate))); +} + +TEST(AudioEncoderFactoryTemplateTest, CanUseTraitWithOnlyV2MakeAudioEncoder) { + const Environment env = CreateEnvironment(); + auto factory = CreateAudioEncoderFactory(); + EXPECT_THAT(factory->Create(env, BaseAudioEncoderApi::AudioFormat(), {}), + Pointer(Property(&AudioEncoder::SampleRateHz, + BaseAudioEncoderApi::kV2SameRate))); +} + +#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) +TEST(AudioEncoderFactoryTemplateTest, CrashesWhenV2OnlyTraitUsedWithOlderApi) { + auto factory = CreateAudioEncoderFactory(); + // V2 signature requires Environment that + // AudioEncoderFactory::MakeAudioEncoder doesn't provide. + EXPECT_DEATH( + factory->MakeAudioEncoder(17, BaseAudioEncoderApi::AudioFormat(), {}), + ""); +} +#endif + TEST(AudioEncoderFactoryTemplateTest, NoEncoderTypes) { test::ScopedKeyValueConfig field_trials; const Environment env = CreateEnvironment(&field_trials);