From a26d6ed26ff7fffaab0591bf6e65f81d469a13d1 Mon Sep 17 00:00:00 2001 From: Tomas Lundqvist Date: Fri, 27 Oct 2023 12:25:57 +0000 Subject: [PATCH] Makes sure that RED is not added twice to the list of codecs when it is used with Opus. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:15606 Change-Id: I3ab3ee287f5d2e3a0a46520608e5c0931e0bff90 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325180 Reviewed-by: Henrik Boström Commit-Queue: Tomas Lundqvist Cr-Commit-Position: refs/heads/main@{#41028} --- pc/BUILD.gn | 1 + pc/media_session.cc | 15 +++- pc/media_session_unittest.cc | 75 ++++++++++++++++++- ...er_connection_encodings_integrationtest.cc | 61 +++++++++++++++ 4 files changed, 145 insertions(+), 7 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index e9a49ca999..7c22a26d12 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -2106,6 +2106,7 @@ if (rtc_include_tests && !build_with_chromium) { ":rtc_pc", ":rtcp_mux_filter", ":rtp_media_utils", + ":rtp_parameters_conversion", ":rtp_transport", ":rtp_transport_internal", ":sctp_transport", diff --git a/pc/media_session.cc b/pc/media_session.cc index bc5d80be40..a763919c16 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1045,6 +1045,7 @@ std::vector MatchCodecPreference( want_red = true; } } + bool red_was_added = false; for (const auto& codec_preference : codec_preferences) { auto found_codec = absl::c_find_if( supported_codecs, [&codec_preference](const Codec& codec) { @@ -1062,7 +1063,13 @@ std::vector MatchCodecPreference( absl::optional found_codec_with_correct_pt = FindMatchingCodec( supported_codecs, codecs, *found_codec, field_trials); if (found_codec_with_correct_pt) { - filtered_codecs.push_back(*found_codec_with_correct_pt); + // RED may already have been added if its primary codec is before RED + // in the codec list. + bool is_red_codec = IsRedCodec(*found_codec_with_correct_pt); + if (!is_red_codec || !red_was_added) { + filtered_codecs.push_back(*found_codec_with_correct_pt); + red_was_added = is_red_codec ? true : red_was_added; + } std::string id = rtc::ToString(found_codec_with_correct_pt->id); // Search for the matching rtx or red codec. if (want_red || want_rtx) { @@ -1083,11 +1090,11 @@ std::vector MatchCodecPreference( if (fmtp != codec.params.end()) { std::vector redundant_payloads = rtc::split(fmtp->second, '/'); - if (redundant_payloads.size() > 0 && + if (!redundant_payloads.empty() && redundant_payloads[0] == id) { - if (std::find(filtered_codecs.begin(), filtered_codecs.end(), - codec) == filtered_codecs.end()) { + if (!red_was_added) { filtered_codecs.push_back(codec); + red_was_added = true; } break; } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 9351c06518..a1770c18c5 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -26,6 +26,7 @@ #include "absl/types/optional.h" #include "api/candidate.h" #include "api/crypto_params.h" +#include "api/rtp_parameters.h" #include "media/base/codec.h" #include "media/base/media_constants.h" #include "media/base/test_utils.h" @@ -35,6 +36,7 @@ #include "p2p/base/transport_info.h" #include "pc/media_protocol_names.h" #include "pc/rtp_media_utils.h" +#include "pc/rtp_parameters_conversion.h" #include "rtc_base/arraysize.h" #include "rtc_base/checks.h" #include "rtc_base/fake_ssl_identity.h" @@ -111,8 +113,16 @@ using ::testing::SizeIs; using webrtc::RtpExtension; using webrtc::RtpTransceiverDirection; +static AudioCodec createRedAudioCodec(absl::string_view encoding_id) { + AudioCodec red = cricket::CreateAudioCodec(63, "red", 48000, 2); + red.SetParam(cricket::kCodecParamNotInNameValueFormat, + std::string(encoding_id) + '/' + std::string(encoding_id)); + return red; +} + static const AudioCodec kAudioCodecs1[] = { - cricket::CreateAudioCodec(103, "ISAC", 16000, 1), + cricket::CreateAudioCodec(111, "opus", 48000, 2), + createRedAudioCodec("111"), cricket::CreateAudioCodec(102, "iLBC", 8000, 1), cricket::CreateAudioCodec(0, "PCMU", 8000, 1), cricket::CreateAudioCodec(8, "PCMA", 8000, 1), @@ -820,6 +830,65 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) { EXPECT_EQ(cricket::kMediaProtocolSavpf, acd->protocol()); } +// Create an offer with just Opus and RED. +TEST_F(MediaSessionDescriptionFactoryTest, + TestCreateAudioOfferWithJustOpusAndRed) { + f1_.set_secure(SEC_ENABLED); + // First, prefer to only use opus and red. + std::vector preferences; + preferences.push_back( + webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[0])); + preferences.push_back( + webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[1])); + EXPECT_EQ("opus", preferences[0].name); + EXPECT_EQ("red", preferences[1].name); + + auto opts = CreatePlanBMediaSessionOptions(); + opts.media_description_options.at(0).codec_preferences = preferences; + std::unique_ptr offer = + f1_.CreateOfferOrError(opts, nullptr).MoveValue(); + ASSERT_TRUE(offer.get()); + const ContentInfo* ac = offer->GetContentByName("audio"); + const ContentInfo* vc = offer->GetContentByName("video"); + ASSERT_TRUE(ac != NULL); + ASSERT_TRUE(vc == NULL); + EXPECT_EQ(MediaProtocolType::kRtp, ac->type); + const AudioContentDescription* acd = ac->media_description()->as_audio(); + EXPECT_EQ(MEDIA_TYPE_AUDIO, acd->type()); + EXPECT_EQ(2U, acd->codecs().size()); + EXPECT_EQ("opus", acd->codecs()[0].name); + EXPECT_EQ("red", acd->codecs()[1].name); +} + +// Create an offer with RED before Opus, which enables RED with Opus encoding. +TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOfferWithRedForOpus) { + f1_.set_secure(SEC_ENABLED); + // First, prefer to only use opus and red. + std::vector preferences; + preferences.push_back( + webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[1])); + preferences.push_back( + webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[0])); + EXPECT_EQ("red", preferences[0].name); + EXPECT_EQ("opus", preferences[1].name); + + auto opts = CreatePlanBMediaSessionOptions(); + opts.media_description_options.at(0).codec_preferences = preferences; + std::unique_ptr offer = + f1_.CreateOfferOrError(opts, nullptr).MoveValue(); + ASSERT_TRUE(offer.get()); + const ContentInfo* ac = offer->GetContentByName("audio"); + const ContentInfo* vc = offer->GetContentByName("video"); + ASSERT_TRUE(ac != NULL); + ASSERT_TRUE(vc == NULL); + EXPECT_EQ(MediaProtocolType::kRtp, ac->type); + const AudioContentDescription* acd = ac->media_description()->as_audio(); + EXPECT_EQ(MEDIA_TYPE_AUDIO, acd->type()); + EXPECT_EQ(2U, acd->codecs().size()); + EXPECT_EQ("red", acd->codecs()[0].name); + EXPECT_EQ("opus", acd->codecs()[1].name); +} + // Create a typical video offer, and ensure it matches what we expect. TEST_F(MediaSessionDescriptionFactoryTest, TestCreateVideoOffer) { MediaSessionOptions opts; @@ -4663,13 +4732,13 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestSetAudioCodecs) { MAKE_VECTOR(kAudioCodecsAnswer); const std::vector no_codecs; - RTC_CHECK_EQ(send_codecs[1].name, "iLBC") + RTC_CHECK_EQ(send_codecs[2].name, "iLBC") << "Please don't change shared test data!"; RTC_CHECK_EQ(recv_codecs[2].name, "iLBC") << "Please don't change shared test data!"; // Alter iLBC send codec to have zero channels, to test that that is handled // properly. - send_codecs[1].channels = 0; + send_codecs[2].channels = 0; // Alter iLBC receive codec to be lowercase, to test that case conversions // are handled properly. diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index 4a786e0733..c7181c53ae 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -1678,6 +1678,67 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, EXPECT_FALSE(parameters.encodings[0].codec); } +TEST_F(PeerConnectionEncodingsIntegrationTest, + EncodingParametersRedEnabledBeforeNegotiationAudio) { + rtc::scoped_refptr local_pc_wrapper = CreatePc(); + rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + std::vector send_codecs = + local_pc_wrapper->pc_factory() + ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_AUDIO) + .codecs; + + absl::optional opus = + local_pc_wrapper->FindFirstSendCodecWithName(cricket::MEDIA_TYPE_AUDIO, + "opus"); + ASSERT_TRUE(opus); + + absl::optional red = + local_pc_wrapper->FindFirstSendCodecWithName(cricket::MEDIA_TYPE_AUDIO, + "red"); + ASSERT_TRUE(red); + + webrtc::RtpTransceiverInit init; + init.direction = webrtc::RtpTransceiverDirection::kSendOnly; + webrtc::RtpEncodingParameters encoding_parameters; + encoding_parameters.codec = opus; + init.send_encodings.push_back(encoding_parameters); + + auto transceiver_or_error = + local_pc_wrapper->pc()->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init); + ASSERT_TRUE(transceiver_or_error.ok()); + rtc::scoped_refptr audio_transceiver = + transceiver_or_error.MoveValue(); + + // Preferring RED over Opus should enable RED with Opus encoding. + send_codecs[0] = red.value(); + send_codecs[1] = opus.value(); + + ASSERT_TRUE(audio_transceiver->SetCodecPreferences(send_codecs).ok()); + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + webrtc::RtpParameters parameters = + audio_transceiver->sender()->GetParameters(); + EXPECT_EQ(parameters.encodings[0].codec, opus); + EXPECT_EQ(parameters.codecs[0].payload_type, red->preferred_payload_type); + EXPECT_EQ(parameters.codecs[0].name, red->name); + + // Check that it's possible to switch back to Opus without RED. + send_codecs[0] = opus.value(); + send_codecs[1] = red.value(); + + ASSERT_TRUE(audio_transceiver->SetCodecPreferences(send_codecs).ok()); + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + + parameters = audio_transceiver->sender()->GetParameters(); + EXPECT_EQ(parameters.encodings[0].codec, opus); + EXPECT_EQ(parameters.codecs[0].payload_type, opus->preferred_payload_type); + EXPECT_EQ(parameters.codecs[0].name, opus->name); +} + TEST_F(PeerConnectionEncodingsIntegrationTest, SetParametersRejectsScalabilityModeForSelectedCodec) { rtc::scoped_refptr local_pc_wrapper = CreatePc();