From d4fe3ce902fab51b536d285665c58c9f57703c00 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 31 Mar 2023 13:33:42 +0200 Subject: [PATCH] Only answer with non-stopped RTP header extensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This extends the RTP header extension API usage to generating answers. Also re-adds unit tests removed by the revert. BUG=chromium:1051821 Change-Id: Ib754284e9a77cb49e22bea7072c475d240f2563b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298740 Reviewed-by: Henrik Boström Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#39800} --- pc/media_session.cc | 22 ++++- pc/media_session.h | 4 +- ...er_connection_header_extension_unittest.cc | 94 +++++++++++++++++++ 3 files changed, 115 insertions(+), 5 deletions(-) diff --git a/pc/media_session.cc b/pc/media_session.cc index 8aff4d93aa..23a2ad11cb 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1411,8 +1411,24 @@ static bool CreateMediaContentAnswer( enable_encrypted_rtp_header_extensions ? webrtc::RtpExtension::Filter::kPreferEncryptedExtension : webrtc::RtpExtension::Filter::kDiscardEncryptedExtension; + + // Filter local extensions by capabilities and direction. + RtpHeaderExtensions local_rtp_extensions_to_reply_with; + for (auto extension_with_id : local_rtp_extensions) { + for (const auto& extension : media_description_options.header_extensions) { + if (extension_with_id.uri == extension.uri) { + // TODO(crbug.com/1051821): Configure the extension direction from + // the information in the media_description_options extension + // capability. For now, do not include stopped extensions. + // See also crbug.com/webrtc/7477 about the general lack of direction. + if (extension.direction != RtpTransceiverDirection::kStopped) { + local_rtp_extensions_to_reply_with.push_back(extension_with_id); + } + } + } + } RtpHeaderExtensions negotiated_rtp_extensions; - NegotiateRtpHeaderExtensions(local_rtp_extensions, + NegotiateRtpHeaderExtensions(local_rtp_extensions_to_reply_with, offer->rtp_header_extensions(), extensions_filter, &negotiated_rtp_extensions); answer->set_rtp_header_extensions(negotiated_rtp_extensions); @@ -2591,7 +2607,7 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( const SessionDescription* current_description, const TransportInfo* bundle_transport, const AudioCodecs& audio_codecs, - const RtpHeaderExtensions& default_audio_rtp_header_extensions, + const RtpHeaderExtensions& rtp_header_extensions, StreamParamsVec* current_streams, SessionDescription* answer, IceCredentialsIterator* ice_credentials) const { @@ -2679,7 +2695,7 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( if (!CreateMediaContentAnswer( offer_audio_description, media_description_options, session_options, sdes_policy, GetCryptos(current_content), - filtered_rtp_header_extensions(default_audio_rtp_header_extensions), + filtered_rtp_header_extensions(rtp_header_extensions), ssrc_generator(), enable_encrypted_rtp_header_extensions_, current_streams, bundle_enabled, audio_answer.get())) { return false; // Fails the session setup. diff --git a/pc/media_session.h b/pc/media_session.h index 6548f9c436..87f04c779f 100644 --- a/pc/media_session.h +++ b/pc/media_session.h @@ -285,7 +285,7 @@ class MediaSessionDescriptionFactory { const SessionDescription* current_description, const TransportInfo* bundle_transport, const AudioCodecs& audio_codecs, - const RtpHeaderExtensions& default_audio_rtp_header_extensions, + const RtpHeaderExtensions& rtp_header_extensions, StreamParamsVec* current_streams, SessionDescription* answer, IceCredentialsIterator* ice_credentials) const; @@ -299,7 +299,7 @@ class MediaSessionDescriptionFactory { const SessionDescription* current_description, const TransportInfo* bundle_transport, const VideoCodecs& video_codecs, - const RtpHeaderExtensions& default_video_rtp_header_extensions, + const RtpHeaderExtensions& rtp_header_extensions, StreamParamsVec* current_streams, SessionDescription* answer, IceCredentialsIterator* ice_credentials) const; diff --git a/pc/peer_connection_header_extension_unittest.cc b/pc/peer_connection_header_extension_unittest.cc index 405b7e0b8c..b1c6c3cfb5 100644 --- a/pc/peer_connection_header_extension_unittest.cc +++ b/pc/peer_connection_header_extension_unittest.cc @@ -199,6 +199,39 @@ TEST_P(PeerConnectionHeaderExtensionTest, OffersUnstoppedModifiedExtensions) { Field(&RtpExtension::uri, "uri3"))); } +TEST_P(PeerConnectionHeaderExtensionTest, AnswersUnstoppedModifiedExtensions) { + cricket::MediaType media_type; + SdpSemantics semantics; + std::tie(media_type, semantics) = GetParam(); + if (semantics != SdpSemantics::kUnifiedPlan) + return; + std::unique_ptr pc1 = + CreatePeerConnection(media_type, semantics); + std::unique_ptr pc2 = + CreatePeerConnection(media_type, semantics); + auto transceiver1 = pc1->AddTransceiver(media_type); + + auto offer = pc1->CreateOfferAndSetAsLocal( + PeerConnectionInterface::RTCOfferAnswerOptions()); + pc2->SetRemoteDescription(std::move(offer)); + + ASSERT_EQ(pc2->pc()->GetTransceivers().size(), 1u); + auto transceiver2 = pc2->pc()->GetTransceivers()[0]; + auto modified_extensions = transceiver2->GetHeaderExtensionsToNegotiate(); + // Don't offer uri4. + modified_extensions[3].direction = RtpTransceiverDirection::kStopped; + transceiver2->SetHeaderExtensionsToNegotiate(modified_extensions); + + auto answer = pc2->CreateAnswerAndSetAsLocal( + PeerConnectionInterface::RTCOfferAnswerOptions()); + EXPECT_THAT(answer->description() + ->contents()[0] + .media_description() + ->rtp_header_extensions(), + ElementsAre(Field(&RtpExtension::uri, "uri2"), + Field(&RtpExtension::uri, "uri3"))); +} + TEST_P(PeerConnectionHeaderExtensionTest, NegotiatedExtensionsAreAccessible) { cricket::MediaType media_type; SdpSemantics semantics; @@ -235,6 +268,67 @@ TEST_P(PeerConnectionHeaderExtensionTest, NegotiatedExtensionsAreAccessible) { RtpTransceiverDirection::kStopped))); } +TEST_P(PeerConnectionHeaderExtensionTest, OfferedExtensionsArePerTransceiver) { + cricket::MediaType media_type; + SdpSemantics semantics; + std::tie(media_type, semantics) = GetParam(); + if (semantics != SdpSemantics::kUnifiedPlan) + return; + std::unique_ptr pc1 = + CreatePeerConnection(media_type, semantics); + auto transceiver1 = pc1->AddTransceiver(media_type); + auto modified_extensions = transceiver1->GetHeaderExtensionsToNegotiate(); + modified_extensions[3].direction = RtpTransceiverDirection::kStopped; + transceiver1->SetHeaderExtensionsToNegotiate(modified_extensions); + auto transceiver2 = pc1->AddTransceiver(media_type); + + auto session_description = pc1->CreateOffer(); + EXPECT_THAT(session_description->description() + ->contents()[0] + .media_description() + ->rtp_header_extensions(), + ElementsAre(Field(&RtpExtension::uri, "uri2"), + Field(&RtpExtension::uri, "uri3"))); + EXPECT_THAT(session_description->description() + ->contents()[1] + .media_description() + ->rtp_header_extensions(), + ElementsAre(Field(&RtpExtension::uri, "uri2"), + Field(&RtpExtension::uri, "uri3"), + Field(&RtpExtension::uri, "uri4"))); +} + +TEST_P(PeerConnectionHeaderExtensionTest, RemovalAfterRenegotiation) { + cricket::MediaType media_type; + SdpSemantics semantics; + std::tie(media_type, semantics) = GetParam(); + if (semantics != SdpSemantics::kUnifiedPlan) + return; + std::unique_ptr pc1 = + CreatePeerConnection(media_type, semantics); + std::unique_ptr pc2 = + CreatePeerConnection(media_type, semantics); + auto transceiver1 = pc1->AddTransceiver(media_type); + + auto offer = pc1->CreateOfferAndSetAsLocal( + PeerConnectionInterface::RTCOfferAnswerOptions()); + pc2->SetRemoteDescription(std::move(offer)); + auto answer = pc2->CreateAnswerAndSetAsLocal( + PeerConnectionInterface::RTCOfferAnswerOptions()); + pc1->SetRemoteDescription(std::move(answer)); + + auto modified_extensions = transceiver1->GetHeaderExtensionsToNegotiate(); + modified_extensions[3].direction = RtpTransceiverDirection::kStopped; + transceiver1->SetHeaderExtensionsToNegotiate(modified_extensions); + auto session_description = pc1->CreateOffer(); + EXPECT_THAT(session_description->description() + ->contents()[0] + .media_description() + ->rtp_header_extensions(), + ElementsAre(Field(&RtpExtension::uri, "uri2"), + Field(&RtpExtension::uri, "uri3"))); +} + TEST_P(PeerConnectionHeaderExtensionTest, StoppedByDefaultExtensionCanBeActivatedByRemoteSdp) { cricket::MediaType media_type;