Fix payload type assignment issue with SetCodecPreferences
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 <deadbeef@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34961}
diff --git a/pc/media_session.cc b/pc/media_session.cc
index 4bbb877..615dbff 100644
--- a/pc/media_session.cc
+++ b/pc/media_session.cc
@@ -944,17 +944,23 @@
}
}
+// `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 <typename Codecs>
static Codecs MatchCodecPreference(
const std::vector<webrtc::RtpCodecCapability>& codec_preferences,
- const Codecs& codecs) {
+ const Codecs& codecs,
+ const Codecs& supported_codecs) {
Codecs filtered_codecs;
std::set<std::string> 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 @@
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 @@
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 @@
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 @@
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 @@
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 d5d0b92..067dc14 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 <memory>
+#include <set>
#include <tuple>
#include "absl/algorithm/container.h"
@@ -846,6 +847,29 @@
return false;
}
+bool HasPayloadTypeConflict(const cricket::SessionDescription* desc) {
+ std::set<int> 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<FakeMediaEngine>();
@@ -1793,6 +1817,147 @@
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<cricket::FakeMediaEngine>();
+
+ std::vector<cricket::AudioCodec> 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<cricket::VideoCodec> 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<cricket::FakeMediaEngine>();
+
+ std::vector<cricket::AudioCodec> 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<cricket::VideoCodec> 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<cricket::FakeMediaEngine>();
+
+ std::vector<cricket::AudioCodec> 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<cricket::VideoCodec> 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,
