sdp: make msid support parsing more robust

by also taking into account any a=msid: line in addition to
msid-semantic. Also document issues with msid-semantic generation and unify support determination by removing the msid_supported flag.

BUG=webrtc:10421

Change-Id: Icea554ebd1998f2b526846457029eff6854a772a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/329760
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41386}
This commit is contained in:
Philipp Hancke 2023-12-12 13:15:03 +01:00 committed by WebRTC LUCI CQ
parent c202f9635e
commit 6f0f158af0
9 changed files with 141 additions and 72 deletions

View File

@ -1881,11 +1881,13 @@ MediaSessionDescriptionFactory::CreateOfferOrError(
// Be conservative and signal using both a=msid and a=ssrc lines. Unified
// Plan answerers will look at a=msid and Plan B answerers will look at the
// a=ssrc MSID line.
offer->set_msid_signaling(cricket::kMsidSignalingMediaSection |
offer->set_msid_signaling(cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute);
} else {
// Plan B always signals MSID using a=ssrc lines.
offer->set_msid_signaling(cricket::kMsidSignalingSsrcAttribute);
offer->set_msid_signaling(cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingSsrcAttribute);
}
offer->set_extmap_allow_mixed(session_options.offer_extmap_allow_mixed);
@ -2058,7 +2060,9 @@ MediaSessionDescriptionFactory::CreateAnswerOrError(
if (is_unified_plan_) {
// Unified Plan needs to look at what the offer included to find the most
// compatible answer.
if (offer->msid_signaling() == 0) {
int msid_signaling = offer->msid_signaling();
if (msid_signaling == cricket::kMsidSignalingNotUsed ||
msid_signaling == cricket::kMsidSignalingSemantic) {
// We end up here in one of three cases:
// 1. An empty offer. We'll reply with an empty answer so it doesn't
// matter what we pick here.
@ -2067,23 +2071,26 @@ MediaSessionDescriptionFactory::CreateAnswerOrError(
// 3. Media that's either sendonly or inactive from the remote endpoint.
// We don't have any information to say whether the endpoint is Plan B
// or Unified Plan, so be conservative and send both.
answer->set_msid_signaling(cricket::kMsidSignalingMediaSection |
answer->set_msid_signaling(cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute);
} else if (offer->msid_signaling() ==
(cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute)) {
} else if (msid_signaling == (cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute)) {
// If both a=msid and a=ssrc MSID signaling methods were used, we're
// probably talking to a Unified Plan endpoint so respond with just
// a=msid.
answer->set_msid_signaling(cricket::kMsidSignalingMediaSection);
answer->set_msid_signaling(cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingMediaSection);
} else {
// Otherwise, it's clear which method the offerer is using so repeat that
// back to them.
answer->set_msid_signaling(offer->msid_signaling());
answer->set_msid_signaling(msid_signaling);
}
} else {
// Plan B always signals MSID using a=ssrc lines.
answer->set_msid_signaling(cricket::kMsidSignalingSsrcAttribute);
answer->set_msid_signaling(cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingSsrcAttribute);
}
return answer;

View File

@ -2814,7 +2814,7 @@ TEST_F(PeerConnectionInterfaceTestPlanB,
// This tests that a default MediaStream is not created if a remote session
// description is updated to not have any MediaStreams.
// Don't run under Unified Plan since this behavior is Plan B specific.
TEST_F(PeerConnectionInterfaceTestPlanB, VerifyDefaultStreamIsNotCreated) {
TEST_F(PeerConnectionInterfaceTestPlanB, VerifyDefaultStreamIsNotRecreated) {
RTCConfiguration config;
CreatePeerConnection(config);
CreateAndSetRemoteOffer(GetSdpStringWithStream1());
@ -2822,7 +2822,7 @@ TEST_F(PeerConnectionInterfaceTestPlanB, VerifyDefaultStreamIsNotCreated) {
EXPECT_TRUE(
CompareStreamCollections(observer_.remote_streams(), reference.get()));
CreateAndSetRemoteOffer(kSdpStringWithoutStreams);
CreateAndSetRemoteOffer(kSdpStringWithMsidWithoutStreams);
EXPECT_EQ(0u, observer_.remote_streams()->count());
}

View File

@ -1836,14 +1836,16 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) {
// Offer should have had both a=msid and a=ssrc MSID lines.
auto* offer = callee->pc()->remote_description();
EXPECT_EQ((cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute),
offer->description()->msid_signaling());
EXPECT_EQ(
(cricket::kMsidSignalingSemantic | cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute),
offer->description()->msid_signaling());
// Answer should have had only a=msid lines.
auto* answer = caller->pc()->remote_description();
EXPECT_EQ(cricket::kMsidSignalingMediaSection,
answer->description()->msid_signaling());
EXPECT_EQ(
cricket::kMsidSignalingSemantic | cricket::kMsidSignalingMediaSection,
answer->description()->msid_signaling());
}
TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) {
@ -1856,13 +1858,15 @@ TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) {
// Offer should have only a=ssrc MSID lines.
auto* offer = callee->pc()->remote_description();
EXPECT_EQ(cricket::kMsidSignalingSsrcAttribute,
offer->description()->msid_signaling());
EXPECT_EQ(
cricket::kMsidSignalingSemantic | cricket::kMsidSignalingSsrcAttribute,
offer->description()->msid_signaling());
// Answer should have only a=ssrc MSID lines to match the offer.
auto* answer = caller->pc()->remote_description();
EXPECT_EQ(cricket::kMsidSignalingSsrcAttribute,
answer->description()->msid_signaling());
EXPECT_EQ(
cricket::kMsidSignalingSemantic | cricket::kMsidSignalingSsrcAttribute,
answer->description()->msid_signaling());
}
// This tests that a Plan B endpoint appropriately sets the remote description
@ -1884,9 +1888,10 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanToPlanBAnswer) {
// Offer should have had both a=msid and a=ssrc MSID lines.
auto* offer = callee->pc()->remote_description();
EXPECT_EQ((cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute),
offer->description()->msid_signaling());
EXPECT_EQ(
(cricket::kMsidSignalingSemantic | cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute),
offer->description()->msid_signaling());
// Callee should always have 1 stream for all of it's receivers.
const auto& track_events = callee->observer()->add_track_events_;
@ -1907,7 +1912,8 @@ TEST_F(PeerConnectionMsidSignalingTest, PureUnifiedPlanToUs) {
auto offer = caller->CreateOffer();
// Simulate a pure Unified Plan offerer by setting the MSID signaling to media
// section only.
offer->description()->set_msid_signaling(cricket::kMsidSignalingMediaSection);
offer->description()->set_msid_signaling(cricket::kMsidSignalingSemantic |
cricket::kMsidSignalingMediaSection);
ASSERT_TRUE(
caller->SetLocalDescription(CloneSessionDescription(offer.get())));
@ -1915,8 +1921,9 @@ TEST_F(PeerConnectionMsidSignalingTest, PureUnifiedPlanToUs) {
// Answer should have only a=msid to match the offer.
auto answer = callee->CreateAnswer();
EXPECT_EQ(cricket::kMsidSignalingMediaSection,
answer->description()->msid_signaling());
EXPECT_EQ(
cricket::kMsidSignalingSemantic | cricket::kMsidSignalingMediaSection,
answer->description()->msid_signaling());
}
// Sender setups in a call.

View File

@ -2078,24 +2078,16 @@ void SdpOfferAnswerHandler::ApplyRemoteDescription(
if (operation->unified_plan()) {
ApplyRemoteDescriptionUpdateTransceiverState(operation->type());
}
const cricket::AudioContentDescription* audio_desc =
GetFirstAudioContentDescription(remote_description()->description());
const cricket::VideoContentDescription* video_desc =
GetFirstVideoContentDescription(remote_description()->description());
// Check if the descriptions include streams, just in case the peer supports
// MSID, but doesn't indicate so with "a=msid-semantic".
if (remote_description()->description()->msid_supported() ||
(audio_desc && !audio_desc->streams().empty()) ||
(video_desc && !video_desc->streams().empty())) {
remote_peer_supports_msid_ = true;
}
remote_peer_supports_msid_ =
remote_description()->description()->msid_signaling() !=
cricket::kMsidSignalingNotUsed;
if (!operation->unified_plan()) {
PlanBUpdateSendersAndReceivers(
GetFirstAudioContent(remote_description()->description()), audio_desc,
GetFirstVideoContent(remote_description()->description()), video_desc);
GetFirstAudioContent(remote_description()->description()),
GetFirstAudioContentDescription(remote_description()->description()),
GetFirstVideoContent(remote_description()->description()),
GetFirstVideoContentDescription(remote_description()->description()));
}
if (operation->type() == SdpType::kAnswer) {

View File

@ -1005,6 +1005,53 @@ TEST_F(SdpOfferAnswerTest, SdpMungingWithInvalidPayloadTypeIsRejected) {
}
}
TEST_F(SdpOfferAnswerTest, MsidSignalingInSubsequentOfferAnswer) {
auto pc = CreatePeerConnection();
pc->AddAudioTrack("audio_track", {});
std::string sdp =
"v=0\r\n"
"o=- 0 3 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
"t=0 0\r\n"
"a=msid-semantic: WMS\r\n"
"a=fingerprint:sha-1 "
"4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n"
"a=setup:actpass\r\n"
"a=ice-ufrag:ETEn\r\n"
"a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n"
"m=audio 9 UDP/TLS/RTP/SAVPF 111\r\n"
"c=IN IP4 0.0.0.0\r\n"
"a=rtcp:9 IN IP4 0.0.0.0\r\n"
"a=recvonly\r\n"
"a=rtcp-mux\r\n"
"a=rtpmap:111 opus/48000/2\r\n";
auto offer = CreateSessionDescription(SdpType::kOffer, sdp);
EXPECT_TRUE(pc->SetRemoteDescription(std::move(offer)));
// Check the generated SDP.
auto answer = pc->CreateAnswer();
answer->ToString(&sdp);
EXPECT_NE(std::string::npos, sdp.find("a=msid:- audio_track\r\n"));
EXPECT_TRUE(pc->SetLocalDescription(std::move(answer)));
// Check the local description object.
auto local_description = pc->pc()->local_description();
ASSERT_EQ(local_description->description()->contents().size(), 1u);
auto streams = local_description->description()
->contents()[0]
.media_description()
->streams();
ASSERT_EQ(streams.size(), 1u);
EXPECT_EQ(streams[0].id, "audio_track");
// Check the serialization of the local description.
local_description->ToString(&sdp);
EXPECT_NE(std::string::npos, sdp.find("a=msid:- audio_track\r\n"));
}
// Test variant with boolean order for audio-video and video-audio.
class SdpOfferAnswerShuffleMediaTypes
: public SdpOfferAnswerTest,

View File

@ -468,13 +468,23 @@ const ContentInfo* FindContentInfoByName(const ContentInfos& contents,
const ContentInfo* FindContentInfoByType(const ContentInfos& contents,
const std::string& type);
// Determines how the MSID will be signaled in the SDP. These can be used as
// flags to indicate both or none.
// Determines how the MSID will be signaled in the SDP.
// These can be used as bit flags to indicate both or the special value none.
enum MsidSignaling {
// Signal MSID with one a=msid line in the media section.
// MSID is not signaled. This is not a bit flag and must be compared for
// equality.
kMsidSignalingNotUsed = 0x0,
// Signal MSID with at least one a=msid line in the media section.
// This requires unified plan.
kMsidSignalingMediaSection = 0x1,
// Signal MSID with a=ssrc: msid lines in the media section.
kMsidSignalingSsrcAttribute = 0x2
// This should only be used with plan-b but is signalled in
// offers for backward compability reasons.
kMsidSignalingSsrcAttribute = 0x2,
// Signal MSID with a=msid-semantic: WMS in the session section.
// This is deprecated but signalled for backward compability reasons.
// It is typically combined with 0x1 or 0x2.
kMsidSignalingSemantic = 0x4
};
// Describes a collection of contents, each with its own name and
@ -548,9 +558,6 @@ class SessionDescription {
void RemoveGroupByName(const std::string& name);
// Global attributes.
void set_msid_supported(bool supported) { msid_supported_ = supported; }
bool msid_supported() const { return msid_supported_; }
// Determines how the MSIDs were/will be signaled. Flag value composed of
// MsidSignaling bits (see enum above).
void set_msid_signaling(int msid_signaling) {
@ -582,10 +589,9 @@ class SessionDescription {
ContentInfos contents_;
TransportInfos transport_infos_;
ContentGroups content_groups_;
bool msid_supported_ = true;
// Default to what Plan B would do.
// TODO(bugs.webrtc.org/8530): Change default to kMsidSignalingMediaSection.
int msid_signaling_ = kMsidSignalingSsrcAttribute;
int msid_signaling_ = kMsidSignalingSsrcAttribute | kMsidSignalingSemantic;
bool extmap_allow_mixed_ = true;
};

View File

@ -22,7 +22,6 @@ void RemoveSsrcsAndMsids(cricket::SessionDescription* desc) {
for (ContentInfo& content : desc->contents()) {
content.media_description()->mutable_streams().clear();
}
desc->set_msid_supported(false);
desc->set_msid_signaling(0);
}

View File

@ -722,7 +722,7 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos,
// This is the case with Plan B SDP msid signaling.
stream_ids.push_back(ssrc_info.stream_id);
track_id = ssrc_info.track_id;
} else {
} else if (msid_signaling == cricket::kMsidSignalingNotUsed) {
// Since no media streams isn't supported with older SDP signaling, we
// use a default stream id.
stream_ids.push_back(kDefaultMsid);
@ -924,6 +924,9 @@ std::string SdpSerialize(const JsepSessionDescription& jdesc) {
InitAttrLine(kAttributeMsidSemantics, &os);
os << kSdpDelimiterColon << " " << kMediaStreamSemantic;
// TODO(bugs.webrtc.org/10421): this code only looks at the first audio/video
// content. Fixing that might result in much larger SDP and the msid-semantic
// line should eventually go away so this is not worth fixing.
std::set<std::string> media_stream_ids;
const ContentInfo* audio_content = GetFirstAudioContent(desc);
if (audio_content)
@ -2131,7 +2134,7 @@ bool ParseSessionDescription(absl::string_view message,
SdpParseError* error) {
absl::optional<absl::string_view> line;
desc->set_msid_supported(false);
desc->set_msid_signaling(cricket::kMsidSignalingNotUsed);
desc->set_extmap_allow_mixed(false);
// RFC 4566
// v= (protocol version)
@ -2273,8 +2276,9 @@ bool ParseSessionDescription(absl::string_view message,
if (!GetValue(*aline, kAttributeMsidSemantics, &semantics, error)) {
return false;
}
desc->set_msid_supported(
CaseInsensitiveFind(semantics, kMediaStreamSemantic));
if (CaseInsensitiveFind(semantics, kMediaStreamSemantic)) {
desc->set_msid_signaling(cricket::kMsidSignalingSemantic);
}
} else if (HasAttribute(*aline, kAttributeExtmapAllowMixed)) {
desc->set_extmap_allow_mixed(true);
} else if (HasAttribute(*aline, kAttributeExtmap)) {
@ -2694,7 +2698,7 @@ bool ParseMediaDescription(
SdpParseError* error) {
RTC_DCHECK(desc != NULL);
int mline_index = -1;
int msid_signaling = 0;
int msid_signaling = desc->msid_signaling();
// Zero or more media descriptions
// RFC 4566
@ -2746,7 +2750,7 @@ bool ParseMediaDescription(
std::unique_ptr<MediaContentDescription> content;
std::string content_name;
bool bundle_only = false;
int section_msid_signaling = 0;
int section_msid_signaling = cricket::kMsidSignalingNotUsed;
absl::string_view media_type = fields[0];
if ((media_type == kMediaTypeVideo || media_type == kMediaTypeAudio) &&
!cricket::IsRtpProtocol(protocol)) {

View File

@ -1219,7 +1219,8 @@ class WebRtcSdpTest : public ::testing::Test {
absl::WrapUnique(video_desc_3));
desc_.AddTransportInfo(TransportInfo(
kVideoContentName3, TransportDescription(kUfragVideo3, kPwdVideo3)));
desc_.set_msid_signaling(cricket::kMsidSignalingMediaSection);
desc_.set_msid_signaling(cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSemantic);
ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(),
jdesc_.session_version()));
@ -1299,7 +1300,8 @@ class WebRtcSdpTest : public ::testing::Test {
absl::WrapUnique(audio_desc));
// Enable signaling a=msid lines.
desc_.set_msid_signaling(cricket::kMsidSignalingMediaSection);
desc_.set_msid_signaling(cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSemantic);
ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(),
jdesc_.session_version()));
}
@ -1508,7 +1510,7 @@ class WebRtcSdpTest : public ::testing::Test {
}
// global attributes
EXPECT_EQ(desc1.msid_supported(), desc2.msid_supported());
EXPECT_EQ(desc1.msid_signaling(), desc2.msid_signaling());
EXPECT_EQ(desc1.extmap_allow_mixed(), desc2.extmap_allow_mixed());
}
@ -3751,7 +3753,7 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionSpecialMsid) {
// Create both msid lines for Plan B and Unified Plan support.
MakeUnifiedPlanDescriptionMultipleStreamIds(
cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute);
cricket::kMsidSignalingSsrcAttribute | cricket::kMsidSignalingSemantic);
JsepSessionDescription deserialized_description(kDummyType);
EXPECT_TRUE(SdpDeserialize(kUnifiedPlanSdpFullStringWithSpecialMsid,
@ -3759,7 +3761,8 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionSpecialMsid) {
EXPECT_TRUE(CompareSessionDescription(jdesc_, deserialized_description));
EXPECT_EQ(cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute,
cricket::kMsidSignalingSsrcAttribute |
cricket::kMsidSignalingSemantic,
deserialized_description.description()->msid_signaling());
}
@ -3771,7 +3774,7 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionSpecialMsid) {
// Create both msid lines for Plan B and Unified Plan support.
MakeUnifiedPlanDescriptionMultipleStreamIds(
cricket::kMsidSignalingMediaSection |
cricket::kMsidSignalingSsrcAttribute);
cricket::kMsidSignalingSsrcAttribute | cricket::kMsidSignalingSemantic);
std::string serialized_sdp = webrtc::SdpSerialize(jdesc_);
// We explicitly test that the serialized SDP string is equal to the hard
// coded SDP string. This is necessary, because in the parser "a=msid" lines
@ -3787,7 +3790,7 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionSpecialMsid) {
TEST_F(WebRtcSdpTest, UnifiedPlanDeserializeSessionDescriptionSpecialMsid) {
// Only create a=msid lines for strictly Unified Plan stream ID support.
MakeUnifiedPlanDescriptionMultipleStreamIds(
cricket::kMsidSignalingMediaSection);
cricket::kMsidSignalingMediaSection | cricket::kMsidSignalingSemantic);
JsepSessionDescription deserialized_description(kDummyType);
std::string unified_plan_sdp_string =
@ -3805,7 +3808,7 @@ TEST_F(WebRtcSdpTest, UnifiedPlanDeserializeSessionDescriptionSpecialMsid) {
TEST_F(WebRtcSdpTest, UnifiedPlanSerializeSessionDescriptionSpecialMsid) {
// Only create a=msid lines for strictly Unified Plan stream ID support.
MakeUnifiedPlanDescriptionMultipleStreamIds(
cricket::kMsidSignalingMediaSection);
cricket::kMsidSignalingMediaSection | cricket::kMsidSignalingSemantic);
TestSerialize(jdesc_);
}
@ -3837,7 +3840,8 @@ TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescriptionNoSsrcSignaling) {
TEST_F(WebRtcSdpTest, EmptyDescriptionHasNoMsidSignaling) {
JsepSessionDescription jsep_desc(kDummyType);
ASSERT_TRUE(SdpDeserialize(kSdpSessionString, &jsep_desc));
EXPECT_EQ(0, jsep_desc.description()->msid_signaling());
EXPECT_EQ(cricket::kMsidSignalingSemantic,
jsep_desc.description()->msid_signaling());
}
TEST_F(WebRtcSdpTest, DataChannelOnlyHasNoMsidSignaling) {
@ -3845,21 +3849,24 @@ TEST_F(WebRtcSdpTest, DataChannelOnlyHasNoMsidSignaling) {
std::string sdp = kSdpSessionString;
sdp += kSdpSctpDataChannelString;
ASSERT_TRUE(SdpDeserialize(sdp, &jsep_desc));
EXPECT_EQ(0, jsep_desc.description()->msid_signaling());
EXPECT_EQ(cricket::kMsidSignalingSemantic,
jsep_desc.description()->msid_signaling());
}
TEST_F(WebRtcSdpTest, PlanBHasSsrcAttributeMsidSignaling) {
JsepSessionDescription jsep_desc(kDummyType);
ASSERT_TRUE(SdpDeserialize(kPlanBSdpFullString, &jsep_desc));
EXPECT_EQ(cricket::kMsidSignalingSsrcAttribute,
jsep_desc.description()->msid_signaling());
EXPECT_EQ(
cricket::kMsidSignalingSsrcAttribute | cricket::kMsidSignalingSemantic,
jsep_desc.description()->msid_signaling());
}
TEST_F(WebRtcSdpTest, UnifiedPlanHasMediaSectionMsidSignaling) {
JsepSessionDescription jsep_desc(kDummyType);
ASSERT_TRUE(SdpDeserialize(kUnifiedPlanSdpFullString, &jsep_desc));
EXPECT_EQ(cricket::kMsidSignalingMediaSection,
jsep_desc.description()->msid_signaling());
EXPECT_EQ(
cricket::kMsidSignalingMediaSection | cricket::kMsidSignalingSemantic,
jsep_desc.description()->msid_signaling());
}
const char kMediaSectionMsidLine[] = "a=msid:local_stream_1 audio_track_id_1";