From 3c529893e008e8fa1bebfe0565215a4232933efe Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Mon, 5 Dec 2022 19:59:10 +0100 Subject: [PATCH] sdp: ignore duplicate stream ids in msid parsing 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 Reviewed-by: Harald Alvestrand Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#38840} --- pc/webrtc_sdp.cc | 6 +- pc/webrtc_sdp_unittest.cc | 131 ++++++++++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 15 deletions(-) diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index a931500206..69fa62ca37 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -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; diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 772a97cd8f..b176c008ab 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -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