From be03c0971863c9e6807fcbdb4175754e8242a652 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Wed, 1 Feb 2023 10:10:06 +0100 Subject: [PATCH] Only serialize non-stopped RTP header extensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit as described in https://w3c.github.io/webrtc-extensions/#modifications-to-existing-procedures-0 "For each RTP header extension "e" listed in [[HeaderExtensionsToOffer]] where direction is not "stopped", an "a=extmap" line, as specified in [RFC5285], section 5 This avoids including them in case they are stopped on one transceiver but not the other. Also, this allows extensions to be removed from a subsequent offer. See also https://github.com/w3c/webrtc-extensions/issues/140 BUG=chromium:1051821 Change-Id: I4d7462f939ce4cd5d8c2331bc038200fe18f70e2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/291703 Commit-Queue: Philipp Hancke Reviewed-by: Harald Alvestrand Reviewed-by: Markus Handell Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#39242} --- pc/media_session.cc | 7 ++- pc/media_session_unittest.cc | 5 +- ...er_connection_header_extension_unittest.cc | 62 +++++++++++++++++++ 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/pc/media_session.cc b/pc/media_session.cc index e703b44101..92fe80f3d1 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -684,8 +684,11 @@ static bool CreateContentOffer( 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. - extensions.push_back(extension_with_id); + // 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) { + extensions.push_back(extension_with_id); + } } } } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 8f38f6a1b4..6dc6d76021 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, @@ -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 1a452b0a1f..9bbc32b7e0 100644 --- a/pc/peer_connection_header_extension_unittest.cc +++ b/pc/peer_connection_header_extension_unittest.cc @@ -229,6 +229,68 @@ TEST_P(PeerConnectionHeaderExtensionTest, NegotiatedExtensionsAreAccessible) { Field(&RtpHeaderExtensionCapability::uri, "uri3"))); } +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->HeaderExtensionsToOffer(); + modified_extensions[3].direction = RtpTransceiverDirection::kStopped; + transceiver1->SetOfferedRtpHeaderExtensions(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->HeaderExtensionsToOffer(); + modified_extensions[3].direction = RtpTransceiverDirection::kStopped; + modified_extensions[3].direction = RtpTransceiverDirection::kStopped; + transceiver1->SetOfferedRtpHeaderExtensions(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"))); +} + INSTANTIATE_TEST_SUITE_P( , PeerConnectionHeaderExtensionTest,