diff --git a/media/base/codec.cc b/media/base/codec.cc index 7f38a23dad..dfff5176fb 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc @@ -68,6 +68,15 @@ void FeedbackParams::Add(const FeedbackParam& param) { RTC_CHECK(!HasDuplicateEntries()); } +bool FeedbackParams::Remove(const FeedbackParam& param) { + if (!Has(param)) { + return false; + } + params_.erase(std::remove(params_.begin(), params_.end(), param), + params_.end()); + return true; +} + void FeedbackParams::Intersect(const FeedbackParams& from) { std::vector::iterator iter_to = params_.begin(); while (iter_to != params_.end()) { diff --git a/media/base/codec.h b/media/base/codec.h index 542607a0cd..d96ea842cc 100644 --- a/media/base/codec.h +++ b/media/base/codec.h @@ -56,6 +56,7 @@ class FeedbackParams { bool Has(const FeedbackParam& param) const; void Add(const FeedbackParam& param); + bool Remove(const FeedbackParam& param); void Intersect(const FeedbackParams& from); diff --git a/pc/congestion_control_integrationtest.cc b/pc/congestion_control_integrationtest.cc index 2ac804bba5..0986a2d20c 100644 --- a/pc/congestion_control_integrationtest.cc +++ b/pc/congestion_control_integrationtest.cc @@ -25,6 +25,7 @@ namespace webrtc { using testing::Eq; using testing::HasSubstr; +using testing::Not; class PeerConnectionCongestionControlTest : public PeerConnectionIntegrationBaseTest { @@ -65,6 +66,9 @@ TEST_F(PeerConnectionCongestionControlTest, ReceiveOfferSetsCcfbFlag) { for (const auto& content : parsed_contents) { EXPECT_TRUE(content.media_description()->rtcp_fb_ack_ccfb()); } + // Check that the answer does not contain transport-cc + std::string answer_str = absl::StrCat(*caller()->pc()->remote_description()); + EXPECT_THAT(answer_str, Not(HasSubstr("transport-cc"))); } TEST_F(PeerConnectionCongestionControlTest, CcfbGetsUsed) { @@ -82,6 +86,9 @@ TEST_F(PeerConnectionCongestionControlTest, CcfbGetsUsed) { auto pc_internal = caller()->pc_internal(); EXPECT_TRUE_WAIT(pc_internal->FeedbackAccordingToRfc8888CountForTesting() > 0, kDefaultTimeout); + // There should be no transport-cc generated. + EXPECT_THAT(pc_internal->FeedbackAccordingToTransportCcCountForTesting(), + Eq(0)); } TEST_F(PeerConnectionCongestionControlTest, TransportCcGetsUsed) { diff --git a/pc/media_session.cc b/pc/media_session.cc index 9e73ea65f8..3e0d762d10 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1542,6 +1542,22 @@ MediaSessionDescriptionFactory::CreateAnswerOrError( StreamParamsVec current_streams = GetCurrentStreamParams(current_active_contents); + // Decide what congestion control feedback format we're using. + bool has_ack_ccfb = false; + if (transport_desc_factory_->trials().IsEnabled( + "WebRTC-RFC8888CongestionControlFeedback")) { + for (const auto& content : offer->contents()) { + if (content.media_description()->rtcp_fb_ack_ccfb()) { + has_ack_ccfb = true; + } else if (has_ack_ccfb) { + RTC_LOG(LS_ERROR) + << "Inconsistent rtcp_fb_ack_ccfb marking, ignoring all"; + has_ack_ccfb = false; + break; + } + } + } + // Get list of all possible codecs that respects existing payload type // mappings and uses a single payload type space. // @@ -1601,9 +1617,17 @@ MediaSessionDescriptionFactory::CreateAnswerOrError( msection_index < current_description->contents().size()) { current_content = ¤t_description->contents()[msection_index]; } + // Don't offer the transport-cc header extension if "ack ccfb" is in use. + auto header_extensions_in = media_description_options.header_extensions; + if (has_ack_ccfb) { + for (auto& option : header_extensions_in) { + if (option.uri == webrtc::RtpExtension::kTransportSequenceNumberUri) { + option.direction = RtpTransceiverDirection::kStopped; + } + } + } RtpHeaderExtensions header_extensions = RtpHeaderExtensionsFromCapabilities( - UnstoppedRtpHeaderExtensionCapabilities( - media_description_options.header_extensions)); + UnstoppedRtpHeaderExtensionCapabilities(header_extensions_in)); RTCError error; switch (media_description_options.type) { case MEDIA_TYPE_AUDIO: @@ -2220,7 +2244,16 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForAnswer( } AssignCodecIdsAndLinkRed(pt_suggester_, media_description_options.mid, codecs_to_include); - + // RFC 8888 support. Only answer with "ack ccfb" if offer has it and + // experiment is enabled. + if (offer_content_description->rtcp_fb_ack_ccfb()) { + answer_content->set_rtcp_fb_ack_ccfb( + transport_desc_factory_->trials().IsEnabled( + "WebRTC-RFC8888CongestionControlFeedback")); + for (auto& codec : codecs_to_include) { + codec.feedback_params.Remove(FeedbackParam(kRtcpFbParamTransportCc)); + } + } if (!SetCodecsInAnswer(offer_content_description, codecs_to_include, media_description_options, session_options, ssrc_generator(), current_streams, @@ -2229,15 +2262,6 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForAnswer( LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, "Failed to set codecs in answer"); } - // RFC 8888 support. Only answer with "ack ccfb" if offer has it and - // experiment is enabled. - // TODO: https://issues.webrtc.org/42225697 - disable transport-cc - // when ccfb is negotiated. - if (offer_content_description->rtcp_fb_ack_ccfb()) { - answer_content->set_rtcp_fb_ack_ccfb( - transport_desc_factory_->trials().IsEnabled( - "WebRTC-RFC8888CongestionControlFeedback")); - } if (!CreateMediaContentAnswer( offer_content_description, media_description_options, session_options, filtered_rtp_header_extensions(header_extensions), ssrc_generator(),