From: Michael Froman Date: Fri, 19 Sep 2025 14:44:43 -0500 Subject: Moz backout [M139 merge] Revert "Change SetLocalContent / SetRemoteContent to update header extensions" Upstream attempted to revert a commit in the release branch, but did a poor job. Commit 1ab982fa34 results in changes after reverting. We'll back this one out as a pre-stack commit and then cherry-pick the real trunk version of the revert in d27b4ef208. --- media/base/codec.h | 3 ++ pc/channel.cc | 13 ++++-- pc/channel_unittest.cc | 24 +++++++----- pc/congestion_control_integrationtest.cc | 50 +++++++++++++++++++++++- pc/peer_connection_integrationtest.cc | 14 +++++-- test/peer_scenario/tests/l4s_test.cc | 2 +- 6 files changed, 87 insertions(+), 19 deletions(-) diff --git a/media/base/codec.h b/media/base/codec.h index 694589dd14..e26306df20 100644 --- a/media/base/codec.h +++ b/media/base/codec.h @@ -192,6 +192,9 @@ struct RTC_EXPORT Codec { sink.Append("video/"); } absl::Format(&sink, "%s/%d/%d", c.name, c.clockrate, c.channels); + if (c.packetization) { + absl::Format(&sink, ",packetization=%s", *c.packetization); + } for (auto param : c.params) { sink.Append(";"); sink.Append(param.first); diff --git a/pc/channel.cc b/pc/channel.cc index bccd1e2480..2b548064a4 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -911,7 +911,6 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, } } } - last_recv_params_ = recv_params; if (!UpdateLocalStreams_w(content->streams(), type, error_desc)) { @@ -922,7 +921,7 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, set_local_content_direction(content->direction()); UpdateMediaSendRecvState_w(); - // Disabled because suggeting PTs takes thread jumps. + // Disabled because suggesting PTs takes thread jumps. // TODO: https://issues.webrtc.org/360058654 - reenable after cleanup // RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(0); @@ -1038,7 +1037,6 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, media_send_channel()->SetExtmapAllowMixed(content->extmap_allow_mixed()); VideoReceiverParameters recv_params = last_recv_params_; - MediaChannelParametersFromMediaDescription( content, header_extensions, RtpTransceiverDirectionHasRecv(content->direction()), &recv_params); @@ -1113,7 +1111,12 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, last_recv_params_ = recv_params; + // Also update send parameters if header extensions are changed. + needs_send_params_update |= + (last_send_params_.extensions != header_extensions && + !send_params.codecs.empty()); if (needs_send_params_update) { + send_params.extensions = header_extensions; if (!media_send_channel()->SetSenderParameters(send_params)) { error_desc = StringFormat( "Failed to set send parameters for m-section with mid='%s'.", @@ -1224,7 +1227,11 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, media_send_channel()->SendCodecRtxTime()); last_send_params_ = send_params; + needs_recv_params_update |= + (recv_params.extensions != send_params.extensions && + !recv_params.codecs.empty()); if (needs_recv_params_update) { + recv_params.extensions = send_params.extensions; if (!media_receive_channel()->SetReceiverParameters(recv_params)) { error_desc = StringFormat( "Failed to set recv parameters for m-section with mid='%s'.", diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index e3488a98a9..4bb9573632 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -2385,18 +2385,22 @@ TEST_F(VideoChannelSingleThreadTest, EXPECT_THAT( media_receive_channel1_impl()->recv_codecs(), - ElementsAre(AllOf(Field(&webrtc::Codec::id, 96), - Field(&webrtc::Codec::packetization, "foo")), - AllOf(Field(&webrtc::Codec::id, 98), - Field(&webrtc::Codec::packetization, std::nullopt)))); + ElementsAre( + AllOf(Field("id", &webrtc::Codec::id, 96), + Field("packetization", &webrtc::Codec::packetization, "foo")), + AllOf(Field("id", &webrtc::Codec::id, 98), + Field("packetization", &webrtc::Codec::packetization, + std::nullopt)))); EXPECT_THAT( media_send_channel1_impl()->send_codecs(), - ElementsAre(AllOf(Field(&webrtc::Codec::id, 96), - Field(&webrtc::Codec::packetization, "foo")), - AllOf(Field(&webrtc::Codec::id, 97), - Field(&webrtc::Codec::packetization, "bar")), - AllOf(Field(&webrtc::Codec::id, 99), - Field(&webrtc::Codec::packetization, std::nullopt)))); + ElementsAre( + AllOf(Field("id", &webrtc::Codec::id, 96), + Field("packetization", &webrtc::Codec::packetization, "foo")), + AllOf(Field("id", &webrtc::Codec::id, 97), + Field("packetization", &webrtc::Codec::packetization, "bar")), + AllOf(Field("id", &webrtc::Codec::id, 99), + Field("packetization", &webrtc::Codec::packetization, + std::nullopt)))); } TEST_F(VideoChannelSingleThreadTest, diff --git a/pc/congestion_control_integrationtest.cc b/pc/congestion_control_integrationtest.cc index a0bb6a6997..da5612d99e 100644 --- a/pc/congestion_control_integrationtest.cc +++ b/pc/congestion_control_integrationtest.cc @@ -84,6 +84,53 @@ TEST_F(PeerConnectionCongestionControlTest, ReceiveOfferSetsCcfbFlag) { EXPECT_THAT(answer_str, Not(HasSubstr("transport-cc"))); } +TEST_F(PeerConnectionCongestionControlTest, NegotiatingCcfbRemovesTsn) { + SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignalingForSdpOnly(); + callee()->AddVideoTrack(); + // Add transceivers to caller in order to accomodate reception + caller()->pc()->AddTransceiver(MediaType::VIDEO); + auto parameters = caller()->pc()->GetSenders()[0]->GetParameters(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_THAT(WaitUntil([&] { return SignalingStateStable(); }, IsTrue()), + IsRtcOk()); + + std::vector negotiated_header_extensions = + caller()->pc()->GetTransceivers()[0]->GetNegotiatedHeaderExtensions(); + EXPECT_THAT( + negotiated_header_extensions, + Not(Contains( + AllOf(Field("uri", &RtpHeaderExtensionCapability::uri, + RtpExtension::kTransportSequenceNumberUri), + Not(Field("direction", &RtpHeaderExtensionCapability::direction, + RtpTransceiverDirection::kStopped)))))) + << " in caller negotiated header extensions"; + + parameters = caller()->pc()->GetSenders()[0]->GetParameters(); + EXPECT_THAT(parameters.header_extensions, + Not(Contains(Field("uri", &RtpExtension::uri, + RtpExtension::kTransportSequenceNumberUri)))) + << " in caller sender parameters"; + parameters = caller()->pc()->GetReceivers()[0]->GetParameters(); + EXPECT_THAT(parameters.header_extensions, + Not(Contains(Field("uri", &RtpExtension::uri, + RtpExtension::kTransportSequenceNumberUri)))) + << " in caller receiver parameters"; + + parameters = callee()->pc()->GetSenders()[0]->GetParameters(); + EXPECT_THAT(parameters.header_extensions, + Not(Contains(Field("uri", &RtpExtension::uri, + RtpExtension::kTransportSequenceNumberUri)))) + << " in callee sender parameters"; + + parameters = callee()->pc()->GetReceivers()[0]->GetParameters(); + EXPECT_THAT(parameters.header_extensions, + Not(Contains(Field("uri", &RtpExtension::uri, + RtpExtension::kTransportSequenceNumberUri)))) + << " in callee receiver parameters"; +} + TEST_F(PeerConnectionCongestionControlTest, CcfbGetsUsed) { SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); ASSERT_TRUE(CreatePeerConnectionWrappers()); @@ -133,8 +180,7 @@ TEST_F(PeerConnectionCongestionControlTest, TransportCcGetsUsed) { EXPECT_THAT(pc_internal->FeedbackAccordingToRfc8888CountForTesting(), Eq(0)); } -TEST_F(PeerConnectionCongestionControlTest, - DISABLED_CcfbGetsUsedCalleeToCaller) { +TEST_F(PeerConnectionCongestionControlTest, CcfbGetsUsedCalleeToCaller) { SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignaling(); diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 3a2babda3e..4d4ec552d7 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -4631,8 +4631,9 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, PeerConnectionInterface::kStable); } +// TODO: issues.webrtc.org/425336456 - figure out correct behavior and reenable TEST_F(PeerConnectionIntegrationTestUnifiedPlan, - OnlyOnePairWantsCorruptionScorePlumbing) { + DISABLED_OnlyOnePairWantsCorruptionScorePlumbing) { // In order for corruption score to be logged, encryption of RTP header // extensions must be allowed. CryptoOptions crypto_options; @@ -4656,12 +4657,19 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, ASSERT_THAT( WaitUntil([&] { return SignalingStateStable(); }, ::testing::IsTrue()), IsRtcOk()); + std::vector negotiated_extensions = + caller()->pc()->GetTransceivers()[0]->GetNegotiatedHeaderExtensions(); + ASSERT_THAT(negotiated_extensions, + Contains(Field("uri", &RtpHeaderExtensionCapability::uri, + RtpExtension::kCorruptionDetectionUri))); ASSERT_THAT(WaitUntil([&] { return caller()->GetCorruptionScoreCount(); }, ::testing::Gt(0), {.timeout = kMaxWaitForStats}), - IsRtcOk()); + IsRtcOk()) + << "Waiting for caller corruption score count > 0"; ASSERT_THAT(WaitUntil([&] { return callee()->GetCorruptionScoreCount(); }, ::testing::Eq(0), {.timeout = kMaxWaitForStats}), - IsRtcOk()); + IsRtcOk()) + << "Waiting for callee corruption score count = 0"; for (const auto& pair : {caller(), callee()}) { scoped_refptr report = pair->NewGetStats(); diff --git a/test/peer_scenario/tests/l4s_test.cc b/test/peer_scenario/tests/l4s_test.cc index f65a24fff9..378ea2e746 100644 --- a/test/peer_scenario/tests/l4s_test.cc +++ b/test/peer_scenario/tests/l4s_test.cc @@ -120,7 +120,7 @@ DataRate GetAvailableSendBitrate( return DataRate::BitsPerSec(*stats[0]->available_outgoing_bitrate); } -TEST(L4STest, DISABLED_NegotiateAndUseCcfbIfEnabled) { +TEST(L4STest, NegotiateAndUseCcfbIfEnabled) { PeerScenario s(*test_info_); PeerScenarioClient::Config config;