From cd013b1d59eb6ac301b9bd872b1b85b12def7ae9 Mon Sep 17 00:00:00 2001 From: Alessio Bazzica Date: Thu, 21 Nov 2024 14:59:24 +0100 Subject: [PATCH] Opus decoder: stereo decoding by default (behind field trial) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `WebRTC-Audio-OpusDecodeStereoByDefault` field trial - Behind that field trial, `AudioDecoderOpus::SdpToConfig` uses 2 instead of 1 as default number of channels when the `stereo` codec param is unspecified - Instead of wiring up `FieldTrialsView` to `SdpToConfig`, which requires API changes that break downstream projects, a change in `AudioDecoderOpus::Config` is made to signal when the number of channels is forced via SDP config Bug: webrtc:379996136 Change-Id: If70eb19bc7e3bc74dd0423610cb04ae33ea602fe Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/368860 Commit-Queue: Alessio Bazzica Reviewed-by: Tomas Gunnarsson Reviewed-by: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#43440} --- api/BUILD.gn | 1 + api/audio_codecs/opus/BUILD.gn | 13 +++ api/audio_codecs/opus/audio_decoder_opus.cc | 65 ++++++----- api/audio_codecs/opus/audio_decoder_opus.h | 2 +- .../opus/audio_decoder_opus_unittest.cc | 109 ++++++++++++++++++ experiments/field_trials.py | 3 + .../builtin_audio_decoder_factory_unittest.cc | 4 +- 7 files changed, 165 insertions(+), 32 deletions(-) create mode 100644 api/audio_codecs/opus/audio_decoder_opus_unittest.cc diff --git a/api/BUILD.gn b/api/BUILD.gn index 935cb12c39..904447f05c 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -1589,6 +1589,7 @@ if (rtc_include_tests) { "../test:fileutils", "../test:rtc_expect_death", "../test:test_support", + "audio_codecs/opus:unittests", "environment:environment_unittests", "task_queue:task_queue_default_factory_unittests", "test/pclf:media_configuration", diff --git a/api/audio_codecs/opus/BUILD.gn b/api/audio_codecs/opus/BUILD.gn index 29b2999f1b..fad98a9619 100644 --- a/api/audio_codecs/opus/BUILD.gn +++ b/api/audio_codecs/opus/BUILD.gn @@ -98,3 +98,16 @@ rtc_library("audio_decoder_multiopus") { "//third_party/abseil-cpp/absl/strings", ] } + +rtc_library("unittests") { + visibility = [ "*" ] + testonly = true + sources = [ "audio_decoder_opus_unittest.cc" ] + deps = [ + ":audio_decoder_opus", + "../../../api/environment", + "../../../api/environment:environment_factory", + "../../../test:explicit_key_value_config", + "../../../test:test_support", + ] +} diff --git a/api/audio_codecs/opus/audio_decoder_opus.cc b/api/audio_codecs/opus/audio_decoder_opus.cc index 4db27a3fd2..15ddc35027 100644 --- a/api/audio_codecs/opus/audio_decoder_opus.cc +++ b/api/audio_codecs/opus/audio_decoder_opus.cc @@ -25,6 +25,14 @@ #include "rtc_base/checks.h" namespace webrtc { +namespace { + +int GetDefaultNumChannels(const FieldTrialsView& field_trials) { + return field_trials.IsEnabled("WebRTC-Audio-OpusDecodeStereoByDefault") ? 2 + : 1; +} + +} // namespace bool AudioDecoderOpus::Config::IsOk() const { if (sample_rate_hz != 16000 && sample_rate_hz != 48000) { @@ -32,40 +40,37 @@ bool AudioDecoderOpus::Config::IsOk() const { // well; we can add support for them when needed.) return false; } - if (num_channels != 1 && num_channels != 2) { - return false; - } - return true; + return !num_channels.has_value() || *num_channels == 1 || *num_channels == 2; } std::optional AudioDecoderOpus::SdpToConfig( const SdpAudioFormat& format) { - const auto num_channels = [&]() -> std::optional { - auto stereo = format.parameters.find("stereo"); - if (stereo != format.parameters.end()) { - if (stereo->second == "0") { - return 1; - } else if (stereo->second == "1") { - return 2; - } else { - return std::nullopt; // Bad stereo parameter. - } - } - return 1; // Default to mono. - }(); - if (absl::EqualsIgnoreCase(format.name, "opus") && - format.clockrate_hz == 48000 && format.num_channels == 2 && - num_channels) { - Config config; - config.num_channels = *num_channels; - if (!config.IsOk()) { - RTC_DCHECK_NOTREACHED(); - return std::nullopt; - } - return config; - } else { + if (!absl::EqualsIgnoreCase(format.name, "opus") || + format.clockrate_hz != 48000 || format.num_channels != 2) { return std::nullopt; } + + Config config; + + // Parse the "stereo" codec parameter. If set, it overrides the default number + // of channels. + const auto stereo_param = format.parameters.find("stereo"); + if (stereo_param != format.parameters.end()) { + if (stereo_param->second == "0") { + config.num_channels = 1; + } else if (stereo_param->second == "1") { + config.num_channels = 2; + } else { + // Malformed stereo parameter. + return std::nullopt; + } + } + + if (!config.IsOk()) { + RTC_DCHECK_NOTREACHED(); + return std::nullopt; + } + return config; } void AudioDecoderOpus::AppendSupportedDecoders( @@ -86,7 +91,9 @@ std::unique_ptr AudioDecoderOpus::MakeAudioDecoder( return nullptr; } return std::make_unique( - env.field_trials(), config.num_channels, config.sample_rate_hz); + env.field_trials(), + config.num_channels.value_or(GetDefaultNumChannels(env.field_trials())), + config.sample_rate_hz); } } // namespace webrtc diff --git a/api/audio_codecs/opus/audio_decoder_opus.h b/api/audio_codecs/opus/audio_decoder_opus.h index 802de98975..50cf80de71 100644 --- a/api/audio_codecs/opus/audio_decoder_opus.h +++ b/api/audio_codecs/opus/audio_decoder_opus.h @@ -29,7 +29,7 @@ struct RTC_EXPORT AudioDecoderOpus { struct Config { bool IsOk() const; // Checks if the values are currently OK. int sample_rate_hz = 48000; - int num_channels = 1; + std::optional num_channels; }; static std::optional SdpToConfig(const SdpAudioFormat& audio_format); static void AppendSupportedDecoders(std::vector* specs); diff --git a/api/audio_codecs/opus/audio_decoder_opus_unittest.cc b/api/audio_codecs/opus/audio_decoder_opus_unittest.cc new file mode 100644 index 0000000000..42ab0fadc2 --- /dev/null +++ b/api/audio_codecs/opus/audio_decoder_opus_unittest.cc @@ -0,0 +1,109 @@ +/* + * Copyright (c) 2024 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/audio_codecs/opus/audio_decoder_opus.h" + +#include + +#include "api/environment/environment.h" +#include "api/environment/environment_factory.h" +#include "test/explicit_key_value_config.h" +#include "test/gmock.h" +#include "test/gtest.h" + +namespace webrtc { +namespace { + +using test::ExplicitKeyValueConfig; +using ::testing::Field; +using ::testing::Optional; +using Config = AudioDecoderOpus::Config; + +enum class StereoParam { kUnset, kMono, kStereo }; + +SdpAudioFormat GetSdpAudioFormat(StereoParam param) { + SdpAudioFormat format("opus", 48000, 2); + switch (param) { + case StereoParam::kUnset: + // Do nothing. + break; + case StereoParam::kMono: + format.parameters.emplace("stereo", "0"); + break; + case StereoParam::kStereo: + format.parameters.emplace("stereo", "1"); + break; + } + return format; +} + +constexpr int kDefaultNumChannels = 1; +constexpr int kAlternativeNumChannels = 2; + +TEST(AudioDecoderOpusTest, SdpToConfigDoesNotSetNumChannels) { + const std::optional config = + AudioDecoderOpus::SdpToConfig(GetSdpAudioFormat(StereoParam::kUnset)); + + EXPECT_THAT(config, Optional(Field(&Config::num_channels, std::nullopt))); +} + +TEST(AudioDecoderOpusTest, SdpToConfigForcesMono) { + const std::optional config = + AudioDecoderOpus::SdpToConfig(GetSdpAudioFormat(StereoParam::kMono)); + + EXPECT_THAT(config, Optional(Field(&Config::num_channels, 1))); +} + +TEST(AudioDecoderOpusTest, SdpToConfigForcesStereo) { + const std::optional config = + AudioDecoderOpus::SdpToConfig(GetSdpAudioFormat(StereoParam::kStereo)); + + EXPECT_THAT(config, Optional(Field(&Config::num_channels, 2))); +} + +TEST(AudioDecoderOpusTest, MakeAudioDecoderForcesDefaultNumChannels) { + const Environment env = CreateEnvironment(); + auto decoder = AudioDecoderOpus::MakeAudioDecoder( + env, /*config=*/{.num_channels = std::nullopt}); + + EXPECT_EQ(decoder->Channels(), static_cast(kDefaultNumChannels)); +} + +TEST(AudioDecoderOpusTest, MakeAudioDecoderCannotForceDefaultNumChannels) { + const Environment env = CreateEnvironment(); + auto decoder = AudioDecoderOpus::MakeAudioDecoder( + env, /*config=*/{.num_channels = kAlternativeNumChannels}); + + EXPECT_EQ(decoder->Channels(), static_cast(kAlternativeNumChannels)); +} + +TEST(AudioDecoderOpusTest, MakeAudioDecoderForcesStereo) { + const Environment env = + CreateEnvironment(std::make_unique( + "WebRTC-Audio-OpusDecodeStereoByDefault/Enabled/")); + auto decoder = AudioDecoderOpus::MakeAudioDecoder( + env, + /*config=*/{.num_channels = std::nullopt}); + + EXPECT_EQ(decoder->Channels(), 2u); +} + +TEST(AudioDecoderOpusTest, MakeAudioDecoderCannotForceStereo) { + const Environment env = + CreateEnvironment(std::make_unique( + "WebRTC-Audio-OpusDecodeStereoByDefault/Enabled/")); + auto decoder = + AudioDecoderOpus::MakeAudioDecoder(env, /*config=*/{.num_channels = 1}); + + EXPECT_EQ(decoder->Channels(), 1u); +} + +} // namespace +} // namespace webrtc diff --git a/experiments/field_trials.py b/experiments/field_trials.py index e7b91e4f66..3eea5853ec 100755 --- a/experiments/field_trials.py +++ b/experiments/field_trials.py @@ -47,6 +47,9 @@ ACTIVE_FIELD_TRIALS: FrozenSet[FieldTrial] = frozenset([ FieldTrial('WebRTC-Audio-GainController2', 42232605, date(2024, 4, 1)), + FieldTrial('WebRTC-Audio-OpusDecodeStereoByDefault', + 379996136, + date(2025, 11, 15)), FieldTrial('WebRTC-Audio-OpusGeneratePlc', 42223518, date(2025, 4, 1)), diff --git a/modules/audio_coding/codecs/builtin_audio_decoder_factory_unittest.cc b/modules/audio_coding/codecs/builtin_audio_decoder_factory_unittest.cc index 7cd2dc0ffe..ac3703d42c 100644 --- a/modules/audio_coding/codecs/builtin_audio_decoder_factory_unittest.cc +++ b/modules/audio_coding/codecs/builtin_audio_decoder_factory_unittest.cc @@ -118,8 +118,8 @@ TEST(AudioDecoderFactoryTest, CreateOpus) { rtc::scoped_refptr adf = CreateBuiltinAudioDecoderFactory(); ASSERT_TRUE(adf); - // Opus supports 48 kHz, 2 channels, and wants a "stereo" parameter whose - // value is either "0" or "1". + // Opus supports 48 kHz and 2 channels. It is possible to specify a "stereo" + // parameter whose value is either "0" or "1". for (int hz : {8000, 16000, 32000, 48000}) { for (int channels : {0, 1, 2, 3}) { for (std::string stereo : {"XX", "0", "1", "2"}) {