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}
This commit is contained in:
Taylor Brandstetter
WebRTC LUCI CQ
parent
fb501792eb
commit
f7fcfb7e66
@ -944,17 +944,23 @@ static void MergeCodecs(const std::vector<C>& 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 <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 @@ 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.
|
||||
|
||||
@ -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 @@ bool HasAnyComfortNoiseCodecs(const cricket::SessionDescription* desc) {
|
||||
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 @@ 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<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,
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user