Refine RFC8888 CCFB enablement logic and add test.
The `SdpOfferAnswerHandler::PushdownMediaDescription` now checks if the remote description supports RFC8888 Congestion Control Feedback when processing an answer. This ensures that RFC8888 feedback is only enabled if both sides signal support.
Bug: webrtc:438613826
Change-Id: Iecf2050cb5f2110e010ce056af6b58077a60891e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/404482
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Auto-Submit: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#45346}
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 174b4e8..1ccc4e8 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -5040,9 +5040,9 @@
ContentSource source,
const std::map<std::string, const ContentGroup*>& bundle_groups_by_mid) {
TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::PushdownMediaDescription");
+ RTC_DCHECK_RUN_ON(signaling_thread());
const SessionDescriptionInterface* sdesc =
(source == CS_LOCAL ? local_description() : remote_description());
- RTC_DCHECK_RUN_ON(signaling_thread());
RTC_DCHECK(sdesc);
if (ConfiguredForMedia()) {
@@ -5114,11 +5114,29 @@
// If local and remote are both set, we assume that it's safe to trigger
// CCFB.
if (pc_->trials().IsEnabled("WebRTC-RFC8888CongestionControlFeedback")) {
- if (use_ccfb && local_description() && remote_description()) {
- // The call and the congestion controller live on the worker thread.
- context_->worker_thread()->PostTask([call = pc_->call_ptr()] {
- call->EnableSendCongestionControlFeedbackAccordingToRfc8888();
- });
+ if (type == SdpType::kAnswer && use_ccfb && local_description() &&
+ remote_description()) {
+ bool remote_supports_ccfb = true;
+ // Verify that the remote supports CCFB before enabling.
+ for (const auto& content :
+ remote_description()->description()->contents()) {
+ if (content.type != MediaProtocolType::kRtp || content.rejected ||
+ content.media_description() == nullptr) {
+ continue;
+ }
+ if (!content.media_description()->rtcp_fb_ack_ccfb()) {
+ remote_supports_ccfb = false;
+ break;
+ }
+ }
+ if (remote_supports_ccfb) {
+ // The call and the congestion controller live on the worker thread.
+ context_->worker_thread()->PostTask([call = pc_->call_ptr()] {
+ call->EnableSendCongestionControlFeedbackAccordingToRfc8888();
+ });
+ } else {
+ RTC_LOG(LS_WARNING) << "Local but not Remote supports CCFB.";
+ }
}
}
}
diff --git a/test/peer_scenario/peer_scenario_client.cc b/test/peer_scenario/peer_scenario_client.cc
index e9aa2ae..740e3ba 100644
--- a/test/peer_scenario/peer_scenario_client.cc
+++ b/test/peer_scenario/peer_scenario_client.cc
@@ -465,6 +465,28 @@
}));
}
+void PeerScenarioClient::SetLocalDescription(
+ std::string sdp,
+ SdpType type,
+ std::function<void(RTCError)> on_complete) {
+ RTC_DCHECK_RUN_ON(signaling_thread_);
+ peer_connection_->SetLocalDescription(
+ CreateSessionDescription(type, sdp),
+ make_ref_counted<LambdaSetLocalDescriptionObserver>(
+ std::move(on_complete)));
+}
+
+void PeerScenarioClient::SetRemoteDescription(
+ std::string sdp,
+ SdpType type,
+ std::function<void(RTCError)> on_complete) {
+ RTC_DCHECK_RUN_ON(signaling_thread_);
+ peer_connection_->SetRemoteDescription(
+ CreateSessionDescription(type, sdp),
+ make_ref_counted<LambdaSetRemoteDescriptionObserver>(
+ std::move(on_complete)));
+}
+
void PeerScenarioClient::AddIceCandidate(
std::unique_ptr<IceCandidate> candidate) {
RTC_DCHECK_RUN_ON(signaling_thread_);
diff --git a/test/peer_scenario/peer_scenario_client.h b/test/peer_scenario/peer_scenario_client.h
index a4c4b48..7ef3df6 100644
--- a/test/peer_scenario/peer_scenario_client.h
+++ b/test/peer_scenario/peer_scenario_client.h
@@ -26,6 +26,7 @@
#include "api/jsep.h"
#include "api/media_stream_interface.h"
#include "api/peer_connection_interface.h"
+#include "api/rtc_error.h"
#include "api/rtp_receiver_interface.h"
#include "api/rtp_sender_interface.h"
#include "api/rtp_transceiver_interface.h"
@@ -174,6 +175,13 @@
std::function<void(const SessionDescriptionInterface& answer)>
done_handler);
+ void SetLocalDescription(std::string sdp,
+ SdpType type,
+ std::function<void(RTCError)> on_complete);
+ void SetRemoteDescription(std::string sdp,
+ SdpType type,
+ std::function<void(RTCError)> on_complete);
+
// Adds the given ice candidate when the peer connection is ready.
void AddIceCandidate(std::unique_ptr<IceCandidate> candidate);
diff --git a/test/peer_scenario/tests/BUILD.gn b/test/peer_scenario/tests/BUILD.gn
index b4c1800..e156fac 100644
--- a/test/peer_scenario/tests/BUILD.gn
+++ b/test/peer_scenario/tests/BUILD.gn
@@ -25,6 +25,7 @@
"../../../api:audio_options_api",
"../../../api:libjingle_peerconnection_api",
"../../../api:make_ref_counted",
+ "../../../api:rtc_error",
"../../../api:rtc_stats_api",
"../../../api:rtp_parameters",
"../../../api:rtp_transceiver_direction",
diff --git a/test/peer_scenario/tests/l4s_test.cc b/test/peer_scenario/tests/l4s_test.cc
index 293c6bf..5f562ef 100644
--- a/test/peer_scenario/tests/l4s_test.cc
+++ b/test/peer_scenario/tests/l4s_test.cc
@@ -16,6 +16,7 @@
#include "absl/strings/str_cat.h"
#include "api/jsep.h"
#include "api/make_ref_counted.h"
+#include "api/rtc_error.h"
#include "api/scoped_refptr.h"
#include "api/stats/rtc_stats_report.h"
#include "api/stats/rtcstats_objects.h"
@@ -327,6 +328,114 @@
return info.param.test_suffix;
});
+TEST(L4STest, NoCcfbSentAfterRenegotiationAndCallerCachLocalDescription) {
+ // The caller supports CCFB, but the callee does not.
+ // This test that the caller does not start sending CCFB after renegotiation
+ // even if the local description is cached. The callers local description
+ // will contain CCFB since it was used in the initial offer.
+ PeerScenario s(*test_info_);
+ PeerScenarioClient::Config caller_config;
+ caller_config.disable_encryption = true;
+ caller_config.field_trials.Set("WebRTC-RFC8888CongestionControlFeedback",
+ "Enabled");
+ PeerScenarioClient* caller = s.CreateClient(caller_config);
+
+ PeerScenarioClient::Config callee_config;
+ callee_config.disable_encryption = true;
+ callee_config.field_trials.Set("WebRTC-RFC8888CongestionControlFeedback",
+ "Disabled");
+ PeerScenarioClient* callee = s.CreateClient(callee_config);
+
+ auto caller_to_callee = s.net()
+ ->NodeBuilder()
+ .capacity(DataRate::KilobitsPerSec(600))
+ .Build()
+ .node;
+ auto callee_to_caller = s.net()
+ ->NodeBuilder()
+ .capacity(DataRate::KilobitsPerSec(600))
+ .Build()
+ .node;
+ RtcpFeedbackCounter callee_feedback_counter;
+ caller_to_callee->router()->SetWatcher([&](const EmulatedIpPacket& packet) {
+ callee_feedback_counter.Count(packet);
+ });
+ RtcpFeedbackCounter caller_feedback_counter;
+ callee_to_caller->router()->SetWatcher([&](const EmulatedIpPacket& packet) {
+ caller_feedback_counter.Count(packet);
+ });
+
+ s.net()->CreateRoute(caller->endpoint(), {caller_to_callee},
+ callee->endpoint());
+ s.net()->CreateRoute(callee->endpoint(), {callee_to_caller},
+ caller->endpoint());
+
+ auto signaling = s.ConnectSignaling(caller, callee, {caller_to_callee},
+ {callee_to_caller});
+ PeerScenarioClient::VideoSendTrackConfig video_conf;
+ video_conf.generator.squares_video->framerate = 30;
+ video_conf.generator.squares_video->width = 640;
+ video_conf.generator.squares_video->height = 360;
+ caller->CreateVideo("FROM_CALLER", video_conf);
+ callee->CreateVideo("FROM_CALLEE", video_conf);
+
+ signaling.StartIceSignaling();
+ std::atomic<bool> offer_exchange_done(false);
+ signaling.NegotiateSdp([&](const SessionDescriptionInterface& answer) {
+ offer_exchange_done = true;
+ });
+ ASSERT_TRUE(s.WaitAndProcess(&offer_exchange_done));
+ s.ProcessMessages(TimeDelta::Seconds(2));
+
+ EXPECT_EQ(caller_feedback_counter.FeedbackAccordingToRfc8888(), 0);
+ EXPECT_EQ(callee_feedback_counter.FeedbackAccordingToRfc8888(), 0);
+ int transport_cc_caller =
+ caller_feedback_counter.FeedbackAccordingToTransportCc();
+ int transport_cc_callee =
+ callee_feedback_counter.FeedbackAccordingToTransportCc();
+ EXPECT_GT(transport_cc_caller, 0);
+ EXPECT_GT(transport_cc_callee, 0);
+
+ offer_exchange_done = false;
+ // Save the caller's local description and use it as answer to the next offer
+ // from callee.
+ 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"));
+
+ callee->CreateAndSetSdp(
+ [&](SessionDescriptionInterface* /*munge_offer*/) {
+ // Do not munge the offer.
+ },
+ [&](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")));
+ caller->SetRemoteDescription(
+ offer, SdpType::kOffer, [&](RTCError error) {
+ ASSERT_TRUE(error.ok());
+ caller->SetLocalDescription(
+ answer_str, SdpType::kAnswer, [&](RTCError error) {
+ ASSERT_TRUE(error.ok());
+ callee->SetRemoteDescription(answer_str, SdpType::kAnswer,
+ [&](RTCError error) {
+ ASSERT_TRUE(error.ok());
+ offer_exchange_done = true;
+ });
+ });
+ });
+ });
+ ASSERT_TRUE(s.WaitAndProcess(&offer_exchange_done));
+ s.ProcessMessages(TimeDelta::Seconds(4));
+
+ EXPECT_EQ(caller_feedback_counter.FeedbackAccordingToRfc8888(), 0);
+ EXPECT_EQ(callee_feedback_counter.FeedbackAccordingToRfc8888(), 0);
+ EXPECT_GT(caller_feedback_counter.FeedbackAccordingToTransportCc(),
+ transport_cc_caller);
+ EXPECT_GT(callee_feedback_counter.FeedbackAccordingToTransportCc(),
+ transport_cc_callee);
+}
+
// Note - this test only test that the
// caller adapt to the link capacity. It does not test that the caller uses ECN
// to adapt even though the network can mark packets with CE.
