From 49e5587e64a53f611efea031c60b7546503e1f57 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 30 Mar 2023 13:51:39 +0200 Subject: [PATCH] Integrate RTP Header extension API with SDP munging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit in order to not regress existing use-cases while following rules described by the specification. This change now makes the existing regression test pass after the spec-compliant modifications. BUG=chromium:1051821 Change-Id: Ia384adf9a172ed88b5ec6a3cc5c478764a686cb9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299002 Reviewed-by: Harald Alvestrand Reviewed-by: Henrik Boström Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#39726} --- pc/media_session.cc | 4 ++- pc/media_session_unittest.cc | 11 +++--- ...er_connection_header_extension_unittest.cc | 36 +++++++++++++++++-- pc/sdp_offer_answer.cc | 36 ++++++++++++++++++- 4 files changed, 76 insertions(+), 11 deletions(-) diff --git a/pc/media_session.cc b/pc/media_session.cc index e703b44101..8aff4d93aa 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -685,7 +685,9 @@ static bool CreateContentOffer( // TODO(crbug.com/1051821): Configure the extension direction from // the information in the media_description_options extension // capability. - extensions.push_back(extension_with_id); + if (extension.direction != RtpTransceiverDirection::kStopped) { + extensions.push_back(extension_with_id); + } } } } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 8f38f6a1b4..6ba5032092 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -2018,7 +2018,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, } TEST_F(MediaSessionDescriptionFactoryTest, - AppendsStoppedExtensionIfKnownAndPresentInTheOffer) { + AllowsStoppedExtensionsToBeRemovedFromSubsequentOffer) { MediaSessionOptions opts; AddMediaDescriptionOptions(MEDIA_TYPE_VIDEO, "video", RtpTransceiverDirection::kSendRecv, kActive, @@ -2026,12 +2026,12 @@ TEST_F(MediaSessionDescriptionFactoryTest, opts.media_description_options.back().header_extensions = { webrtc::RtpHeaderExtensionCapability("uri1", 1, RtpTransceiverDirection::kSendRecv), - webrtc::RtpHeaderExtensionCapability("uri2", 1, + webrtc::RtpHeaderExtensionCapability("uri2", 2, RtpTransceiverDirection::kSendRecv)}; auto offer = f1_.CreateOffer(opts, nullptr); - // Now add "uri2" as stopped to the options verify that the offer contains - // uri2 since it's already present since before. + // Check that a subsequent offer after setting "uri2" to stopped no longer + // contains the extension. opts.media_description_options.back().header_extensions = { webrtc::RtpHeaderExtensionCapability("uri1", 1, RtpTransceiverDirection::kSendRecv), @@ -2043,8 +2043,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, ElementsAre(Property( &ContentInfo::media_description, Pointee(Property(&MediaContentDescription::rtp_header_extensions, - ElementsAre(Field(&RtpExtension::uri, "uri1"), - Field(&RtpExtension::uri, "uri2"))))))); + ElementsAre(Field(&RtpExtension::uri, "uri1"))))))); } TEST_F(MediaSessionDescriptionFactoryTest, diff --git a/pc/peer_connection_header_extension_unittest.cc b/pc/peer_connection_header_extension_unittest.cc index 42c58fd3bf..405b7e0b8c 100644 --- a/pc/peer_connection_header_extension_unittest.cc +++ b/pc/peer_connection_header_extension_unittest.cc @@ -321,11 +321,11 @@ TEST_P(PeerConnectionHeaderExtensionTest, } } -// This test is a regression test for behavior that the API +// These tests are regression tests for behavior that the API // enables in a proper way. It conflicts with the behavior // of the API to only offer non-stopped extensions. TEST_P(PeerConnectionHeaderExtensionTest, - SdpMungingWithoutApiUsageEnablesExtensions) { + SdpMungingAnswerWithoutApiUsageEnablesExtensions) { cricket::MediaType media_type; SdpSemantics semantics; std::tie(media_type, semantics) = GetParam(); @@ -371,7 +371,6 @@ TEST_P(PeerConnectionHeaderExtensionTest, ASSERT_TRUE(pc->SetLocalDescription(std::move(modified_answer))); auto session_description = pc->CreateOffer(); - ASSERT_TRUE(session_description->ToString(&sdp)); EXPECT_THAT(session_description->description() ->contents()[0] .media_description() @@ -382,6 +381,37 @@ TEST_P(PeerConnectionHeaderExtensionTest, Field(&RtpExtension::uri, "uri4"))); } +TEST_P(PeerConnectionHeaderExtensionTest, + SdpMungingOfferWithoutApiUsageEnablesExtensions) { + cricket::MediaType media_type; + SdpSemantics semantics; + std::tie(media_type, semantics) = GetParam(); + if (semantics != SdpSemantics::kUnifiedPlan) + return; + std::unique_ptr pc = + CreatePeerConnection(media_type, semantics); + pc->AddTransceiver(media_type); + + auto offer = + pc->CreateOffer(PeerConnectionInterface::RTCOfferAnswerOptions()); + std::string modified_sdp; + ASSERT_TRUE(offer->ToString(&modified_sdp)); + modified_sdp += "a=extmap:1 uri1\r\n"; + auto modified_offer = CreateSessionDescription(SdpType::kOffer, modified_sdp); + ASSERT_TRUE(pc->SetLocalDescription(std::move(modified_offer))); + + auto offer2 = + pc->CreateOffer(PeerConnectionInterface::RTCOfferAnswerOptions()); + EXPECT_THAT(offer2->description() + ->contents()[0] + .media_description() + ->rtp_header_extensions(), + ElementsAre(Field(&RtpExtension::uri, "uri2"), + Field(&RtpExtension::uri, "uri3"), + Field(&RtpExtension::uri, "uri4"), + Field(&RtpExtension::uri, "uri1"))); +} + TEST_P(PeerConnectionHeaderExtensionTest, EnablingExtensionsAfterRemoteOffer) { cricket::MediaType media_type; SdpSemantics semantics; diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 9c0e7eb860..c382d611ae 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -815,8 +815,37 @@ bool ContentHasHeaderExtension(const cricket::ContentInfo& content_info, } // namespace +void UpdateRtpHeaderExtensionPreferencesFromSdpMunging( + const cricket::SessionDescription* description, + TransceiverList* transceivers) { + // This integrates the RTP Header Extension Control API and local SDP munging + // for backward compability reasons. If something was enabled in the local + // description via SDP munging, consider it non-stopped in the API as well + // so that is shows up in subsequent offers/answers. + RTC_DCHECK(description); + RTC_DCHECK(transceivers); + for (const auto& content : description->contents()) { + auto transceiver = transceivers->FindByMid(content.name); + if (!transceiver) { + continue; + } + auto extension_capabilities = transceiver->GetHeaderExtensionsToNegotiate(); + // Set the capability of every extension we see here to "sendrecv". + for (auto& ext : content.media_description()->rtp_header_extensions()) { + auto it = absl::c_find_if(extension_capabilities, + [&ext](const RtpHeaderExtensionCapability c) { + return ext.uri == c.uri; + }); + if (it != extension_capabilities.end()) { + it->direction = RtpTransceiverDirection::kSendRecv; + } + } + transceiver->SetHeaderExtensionsToNegotiate(extension_capabilities); + } +} + // This class stores state related to a SetRemoteDescription operation, captures -// and reports potential errors that migth occur and makes sure to notify the +// and reports potential errors that might occur and makes sure to notify the // observer of the operation and the operations chain of completion. class SdpOfferAnswerHandler::RemoteDescriptionOperation { public: @@ -1816,6 +1845,11 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( local_ice_credentials_to_replace_->ClearIceCredentials(); } + if (IsUnifiedPlan()) { + UpdateRtpHeaderExtensionPreferencesFromSdpMunging( + local_description()->description(), transceivers()); + } + return RTCError::OK(); }