admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

sdp: ignore duplicate stream ids in msid parsing

Browse Source 
https://www.rfc-editor.org/rfc/rfc8830.html#section-3.2.2
says
  Check if a MediaStream with the same WebIDL "id"
  attribute already exists. If not, create it.

Ignoring duplicates here satisfies this and brings the behavior
closer to Firefox:
  https://github.com/w3c/webrtc-pc/issues/2803

Also make tests use a std::string for the sdp input string.

BUG=webrtc:14745

Change-Id: Iccaabc08d865b779416f6ba4d2dfd5cff04133f0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/286422
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38840}
This commit is contained in:
Philipp Hancke 2022-12-05 19:59:10 +01:00 committed by WebRTC LUCI CQ
parent 352f38c7a8
commit 3c529893e0
2 changed files with 122 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
pc/webrtc_sdp.cc
View File

@ -2379,7 +2379,11 @@ static bool ParseMsidAttribute(absl::string_view line,
return ParseFailed(line, "Missing stream ID in msid attribute.", error);
}
// The special value "-" indicates "no MediaStream".
if (new_stream_id.compare(kNoStreamMsid) != 0) {
if (new_stream_id.compare(kNoStreamMsid) != 0 &&
!absl::c_any_of(*stream_ids,
[&new_stream_id](const std::string& existing_stream_id) {
return new_stream_id == existing_stream_id;
})) {
stream_ids->push_back(new_stream_id);
}
return true;

131
pc/webrtc_sdp_unittest.cc
View File

@ -4055,11 +4055,54 @@ TEST_F(WebRtcSdpTest, DeserializeInvalidPortInCandidateAttribute) {
EXPECT_FALSE(SdpDeserialize(kSdpWithInvalidCandidatePort, &jdesc_output));
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithStreamIdAndTrackId) {
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
"t=0 0\r\n"
"m=audio 9 RTP/SAVPF 111\r\n"
"c=IN IP4 0.0.0.0\r\n"
"a=rtpmap:111 opus/48000/2\r\n"
"a=msid:stream_id track_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 1u);
EXPECT_EQ(stream.stream_ids()[0], "stream_id");
EXPECT_EQ(stream.id, "track_id");
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithEmptyStreamIdAndTrackId) {
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
"t=0 0\r\n"
"m=audio 9 RTP/SAVPF 111\r\n"
"c=IN IP4 0.0.0.0\r\n"
"a=rtpmap:111 opus/48000/2\r\n"
"a=msid:- track_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 0u);
EXPECT_EQ(stream.id, "track_id");
}
// Test that "a=msid" with a missing track ID is rejected and doesn't crash.
// Regression test for:
// https://bugs.chromium.org/p/chromium/issues/detail?id=686405
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithMissingTrackId) {
static const char kSdpWithMissingTrackId[] =
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
@ -4070,11 +4113,11 @@ TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithMissingTrackId) {
"a=msid:stream_id \r\n";
JsepSessionDescription jdesc_output(kDummyType);
EXPECT_FALSE(SdpDeserialize(kSdpWithMissingTrackId, &jdesc_output));
EXPECT_FALSE(SdpDeserialize(sdp, &jdesc_output));
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppData) {
static const char kSdpWithMissingStreamId[] =
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
@ -4085,11 +4128,19 @@ TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppData) {
"a=msid:stream_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
EXPECT_TRUE(SdpDeserialize(kSdpWithMissingStreamId, &jdesc_output));
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 1u);
EXPECT_EQ(stream.stream_ids()[0], "stream_id");
// Track id is randomly generated.
EXPECT_NE(stream.id, "");
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataTwoStreams) {
static const char kSdpWithMissingStreamId[] =
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
@ -4101,11 +4152,20 @@ TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataTwoStreams) {
"a=msid:stream_id2\r\n";
JsepSessionDescription jdesc_output(kDummyType);
EXPECT_TRUE(SdpDeserialize(kSdpWithMissingStreamId, &jdesc_output));
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 2u);
EXPECT_EQ(stream.stream_ids()[0], "stream_id");
EXPECT_EQ(stream.stream_ids()[1], "stream_id2");
// Track id is randomly generated.
EXPECT_NE(stream.id, "");
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataDuplicate) {
static const char kSdpWithMissingStreamId[] =
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
@ -4117,12 +4177,20 @@ TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataDuplicate) {
"a=msid:stream_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
// This is somewhat silly but accept it.
EXPECT_TRUE(SdpDeserialize(kSdpWithMissingStreamId, &jdesc_output));
// This is somewhat silly but accept it. Duplicates get filtered.
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 1u);
EXPECT_EQ(stream.stream_ids()[0], "stream_id");
// Track id is randomly generated.
EXPECT_NE(stream.id, "");
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataMixed) {
static const char kSdpWithMissingStreamId[] =
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
@ -4131,16 +4199,51 @@ TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataMixed) {
"c=IN IP4 0.0.0.0\r\n"
"a=rtpmap:111 opus/48000/2\r\n"
"a=msid:stream_id\r\n"
"a=msid:stream_id track_id\r\n";
"a=msid:stream_id2 track_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
// Mixing the syntax like this is not a good idea but we accept it
// and the result is the second track_id.
EXPECT_TRUE(SdpDeserialize(kSdpWithMissingStreamId, &jdesc_output));
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 2u);
EXPECT_EQ(stream.stream_ids()[0], "stream_id");
EXPECT_EQ(stream.stream_ids()[1], "stream_id2");
// Track id is taken from second line.
EXPECT_EQ(stream.id, "track_id");
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithoutAppDataMixedNoStream) {
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
"t=0 0\r\n"
"m=audio 9 RTP/SAVPF 111\r\n"
"c=IN IP4 0.0.0.0\r\n"
"a=rtpmap:111 opus/48000/2\r\n"
"a=msid:stream_id\r\n"
"a=msid:- track_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
// This is somewhat undefined behavior but accept it and expect a single
// stream.
EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
auto stream = jdesc_output.description()
->contents()[0]
.media_description()
->streams()[0];
ASSERT_EQ(stream.stream_ids().size(), 1u);
EXPECT_EQ(stream.stream_ids()[0], "stream_id");
EXPECT_EQ(stream.id, "track_id");
}
TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithMissingStreamId) {
static const char kSdpWithMissingStreamId[] =
std::string sdp =
"v=0\r\n"
"o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
@ -4151,7 +4254,7 @@ TEST_F(WebRtcSdpTest, DeserializeMsidAttributeWithMissingStreamId) {
"a=msid: track_id\r\n";
JsepSessionDescription jdesc_output(kDummyType);
EXPECT_FALSE(SdpDeserialize(kSdpWithMissingStreamId, &jdesc_output));
EXPECT_FALSE(SdpDeserialize(sdp, &jdesc_output));
}
// Tests that if both session-level address and media-level address exist, use