Add functions to send feedback according to RFC 8888 in ReceiveSideCongestionController.
With this cl, sending can be forced with field trial "WebRTC-RFC8888CongestionControlFeedback/force_send:true/"
In the future, ReceiveSideCongestionController::EnablSendCongestionControlFeedbackAccordingToRfc8888 if RFC 8888 has been negotiated.
Bug: webrtc:42225697
Change-Id: Ib09066aa89ca7b3fffc551da541090c69ab8d75f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/352720
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42413}
diff --git a/modules/congestion_controller/BUILD.gn b/modules/congestion_controller/BUILD.gn
index 22a7f09..2c00735 100644
--- a/modules/congestion_controller/BUILD.gn
+++ b/modules/congestion_controller/BUILD.gn
@@ -19,9 +19,11 @@
deps = [
"../../api:rtp_parameters",
+ "../../api:sequence_checker",
"../../api/environment",
"../../api/transport:network_control",
"../../api/units:data_rate",
+ "../../api/units:data_size",
"../../api/units:time_delta",
"../../api/units:timestamp",
"../../rtc_base:logging",
@@ -29,6 +31,7 @@
"../../rtc_base/synchronization:mutex",
"../pacing",
"../remote_bitrate_estimator",
+ "../remote_bitrate_estimator:congestion_control_feedback_generator",
"../remote_bitrate_estimator:transport_sequence_number_feedback_generator",
"../rtp_rtcp:rtp_rtcp_format",
"//third_party/abseil-cpp/absl/base:nullability",
@@ -45,6 +48,7 @@
]
deps = [
":congestion_controller",
+ "../../api:rtp_parameters",
"../../api/environment:environment_factory",
"../../api/test/network_emulation",
"../../api/test/network_emulation:create_cross_traffic",
@@ -53,6 +57,7 @@
"../../api/units:time_delta",
"../../api/units:timestamp",
"../../system_wrappers",
+ "../../test:explicit_key_value_config",
"../../test:test_support",
"../../test/scenario",
"../pacing",
diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h
index deb3a3e..7dec287 100644
--- a/modules/congestion_controller/include/receive_side_congestion_controller.h
+++ b/modules/congestion_controller/include/receive_side_congestion_controller.h
@@ -15,10 +15,12 @@
#include "absl/base/nullability.h"
#include "api/environment/environment.h"
+#include "api/sequence_checker.h"
#include "api/transport/network_control.h"
#include "api/units/data_rate.h"
#include "api/units/time_delta.h"
#include "modules/congestion_controller/remb_throttler.h"
+#include "modules/remote_bitrate_estimator/congestion_control_feedback_generator.h"
#include "modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h"
#include "modules/rtp_rtcp/source/rtp_packet_received.h"
#include "rtc_base/synchronization/mutex.h"
@@ -42,6 +44,8 @@
~ReceiveSideCongestionController() override = default;
+ void EnablSendCongestionControlFeedbackAccordingToRfc8888();
+
void OnReceivedPacket(const RtpPacketReceived& packet, MediaType media_type);
// Implements CallStatsObserver.
@@ -74,8 +78,18 @@
const Environment env_;
RembThrottler remb_throttler_;
+
+ // TODO: bugs.webrtc.org/42224904 - Use sequence checker for all usage of
+ // ReceiveSideCongestionController. At the time of
+ // writing OnReceivedPacket and MaybeProcess can unfortunately be called on an
+ // arbitrary thread by external projects.
+ SequenceChecker sequence_checker_;
+
+ bool send_rfc8888_congestion_feedback_ = false;
TransportSequenceNumberFeedbackGenenerator
transport_sequence_number_feedback_generator_;
+ CongestionControlFeedbackGenerator congestion_control_feedback_generator_
+ RTC_GUARDED_BY(sequence_checker_);
mutable Mutex mutex_;
std::unique_ptr<RemoteBitrateEstimator> rbe_ RTC_GUARDED_BY(mutex_);
diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc
index c86b277..591dba3 100644
--- a/modules/congestion_controller/receive_side_congestion_controller.cc
+++ b/modules/congestion_controller/receive_side_congestion_controller.cc
@@ -10,14 +10,18 @@
#include "modules/congestion_controller/include/receive_side_congestion_controller.h"
+#include <memory>
+
#include "absl/base/nullability.h"
#include "api/environment/environment.h"
#include "api/media_types.h"
+#include "api/sequence_checker.h"
#include "api/units/data_rate.h"
-#include "modules/pacing/packet_router.h"
-#include "modules/remote_bitrate_estimator/include/bwe_defines.h"
+#include "modules/remote_bitrate_estimator/congestion_control_feedback_generator.h"
#include "modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h"
#include "modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h"
+#include "modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h"
+#include "modules/rtp_rtcp/source/rtp_header_extensions.h"
#include "rtc_base/logging.h"
namespace webrtc {
@@ -77,17 +81,37 @@
absl::Nullable<NetworkStateEstimator*> network_state_estimator)
: env_(env),
remb_throttler_(std::move(remb_sender), &env_.clock()),
- transport_sequence_number_feedback_generator_(std::move(feedback_sender),
+ transport_sequence_number_feedback_generator_(feedback_sender,
network_state_estimator),
+ congestion_control_feedback_generator_(env, feedback_sender),
rbe_(std::make_unique<RemoteBitrateEstimatorSingleStream>(
env_,
&remb_throttler_)),
using_absolute_send_time_(false),
- packets_since_absolute_send_time_(0) {}
+ packets_since_absolute_send_time_(0) {
+ FieldTrialParameter<bool> force_send_rfc8888_feedback("force_send", false);
+ ParseFieldTrial(
+ {&force_send_rfc8888_feedback},
+ env.field_trials().Lookup("WebRTC-RFC8888CongestionControlFeedback"));
+ if (force_send_rfc8888_feedback) {
+ EnablSendCongestionControlFeedbackAccordingToRfc8888();
+ }
+}
+
+void ReceiveSideCongestionController::
+ EnablSendCongestionControlFeedbackAccordingToRfc8888() {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
+ send_rfc8888_congestion_feedback_ = true;
+}
void ReceiveSideCongestionController::OnReceivedPacket(
const RtpPacketReceived& packet,
MediaType media_type) {
+ if (send_rfc8888_congestion_feedback_) {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
+ congestion_control_feedback_generator_.OnReceivedPacket(packet);
+ return;
+ }
bool has_transport_sequence_number =
packet.HasExtension<TransportSequenceNumber>() ||
packet.HasExtension<TransportSequenceNumberV2>();
@@ -108,12 +132,20 @@
}
void ReceiveSideCongestionController::OnBitrateChanged(int bitrate_bps) {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
+ DataRate send_bandwidth_estimate = DataRate::BitsPerSec(bitrate_bps);
transport_sequence_number_feedback_generator_.OnSendBandwidthEstimateChanged(
- DataRate::BitsPerSec(bitrate_bps));
+ send_bandwidth_estimate);
+ congestion_control_feedback_generator_.OnSendBandwidthEstimateChanged(
+ send_bandwidth_estimate);
}
TimeDelta ReceiveSideCongestionController::MaybeProcess() {
Timestamp now = env_.clock().CurrentTime();
+ if (send_rfc8888_congestion_feedback_) {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
+ return congestion_control_feedback_generator_.Process(now);
+ }
mutex_.Lock();
TimeDelta time_until_rbe = rbe_->Process();
mutex_.Unlock();
@@ -130,8 +162,11 @@
void ReceiveSideCongestionController::SetTransportOverhead(
DataSize overhead_per_packet) {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
transport_sequence_number_feedback_generator_.SetTransportOverhead(
overhead_per_packet);
+ congestion_control_feedback_generator_.SetTransportOverhead(
+ overhead_per_packet);
}
} // namespace webrtc
diff --git a/modules/congestion_controller/receive_side_congestion_controller_unittest.cc b/modules/congestion_controller/receive_side_congestion_controller_unittest.cc
index 5788898..8681560 100644
--- a/modules/congestion_controller/receive_side_congestion_controller_unittest.cc
+++ b/modules/congestion_controller/receive_side_congestion_controller_unittest.cc
@@ -11,17 +11,18 @@
#include "modules/congestion_controller/include/receive_side_congestion_controller.h"
#include "api/environment/environment_factory.h"
+#include "api/media_types.h"
#include "api/test/network_emulation/create_cross_traffic.h"
#include "api/test/network_emulation/cross_traffic.h"
#include "api/units/data_rate.h"
#include "api/units/data_size.h"
#include "api/units/time_delta.h"
#include "api/units/timestamp.h"
-#include "modules/pacing/packet_router.h"
#include "modules/rtp_rtcp/include/rtp_header_extension_map.h"
#include "modules/rtp_rtcp/source/rtp_header_extensions.h"
#include "modules/rtp_rtcp/source/rtp_packet_received.h"
#include "system_wrappers/include/clock.h"
+#include "test/explicit_key_value_config.h"
#include "test/gmock.h"
#include "test/gtest.h"
#include "test/scenario/scenario.h"
@@ -34,6 +35,7 @@
using ::testing::AtLeast;
using ::testing::ElementsAre;
using ::testing::MockFunction;
+using ::testing::SizeIs;
constexpr DataRate kInitialBitrate = DataRate::BitsPerSec(60'000);
@@ -81,6 +83,76 @@
controller.SetMaxDesiredReceiveBitrate(DataRate::BitsPerSec(123));
}
+TEST(ReceiveSideCongestionControllerTest, SendsRfc8888FeedbackIfForced) {
+ test::ExplicitKeyValueConfig field_trials(
+ "WebRTC-RFC8888CongestionControlFeedback/force_send:true/");
+ MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
+ rtcp_sender;
+ MockFunction<void(uint64_t, std::vector<uint32_t>)> remb_sender;
+ SimulatedClock clock(123456);
+ ReceiveSideCongestionController controller(
+ CreateEnvironment(&clock, &field_trials), rtcp_sender.AsStdFunction(),
+ remb_sender.AsStdFunction(), nullptr);
+
+ EXPECT_CALL(rtcp_sender, Call);
+ RtpPacketReceived packet;
+ packet.set_arrival_time(clock.CurrentTime());
+ controller.OnReceivedPacket(packet, MediaType::VIDEO);
+ TimeDelta next_process = controller.MaybeProcess();
+ clock.AdvanceTime(next_process);
+ next_process = controller.MaybeProcess();
+}
+
+TEST(ReceiveSideCongestionControllerTest, SendsRfc8888FeedbackIfEnabled) {
+ MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
+ rtcp_sender;
+ MockFunction<void(uint64_t, std::vector<uint32_t>)> remb_sender;
+ SimulatedClock clock(123456);
+ ReceiveSideCongestionController controller(
+ CreateEnvironment(&clock), rtcp_sender.AsStdFunction(),
+ remb_sender.AsStdFunction(), nullptr);
+ controller.EnablSendCongestionControlFeedbackAccordingToRfc8888();
+
+ EXPECT_CALL(rtcp_sender, Call)
+ .WillOnce(
+ [&](std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets) {
+ ASSERT_THAT(rtcp_packets, SizeIs(1));
+ rtc::Buffer buffer = rtcp_packets[0]->Build();
+ rtcp::CommonHeader header;
+ EXPECT_TRUE(header.Parse(buffer.data(), buffer.size()));
+ EXPECT_EQ(header.fmt(),
+ rtcp::CongestionControlFeedback::kFeedbackMessageType);
+ });
+
+ RtpPacketReceived packet;
+ packet.set_arrival_time(clock.CurrentTime());
+ controller.OnReceivedPacket(packet, MediaType::VIDEO);
+ TimeDelta next_process = controller.MaybeProcess();
+ clock.AdvanceTime(next_process);
+ next_process = controller.MaybeProcess();
+}
+
+TEST(ReceiveSideCongestionControllerTest,
+ SendsNoFeedbackIfNotRfcRfc8888EnabledAndNoTransportFeedback) {
+ MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
+ rtcp_sender;
+ MockFunction<void(uint64_t, std::vector<uint32_t>)> remb_sender;
+ SimulatedClock clock(123456);
+ ReceiveSideCongestionController controller(
+ CreateEnvironment(&clock), rtcp_sender.AsStdFunction(),
+ remb_sender.AsStdFunction(), nullptr);
+
+ // No Transport feedback is sent because received packet does not have
+ // transport sequence number rtp header extension.
+ EXPECT_CALL(rtcp_sender, Call).Times(0);
+ RtpPacketReceived packet;
+ packet.set_arrival_time(clock.CurrentTime());
+ controller.OnReceivedPacket(packet, MediaType::VIDEO);
+ TimeDelta next_process = controller.MaybeProcess();
+ clock.AdvanceTime(next_process);
+ next_process = controller.MaybeProcess();
+}
+
TEST(ReceiveSideCongestionControllerTest, ConvergesToCapacity) {
Scenario s("receive_cc_unit/converge");
NetworkSimulationConfig net_conf;
diff --git a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h
index 7183d5f..3a65745 100644
--- a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h
+++ b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h
@@ -44,10 +44,9 @@
class CongestionControlFeedbackGenerator
: public RtpTransportFeedbackGenerator {
public:
- using RtcpSender = std::function<void(
- std::vector<std::unique_ptr<rtcp::RtcpPacket>> packets)>;
- CongestionControlFeedbackGenerator(const Environment& env,
- RtcpSender feedback_sender);
+ CongestionControlFeedbackGenerator(
+ const Environment& env,
+ RtpTransportFeedbackGenerator::RtcpSender feedback_sender);
~CongestionControlFeedbackGenerator() = default;
void OnReceivedPacket(const RtpPacketReceived& packet) override;
diff --git a/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h b/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h
index 724b589..d835608 100644
--- a/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h
+++ b/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h
@@ -11,6 +11,9 @@
#ifndef MODULES_REMOTE_BITRATE_ESTIMATOR_RTP_TRANSPORT_FEEDBACK_GENERATOR_H_
#define MODULES_REMOTE_BITRATE_ESTIMATOR_RTP_TRANSPORT_FEEDBACK_GENERATOR_H_
+#include <memory>
+#include <vector>
+
#include "api/units/data_rate.h"
#include "api/units/data_size.h"
#include "api/units/time_delta.h"
@@ -21,6 +24,10 @@
class RtpTransportFeedbackGenerator {
public:
+ // Function intented to be used for sending RTCP messages generated by an
+ // implementation of this class.
+ using RtcpSender = std::function<void(
+ std::vector<std::unique_ptr<rtcp::RtcpPacket>> packets)>;
virtual ~RtpTransportFeedbackGenerator() = default;
virtual void OnReceivedPacket(const RtpPacketReceived& packet) = 0;
diff --git a/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h b/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h
index 2291031..4c2fb31 100644
--- a/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h
+++ b/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h
@@ -44,12 +44,8 @@
class TransportSequenceNumberFeedbackGenenerator
: public RtpTransportFeedbackGenerator {
public:
- // Used for sending transport feedback messages when send side
- // BWE is used.
- using RtcpSender = std::function<void(
- std::vector<std::unique_ptr<rtcp::RtcpPacket>> packets)>;
TransportSequenceNumberFeedbackGenenerator(
- RtcpSender feedback_sender,
+ RtpTransportFeedbackGenerator::RtcpSender feedback_sender,
NetworkStateEstimator* network_state_estimator);
~TransportSequenceNumberFeedbackGenenerator();
