From 3488726163bba9811dd441ed0dbf689e66739c95 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 1 Jun 2023 19:07:50 +0200 Subject: [PATCH] sdp: reject spec simulcast answers without the rid extension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit which is mandatory to implement per https://datatracker.ietf.org/doc/html/rfc8853#section-5.5 BUG=chromium:1422258 Change-Id: I3639b15453aaa074fbe9f26b722f5997b439224a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/306661 Reviewed-by: Henrik Boström Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#40208} --- api/rtp_transceiver_interface.h | 1 - pc/peer_connection_simulcast_unittest.cc | 9 ++--- pc/sdp_offer_answer.cc | 30 ++++++++++++++ pc/sdp_offer_answer_unittest.cc | 51 ++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 6 deletions(-) diff --git a/api/rtp_transceiver_interface.h b/api/rtp_transceiver_interface.h index 1531c7ff44..7d0d1a18bf 100644 --- a/api/rtp_transceiver_interface.h +++ b/api/rtp_transceiver_interface.h @@ -41,7 +41,6 @@ struct RTC_EXPORT RtpTransceiverInit final { // The added RtpTransceiver will be added to these streams. std::vector stream_ids; - // TODO(bugs.webrtc.org/7600): Not implemented. std::vector send_encodings; }; diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 11a0fb121f..1adb8a52e0 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -538,9 +538,9 @@ TEST_F(PeerConnectionSimulcastTests, ValidateTransceiverParameters(transceiver, expected_layers); } -// Tests that simulcast is disabled if the RID extension is not negotiated -// regardless of if the RIDs and simulcast attribute were negotiated properly. -TEST_F(PeerConnectionSimulcastTests, NegotiationDoesNotHaveRidExtension) { +// Tests that a simulcast answer is rejected if the RID extension is not +// negotiated. +TEST_F(PeerConnectionSimulcastTests, NegotiationDoesNotHaveRidExtensionFails) { auto local = CreatePeerConnectionWrapper(); auto remote = CreatePeerConnectionWrapper(); auto layers = CreateLayers({"1", "2", "3"}, true); @@ -570,8 +570,7 @@ TEST_F(PeerConnectionSimulcastTests, NegotiationDoesNotHaveRidExtension) { .receive_layers() .GetAllLayers() .size()); - EXPECT_TRUE(local->SetRemoteDescription(std::move(answer), &err)) << err; - ValidateTransceiverParameters(transceiver, expected_layers); + EXPECT_FALSE(local->SetRemoteDescription(std::move(answer), &err)) << err; } TEST_F(PeerConnectionSimulcastTests, SimulcastAudioRejected) { diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index cf22cf3934..17befaf445 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -538,6 +538,30 @@ RTCError ValidateBundledRtpHeaderExtensions( return RTCError::OK(); } +RTCError ValidateRtpHeaderExtensionsForSpecSimulcast( + const cricket::SessionDescription& description) { + for (const ContentInfo& content : description.contents()) { + if (content.type != MediaProtocolType::kRtp) { + continue; + } + const auto media_description = content.media_description(); + if (!media_description->HasSimulcast()) { + continue; + } + auto extensions = media_description->rtp_header_extensions(); + auto it = absl::c_find_if(extensions, [](const RtpExtension& ext) { + return ext.uri == RtpExtension::kRidUri; + }); + if (it == extensions.end()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "The media section with MID='" + content.mid() + + "' negotiates simulcast but does not negotiate " + "the RID RTP header extension."); + } + } + return RTCError::OK(); +} + bool IsValidOfferToReceiveMedia(int value) { typedef PeerConnectionInterface::RTCOfferAnswerOptions Options; return (value >= Options::kUndefined) && @@ -3602,6 +3626,12 @@ RTCError SdpOfferAnswerHandler::ValidateSessionDescription( "which is not supported with Unified Plan."); } } + // Validate spec-simulcast which only works if the remote end negotiated the + // mid and rid header extension. + error = ValidateRtpHeaderExtensionsForSpecSimulcast(*sdesc->description()); + if (!error.ok()) { + return error; + } } return RTCError::OK(); diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 79f945776f..e9ec17bf08 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -548,4 +548,55 @@ TEST_F(SdpOfferAnswerTest, RejectedDataChannelsDoGetReofferedWhenActive) { #endif // WEBRTC_HAVE_SCTP +TEST_F(SdpOfferAnswerTest, SimulcastAnswerWithNoRidsIsRejected) { + auto pc = CreatePeerConnection(); + + RtpTransceiverInit init; + RtpEncodingParameters rid1; + rid1.rid = "1"; + init.send_encodings.push_back(rid1); + RtpEncodingParameters rid2; + rid2.rid = "2"; + init.send_encodings.push_back(rid2); + + auto transceiver = pc->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init); + EXPECT_TRUE(pc->CreateOfferAndSetAsLocal()); + auto mid = pc->pc()->local_description()->description()->contents()[0].mid(); + + // A SDP answer with simulcast but without mid/rid extensions. + std::string sdp = + "v=0\r\n" + "o=- 4131505339648218884 3 IN IP4 **-----**\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=ice-ufrag:zGWFZ+fVXDeN6UoI/136\r\n" + "a=ice-pwd:9AUNgUqRNI5LSIrC1qFD2iTR\r\n" + "a=fingerprint:sha-256 " + "AD:52:52:E0:B1:37:34:21:0E:15:8E:B7:56:56:7B:B4:39:0E:6D:1C:F5:84:A7:EE:" + "B5:27:3E:30:B1:7D:69:42\r\n" + "a=setup:passive\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 96\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp:9 IN IP4 0.0.0.0\r\n" + "a=mid:" + + mid + + "\r\n" + "a=recvonly\r\n" + "a=rtcp-mux\r\n" + "a=rtcp-rsize\r\n" + "a=rtpmap:96 VP8/90000\r\n" + "a=rid:1 recv\r\n" + "a=rid:2 recv\r\n" + "a=simulcast:recv 1;2\r\n"; + std::string extensions = + "a=extmap:9 urn:ietf:params:rtp-hdrext:sdes:mid\r\n" + "a=extmap:10 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id\r\n"; + auto answer = CreateSessionDescription(SdpType::kAnswer, sdp); + EXPECT_FALSE(pc->SetRemoteDescription(std::move(answer))); + + auto answer_with_extensions = + CreateSessionDescription(SdpType::kAnswer, sdp + extensions); + EXPECT_TRUE(pc->SetRemoteDescription(std::move(answer_with_extensions))); +} + } // namespace webrtc