Emit CCFB parameters per codec, not wildcard This works around the inability of some parsers to handle wildcards. Bug: webrtc:489794442 Change-Id: I4633e1d27139e13ec571bbd169ed38c324922248 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/455280 Reviewed-by: Per Kjellander <perkj@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47096}
diff --git a/api/webrtc_sdp.cc b/api/webrtc_sdp.cc index f404b90..aab9251 100644 --- a/api/webrtc_sdp.cc +++ b/api/webrtc_sdp.cc
@@ -1117,12 +1117,21 @@ void BuildRtpmap(const MediaContentDescription* media_desc, const MediaType media_type, + bool use_wildcard, std::string* message) { RTC_DCHECK(message != nullptr); RTC_DCHECK(media_desc != nullptr); StringBuilder os; if (media_type == MediaType::VIDEO) { - for (const Codec& codec : media_desc->codecs()) { + for (Codec codec : media_desc->codecs()) { + // Include transport-wide codec parameters. + // TODO: issues.webrtc.org/489794442 - Remove when wildcards are + // OK to send. + if (!use_wildcard) { + if (media_desc->rtcp_fb_ack_ccfb()) { + codec.feedback_params.Add({"ack", "ccfb"}); + } + } // RFC 4566 // a=rtpmap:<payload type> <encoding name>/<clock rate> // [/<encodingparameters>] @@ -1140,8 +1149,16 @@ std::vector<int> ptimes; std::vector<int> maxptimes; int max_minptime = 0; - for (const Codec& codec : media_desc->codecs()) { + for (Codec codec : media_desc->codecs()) { RTC_DCHECK(!codec.name.empty()); + // Include transport-wide codec parameters. + // TODO: issues.webrtc.org/489794442 - Remove when wildcards are + // OK to send. + if (!use_wildcard) { + if (media_desc->rtcp_fb_ack_ccfb()) { + codec.feedback_params.Add({"ack", "ccfb"}); + } + } // RFC 4566 // a=rtpmap:<payload type> <encoding name>/<clock rate> // [/<encodingparameters>] @@ -1183,18 +1200,21 @@ AddAttributeLine(kCodecParamPTime, ptime, message); } } - if (media_desc->rtcp_fb_ack_ccfb()) { - // RFC 8888 section 6 - InitAttrLine(kAttributeRtcpFb, &os); - os << kSdpDelimiterColon; - os << "* ack ccfb"; - AddLine(os.str(), message); + if (use_wildcard) { + if (media_desc->rtcp_fb_ack_ccfb()) { + // RFC 8888 section 6 + InitAttrLine(kAttributeRtcpFb, &os); + os << kSdpDelimiterColon; + os << "* ack ccfb"; + AddLine(os.str(), message); + } } } void BuildRtpContentAttributes(const MediaContentDescription* media_desc, const MediaType media_type, int msid_signaling, + bool use_wildcard, std::string* message) { SimulcastSdpSerializer serializer; StringBuilder os; @@ -1289,7 +1309,7 @@ // RFC 4566 // a=rtpmap:<payload type> <encoding name>/<clock rate> // [/<encodingparameters>] - BuildRtpmap(media_desc, media_type, message); + BuildRtpmap(media_desc, media_type, use_wildcard, message); for (const StreamParams& track : media_desc->streams()) { // Build the ssrc-group lines. @@ -1449,7 +1469,9 @@ if (IsDtlsSctp(media_desc->protocol())) { BuildSctpContentAttributes(media_desc, message); } else if (IsRtpProtocol(media_desc->protocol())) { - BuildRtpContentAttributes(media_desc, media_type, msid_signaling, message); + // TODO: issues.webrtc.org/48979442 - Make this field trial controlled + BuildRtpContentAttributes(media_desc, media_type, msid_signaling, + /* use_wildcard= */ false, message); } }
diff --git a/pc/congestion_control_integrationtest.cc b/pc/congestion_control_integrationtest.cc index b712b6b..4cf7793 100644 --- a/pc/congestion_control_integrationtest.cc +++ b/pc/congestion_control_integrationtest.cc
@@ -13,6 +13,7 @@ #include <memory> #include <string> +#include <string_view> #include <vector> #include "absl/strings/str_cat.h" @@ -31,6 +32,7 @@ namespace webrtc { +using ::testing::ContainsRegex; using ::testing::Eq; using ::testing::Field; using ::testing::Gt; @@ -45,6 +47,9 @@ : PeerConnectionIntegrationBaseTest(SdpSemantics::kUnifiedPlan) {} }; +// This regexp matches both wildcard and non-wildcard ccfb lines. +constexpr std::string_view ccfb_regex = "a=rtcp-fb:[0-9*]* ack ccfb\r\n"; + TEST_F(PeerConnectionCongestionControlTest, OfferDoesNotContainCcfbEvenIfEnabled) { SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); @@ -53,7 +58,7 @@ std::unique_ptr<SessionDescriptionInterface> offer = caller()->CreateOfferAndWait(); std::string offer_str = absl::StrCat(*offer); - EXPECT_THAT(offer_str, Not(HasSubstr("a=rtcp-fb:* ack ccfb\r\n"))); + EXPECT_THAT(offer_str, Not(ContainsRegex(ccfb_regex))); } TEST_F(PeerConnectionCongestionControlTest, OfferContainsCcfbIfFieldTrial) { @@ -63,7 +68,7 @@ std::unique_ptr<SessionDescriptionInterface> offer = caller()->CreateOfferAndWait(); std::string offer_str = absl::StrCat(*offer); - EXPECT_THAT(offer_str, HasSubstr("a=rtcp-fb:* ack ccfb\r\n")); + EXPECT_THAT(offer_str, ContainsRegex(ccfb_regex)); } TEST_F(PeerConnectionCongestionControlTest, ReceiveOfferSetsCcfbFlag) {
diff --git a/test/peer_scenario/bwe_integration_tests/l4s_test.cc b/test/peer_scenario/bwe_integration_tests/l4s_test.cc index 98fdc96..1d1f5c3 100644 --- a/test/peer_scenario/bwe_integration_tests/l4s_test.cc +++ b/test/peer_scenario/bwe_integration_tests/l4s_test.cc
@@ -11,6 +11,7 @@ #include <atomic> #include <map> #include <string> +#include <string_view> #include <vector> #include "absl/strings/str_cat.h" @@ -54,12 +55,16 @@ using test::GetStatsAndProcess; using test::PeerScenario; using test::PeerScenarioClient; +using ::testing::ContainsRegex; using ::testing::HasSubstr; using ::testing::TestWithParam; // RTC event logs can be gathered from these tests. // Add --peer_logs=true --peer_logs_root=/tmp/l4s/ to write logs to /tmp/l4s +// This regexp matches both wildcard and non-wildcard ccfb lines. +constexpr std::string_view ccfb_regex = "a=rtcp-fb:[0-9*]* ack ccfb\r\n"; + // Helper class used for counting RTCP feedback messages. class RtcpFeedbackCounter { public: @@ -161,7 +166,7 @@ // Check that the offer contain both congestion control feedback // according to RFC 8888, and transport-cc and the header extension // http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01 - EXPECT_THAT(offer_str, HasSubstr("a=rtcp-fb:* ack ccfb\r\n")); + EXPECT_THAT(offer_str, ContainsRegex(ccfb_regex)); EXPECT_THAT(offer_str, HasSubstr("transport-cc")); EXPECT_THAT( offer_str, @@ -170,7 +175,7 @@ }, [&](const SessionDescriptionInterface& answer) { std::string answer_str = absl::StrCat(answer); - EXPECT_THAT(answer_str, HasSubstr("a=rtcp-fb:* ack ccfb\r\n")); + EXPECT_THAT(answer_str, ContainsRegex(ccfb_regex)); // Check that the answer does not contain transport-cc nor the // header extension // http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01 @@ -266,7 +271,7 @@ std::string answer_str; caller->pc()->local_description()->ToString(&answer_str); ASSERT_FALSE(answer_str.empty()); - ASSERT_THAT(answer_str, HasSubstr("a=rtcp-fb:* ack ccfb\r\n")); + ASSERT_THAT(answer_str, ContainsRegex(ccfb_regex)); callee->CreateAndSetSdp( [&](SessionDescriptionInterface* /*munge_offer*/) { @@ -274,7 +279,7 @@ }, [&](std::string offer) { // Callee does not support ccfb and does not have it in the offer. - ASSERT_THAT(offer, Not(HasSubstr("a=rtcp-fb:* ack ccfb\r\n"))); + ASSERT_THAT(offer, Not(ContainsRegex(ccfb_regex))); caller->SetRemoteDescription( offer, SdpType::kOffer, [&](RTCError error) { ASSERT_TRUE(error.ok());