From f7fcfb7e66b555a95d514d04701c991c290c1780 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Thu, 9 Sep 2021 13:39:38 -0700 Subject: [PATCH] Fix payload type assignment issue with SetCodecPreferences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When SetCodecPreferences was used, the media session was adding codecs from a list that didn't have corrected payload type mappings. As a result, it's possible to generate offers or answers that use the same payload type for audio and video codecs, which is a clear violation. Bug: webrtc:12169 Change-Id: Ib7be73b4b3b4c57b8d2f374dba8b039c7a3df5a8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231620 Reviewed-by: Taylor Brandstetter Reviewed-by: Harald Alvestrand Reviewed-by: Henrik Boström Commit-Queue: Taylor Brandstetter Cr-Commit-Position: refs/heads/main@{#34961} --- pc/media_session.cc | 40 ++++--- pc/peer_connection_media_unittest.cc | 165 +++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 13 deletions(-) diff --git a/pc/media_session.cc b/pc/media_session.cc index 4bbb87782a..615dbff6c1 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -944,17 +944,23 @@ static void MergeCodecs(const std::vector& reference_codecs, } } +// `codecs` is a full list of codecs with correct payload type mappings, which +// don't conflict with mappings of the other media type; `supported_codecs` is +// a list filtered for the media section`s direction but with default payload +// types. template static Codecs MatchCodecPreference( const std::vector& codec_preferences, - const Codecs& codecs) { + const Codecs& codecs, + const Codecs& supported_codecs) { Codecs filtered_codecs; std::set kept_codecs_ids; bool want_rtx = false; for (const auto& codec_preference : codec_preferences) { auto found_codec = absl::c_find_if( - codecs, [&codec_preference](const typename Codecs::value_type& codec) { + supported_codecs, + [&codec_preference](const typename Codecs::value_type& codec) { webrtc::RtpCodecParameters codec_parameters = codec.ToCodecParameters(); return codec_parameters.name == codec_preference.name && @@ -965,9 +971,13 @@ static Codecs MatchCodecPreference( codec_parameters.parameters == codec_preference.parameters; }); - if (found_codec != codecs.end()) { - filtered_codecs.push_back(*found_codec); - kept_codecs_ids.insert(std::to_string(found_codec->id)); + if (found_codec != supported_codecs.end()) { + typename Codecs::value_type found_codec_with_correct_pt; + if (FindMatchingCodec(supported_codecs, codecs, *found_codec, + &found_codec_with_correct_pt)) { + filtered_codecs.push_back(found_codec_with_correct_pt); + kept_codecs_ids.insert(std::to_string(found_codec_with_correct_pt.id)); + } } else if (IsRtxCodec(codec_preference)) { want_rtx = true; } @@ -2144,8 +2154,9 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer( if (!media_description_options.codec_preferences.empty()) { // Add the codecs from the current transceiver's codec preferences. // They override any existing codecs from previous negotiations. - filtered_codecs = MatchCodecPreference( - media_description_options.codec_preferences, supported_audio_codecs); + filtered_codecs = + MatchCodecPreference(media_description_options.codec_preferences, + audio_codecs, supported_audio_codecs); } else { // Add the codecs from current content if it exists and is not rejected nor // recycled. @@ -2233,8 +2244,9 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( if (!media_description_options.codec_preferences.empty()) { // Add the codecs from the current transceiver's codec preferences. // They override any existing codecs from previous negotiations. - filtered_codecs = MatchCodecPreference( - media_description_options.codec_preferences, supported_video_codecs); + filtered_codecs = + MatchCodecPreference(media_description_options.codec_preferences, + video_codecs, supported_video_codecs); } else { // Add the codecs from current content if it exists and is not rejected nor // recycled. @@ -2424,8 +2436,9 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( AudioCodecs filtered_codecs; if (!media_description_options.codec_preferences.empty()) { - filtered_codecs = MatchCodecPreference( - media_description_options.codec_preferences, supported_audio_codecs); + filtered_codecs = + MatchCodecPreference(media_description_options.codec_preferences, + audio_codecs, supported_audio_codecs); } else { // Add the codecs from current content if it exists and is not rejected nor // recycled. @@ -2539,8 +2552,9 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( VideoCodecs filtered_codecs; if (!media_description_options.codec_preferences.empty()) { - filtered_codecs = MatchCodecPreference( - media_description_options.codec_preferences, supported_video_codecs); + filtered_codecs = + MatchCodecPreference(media_description_options.codec_preferences, + video_codecs, supported_video_codecs); } else { // Add the codecs from current content if it exists and is not rejected nor // recycled. diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc index d5d0b926b7..067dc14c50 100644 --- a/pc/peer_connection_media_unittest.cc +++ b/pc/peer_connection_media_unittest.cc @@ -13,6 +13,7 @@ // the media-related aspects of SDP. #include +#include #include #include "absl/algorithm/container.h" @@ -846,6 +847,29 @@ bool HasAnyComfortNoiseCodecs(const cricket::SessionDescription* desc) { return false; } +bool HasPayloadTypeConflict(const cricket::SessionDescription* desc) { + std::set payload_types; + const auto* audio_desc = cricket::GetFirstAudioContentDescription(desc); + if (audio_desc) { + for (const auto& codec : audio_desc->codecs()) { + if (payload_types.count(codec.id) > 0) { + return true; + } + payload_types.insert(codec.id); + } + } + const auto* video_desc = cricket::GetFirstVideoContentDescription(desc); + if (video_desc) { + for (const auto& codec : video_desc->codecs()) { + if (payload_types.count(codec.id) > 0) { + return true; + } + payload_types.insert(codec.id); + } + } + return false; +} + TEST_P(PeerConnectionMediaTest, CreateOfferWithNoVoiceActivityDetectionIncludesNoComfortNoiseCodecs) { auto fake_engine = std::make_unique(); @@ -1793,6 +1817,147 @@ TEST_F(PeerConnectionMediaTestUnifiedPlan, EXPECT_FALSE(HasAnyComfortNoiseCodecs(offer->description())); } +// If the "default" payload types of audio/video codecs are the same, and +// audio/video are bundled (as is the default), payload types should be +// remapped to avoid conflict, as normally happens without using +// SetCodecPreferences. +TEST_F(PeerConnectionMediaTestUnifiedPlan, + SetCodecPreferencesAvoidsPayloadTypeConflictInOffer) { + auto fake_engine = std::make_unique(); + + std::vector audio_codecs; + audio_codecs.emplace_back(100, "foo", 0, 0, 1); + audio_codecs.emplace_back(101, cricket::kRtxCodecName, 0, 0, 1); + audio_codecs.back().params[cricket::kCodecParamAssociatedPayloadType] = "100"; + fake_engine->SetAudioCodecs(audio_codecs); + + std::vector video_codecs; + video_codecs.emplace_back(100, "bar"); + video_codecs.emplace_back(101, cricket::kRtxCodecName); + video_codecs.back().params[cricket::kCodecParamAssociatedPayloadType] = "100"; + fake_engine->SetVideoCodecs(video_codecs); + + auto caller = CreatePeerConnectionWithAudioVideo(std::move(fake_engine)); + auto transceivers = caller->pc()->GetTransceivers(); + ASSERT_EQ(2u, transceivers.size()); + + auto audio_transceiver = caller->pc()->GetTransceivers()[0]; + auto capabilities = caller->pc_factory()->GetRtpSenderCapabilities( + cricket::MediaType::MEDIA_TYPE_AUDIO); + EXPECT_TRUE(audio_transceiver->SetCodecPreferences(capabilities.codecs).ok()); + + auto video_transceiver = caller->pc()->GetTransceivers()[1]; + capabilities = caller->pc_factory()->GetRtpSenderCapabilities( + cricket::MediaType::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(video_transceiver->SetCodecPreferences(capabilities.codecs).ok()); + + RTCOfferAnswerOptions options; + auto offer = caller->CreateOffer(options); + EXPECT_FALSE(HasPayloadTypeConflict(offer->description())); + // Sanity check that we got the primary codec and RTX. + EXPECT_EQ(2u, cricket::GetFirstAudioContentDescription(offer->description()) + ->codecs() + .size()); + EXPECT_EQ(2u, cricket::GetFirstVideoContentDescription(offer->description()) + ->codecs() + .size()); +} + +// Same as above, but preferences set for the answer. +TEST_F(PeerConnectionMediaTestUnifiedPlan, + SetCodecPreferencesAvoidsPayloadTypeConflictInAnswer) { + auto fake_engine = std::make_unique(); + + std::vector audio_codecs; + audio_codecs.emplace_back(100, "foo", 0, 0, 1); + audio_codecs.emplace_back(101, cricket::kRtxCodecName, 0, 0, 1); + audio_codecs.back().params[cricket::kCodecParamAssociatedPayloadType] = "100"; + fake_engine->SetAudioCodecs(audio_codecs); + + std::vector video_codecs; + video_codecs.emplace_back(100, "bar"); + video_codecs.emplace_back(101, cricket::kRtxCodecName); + video_codecs.back().params[cricket::kCodecParamAssociatedPayloadType] = "100"; + fake_engine->SetVideoCodecs(video_codecs); + + auto caller = CreatePeerConnectionWithAudioVideo(std::move(fake_engine)); + + RTCOfferAnswerOptions options; + caller->SetRemoteDescription(caller->CreateOffer(options)); + + auto transceivers = caller->pc()->GetTransceivers(); + ASSERT_EQ(2u, transceivers.size()); + + auto audio_transceiver = caller->pc()->GetTransceivers()[0]; + auto capabilities = caller->pc_factory()->GetRtpSenderCapabilities( + cricket::MediaType::MEDIA_TYPE_AUDIO); + EXPECT_TRUE(audio_transceiver->SetCodecPreferences(capabilities.codecs).ok()); + + auto video_transceiver = caller->pc()->GetTransceivers()[1]; + capabilities = caller->pc_factory()->GetRtpSenderCapabilities( + cricket::MediaType::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(video_transceiver->SetCodecPreferences(capabilities.codecs).ok()); + + auto answer = caller->CreateAnswer(options); + + EXPECT_FALSE(HasPayloadTypeConflict(answer->description())); + // Sanity check that we got the primary codec and RTX. + EXPECT_EQ(2u, cricket::GetFirstAudioContentDescription(answer->description()) + ->codecs() + .size()); + EXPECT_EQ(2u, cricket::GetFirstVideoContentDescription(answer->description()) + ->codecs() + .size()); +} + +// Same as above, but preferences set for a subsequent offer. +TEST_F(PeerConnectionMediaTestUnifiedPlan, + SetCodecPreferencesAvoidsPayloadTypeConflictInSubsequentOffer) { + auto fake_engine = std::make_unique(); + + std::vector audio_codecs; + audio_codecs.emplace_back(100, "foo", 0, 0, 1); + audio_codecs.emplace_back(101, cricket::kRtxCodecName, 0, 0, 1); + audio_codecs.back().params[cricket::kCodecParamAssociatedPayloadType] = "100"; + fake_engine->SetAudioCodecs(audio_codecs); + + std::vector video_codecs; + video_codecs.emplace_back(100, "bar"); + video_codecs.emplace_back(101, cricket::kRtxCodecName); + video_codecs.back().params[cricket::kCodecParamAssociatedPayloadType] = "100"; + fake_engine->SetVideoCodecs(video_codecs); + + auto caller = CreatePeerConnectionWithAudioVideo(std::move(fake_engine)); + + RTCOfferAnswerOptions options; + caller->SetRemoteDescription(caller->CreateOffer(options)); + caller->SetLocalDescription(caller->CreateAnswer(options)); + + auto transceivers = caller->pc()->GetTransceivers(); + ASSERT_EQ(2u, transceivers.size()); + + auto audio_transceiver = caller->pc()->GetTransceivers()[0]; + auto capabilities = caller->pc_factory()->GetRtpSenderCapabilities( + cricket::MediaType::MEDIA_TYPE_AUDIO); + EXPECT_TRUE(audio_transceiver->SetCodecPreferences(capabilities.codecs).ok()); + + auto video_transceiver = caller->pc()->GetTransceivers()[1]; + capabilities = caller->pc_factory()->GetRtpSenderCapabilities( + cricket::MediaType::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(video_transceiver->SetCodecPreferences(capabilities.codecs).ok()); + + auto reoffer = caller->CreateOffer(options); + + EXPECT_FALSE(HasPayloadTypeConflict(reoffer->description())); + // Sanity check that we got the primary codec and RTX. + EXPECT_EQ(2u, cricket::GetFirstAudioContentDescription(reoffer->description()) + ->codecs() + .size()); + EXPECT_EQ(2u, cricket::GetFirstVideoContentDescription(reoffer->description()) + ->codecs() + .size()); +} + INSTANTIATE_TEST_SUITE_P(PeerConnectionMediaTest, PeerConnectionMediaTest, Values(SdpSemantics::kPlanB,