diff --git a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.cc b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.cc index 7ef1b60a56..9e6bd4115f 100644 --- a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.cc +++ b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.cc @@ -28,9 +28,12 @@ #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/experiments/field_trial_parser.h" +#include "rtc_base/logging.h" namespace webrtc { +constexpr DataRate kMaxFeedbackRate = webrtc::DataRate::KilobitsPerSec(500); + CongestionControlFeedbackGenerator::CongestionControlFeedbackGenerator( const Environment& env, RtcpSender rtcp_sender) @@ -39,7 +42,7 @@ CongestionControlFeedbackGenerator::CongestionControlFeedbackGenerator( min_time_between_feedback_("min_send_delta", TimeDelta::Millis(25)), max_time_to_wait_for_packet_with_marker_("max_wait_for_marker", TimeDelta::Millis(25)), - max_time_between_feedback_("max_send_delta", TimeDelta::Millis(250)) { + max_time_between_feedback_("max_send_delta", TimeDelta::Millis(500)) { ParseFieldTrial( {&min_time_between_feedback_, &max_time_to_wait_for_packet_with_marker_, &max_time_between_feedback_}, @@ -82,14 +85,8 @@ TimeDelta CongestionControlFeedbackGenerator::Process(Timestamp now) { return NextFeedbackTime() - now; } -void CongestionControlFeedbackGenerator::OnSendBandwidthEstimateChanged( - DataRate estimate) { - RTC_DCHECK_RUN_ON(&sequence_checker_); - // Feedback reports should max occupy 5% of total bandwidth. - max_feedback_rate_ = estimate * 0.05; -} - void CongestionControlFeedbackGenerator::SendFeedback(Timestamp now) { + RTC_DCHECK_GE(now, next_possible_feedback_send_time_); uint32_t compact_ntp = CompactNtp(env_.clock().ConvertTimestampToNtpTime(now)); std::vector rtcp_packet_info; @@ -112,15 +109,13 @@ void CongestionControlFeedbackGenerator::CalculateNextPossibleSendTime( DataSize feedback_size, Timestamp now) { TimeDelta time_since_last_sent = now - last_feedback_sent_time_; - DataSize debt_payed = time_since_last_sent * max_feedback_rate_; + DataSize debt_payed = time_since_last_sent * kMaxFeedbackRate; send_rate_debt_ = debt_payed > send_rate_debt_ ? DataSize::Zero() : send_rate_debt_ - debt_payed; send_rate_debt_ += feedback_size; last_feedback_sent_time_ = now; next_possible_feedback_send_time_ = - now + std::clamp(max_feedback_rate_.IsZero() - ? TimeDelta::PlusInfinity() - : send_rate_debt_ / max_feedback_rate_, + now + std::clamp(send_rate_debt_ / kMaxFeedbackRate, min_time_between_feedback_.Get(), max_time_between_feedback_.Get()); } diff --git a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h index ad93d8f1f7..a39f1daaef 100644 --- a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h +++ b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h @@ -31,9 +31,17 @@ namespace webrtc { // incoming media packets. Feedback format will comply with RFC 8888. // https://datatracker.ietf.org/doc/rfc8888/ -// Feedback should not use more than 5% of the configured send bandwidth -// estimate. Min and max duration between feedback is configurable using field -// trials, but per default, min is 25ms and max is 250ms. +// Min and max duration between feedback is configurable using field +// trials, but per default, min is 25ms and max is 500ms. +// +// RTCP should not use more than 5% of the uplink link capacity. +// However, there is no good way for a feedback sender to know the +// link capacity unless media is sent in both directions. So we just assume that +// the link capacity is 10 Mbit/s or more and allow sending 500 kbit/s of +// feedback packets. This allows an approximate receive rate of 200 +// Mbit/s with feedback every 25ms. (200 Mbit/s with average size of 800 bytes = +// 31250 packets/s => 40 feedback packets/s with feedback of 780 packets each) + // If possible, given the other constraints, feedback will be sent when a packet // with marker bit is received in order to provide feedback as soon as possible // after receiving a complete video frame. If no packet with marker bit is @@ -50,7 +58,7 @@ class CongestionControlFeedbackGenerator void OnReceivedPacket(const RtpPacketReceived& packet) override; - void OnSendBandwidthEstimateChanged(DataRate estimate) override; + void OnSendBandwidthEstimateChanged(DataRate estimate) override {} TimeDelta Process(Timestamp now) override; @@ -70,7 +78,6 @@ class CongestionControlFeedbackGenerator FieldTrialParameter max_time_to_wait_for_packet_with_marker_; FieldTrialParameter max_time_between_feedback_; - DataRate max_feedback_rate_ = DataRate::KilobitsPerSec(1000); DataSize packet_overhead_ = DataSize::Zero(); DataSize send_rate_debt_ = DataSize::Zero(); diff --git a/modules/remote_bitrate_estimator/congestion_control_feedback_generator_unittest.cc b/modules/remote_bitrate_estimator/congestion_control_feedback_generator_unittest.cc index b1b592d797..014ccc3e6f 100644 --- a/modules/remote_bitrate_estimator/congestion_control_feedback_generator_unittest.cc +++ b/modules/remote_bitrate_estimator/congestion_control_feedback_generator_unittest.cc @@ -26,9 +26,9 @@ #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/buffer.h" +#include "rtc_base/logging.h" #include "rtc_base/network/ecn_marking.h" #include "system_wrappers/include/clock.h" -#include "test/explicit_key_value_config.h" #include "test/gmock.h" #include "test/gtest.h" @@ -133,122 +133,86 @@ TEST(CongestionControlFeedbackGeneratorTest, } TEST(CongestionControlFeedbackGeneratorTest, - FeedbackUtilizeMax5PercentOfConfiguredBwe) { + FeedbackFor30KPacketsUtilizeLessThan500kbitPerSecond) { MockFunction>)> rtcp_sender; SimulatedClock clock(123456); - constexpr TimeDelta kSmallTimeInterval = TimeDelta::Millis(2); CongestionControlFeedbackGenerator generator(CreateEnvironment(&clock), rtcp_sender.AsStdFunction()); - const DataRate kSendBandwidthEstimate = DataRate::BytesPerSec(10'000); - generator.OnSendBandwidthEstimateChanged(kSendBandwidthEstimate); - TimeDelta time_to_next_process = generator.Process(clock.CurrentTime()); - clock.AdvanceTime(kSmallTimeInterval); - time_to_next_process -= kSmallTimeInterval; - - // Two packets with marker bit is received within a short duration. - // Expect the first feedback to be sent immidately and the second to be - // delayed. Delay depend on send bandwith estimate. - Timestamp expected_feedback_time = clock.CurrentTime(); + int number_of_feedback_packets = 0; + DataSize total_feedback_size; EXPECT_CALL(rtcp_sender, Call) - .Times(2) .WillRepeatedly( [&](std::vector> rtcp_packets) { - EXPECT_EQ(clock.CurrentTime(), expected_feedback_time); ASSERT_THAT(rtcp_packets, SizeIs(1)); - rtcp::CongestionControlFeedback* rtcp = - static_cast( - rtcp_packets[0].get()); - int rtcp_len = rtcp->BlockLength(); - // Expect at most 5% of send bandwidth to be used. This decide the - // time to next feedback. - expected_feedback_time += - DataSize::Bytes(rtcp_len) / (0.05 * kSendBandwidthEstimate); + number_of_feedback_packets++; + total_feedback_size += + DataSize::Bytes(rtcp_packets[0]->BlockLength()); }); - generator.OnReceivedPacket( - CreatePacket(clock.CurrentTime(), /*marker=*/true)); - clock.AdvanceTime(kSmallTimeInterval); - time_to_next_process -= kSmallTimeInterval; - generator.OnReceivedPacket( - CreatePacket(clock.CurrentTime(), /*marker=*/true)); + Timestamp start_time = clock.CurrentTime(); + Timestamp last_process_time = clock.CurrentTime(); + TimeDelta time_to_next_process = generator.Process(clock.CurrentTime()); + uint16_t rtp_sequence_number = 0; + // Receive 30 packet per ms in 1s => 30'0000 packets. + while (clock.CurrentTime() < start_time + TimeDelta::Seconds(1)) { + for (int i = 0; i < 30; ++i) { + generator.OnReceivedPacket(CreatePacket(clock.CurrentTime(), + /*marker=*/true, /*ssrc=*/1234, + rtp_sequence_number++)); + } + if (clock.CurrentTime() >= last_process_time + time_to_next_process) { + last_process_time = clock.CurrentTime(); + time_to_next_process = generator.Process(clock.CurrentTime()); + } + clock.AdvanceTime(TimeDelta::Millis(1)); + } - clock.AdvanceTime(time_to_next_process); - time_to_next_process = generator.Process(clock.CurrentTime()); - clock.AdvanceTime(time_to_next_process); - time_to_next_process = generator.Process(clock.CurrentTime()); + EXPECT_LE(total_feedback_size / TimeDelta::Seconds(1), + DataRate::KilobitsPerSec(500)); + EXPECT_EQ(number_of_feedback_packets, 40); } TEST(CongestionControlFeedbackGeneratorTest, - SendsFeedbackAfterMax250MsIfBweVeryLow) { - test::ExplicitKeyValueConfig field_trials( - "WebRTC-RFC8888CongestionControlFeedback/max_send_delta:250ms/"); + FeedbackFor60KPacketsUtilizeApproximately500kbitPerSecond) { MockFunction>)> rtcp_sender; SimulatedClock clock(123456); - constexpr TimeDelta kSmallTimeInterval = TimeDelta::Millis(2); - CongestionControlFeedbackGenerator generator( - CreateEnvironment(&clock, &field_trials), rtcp_sender.AsStdFunction()); - // Regardless of BWE, feedback is sent at least every 250ms. - generator.OnSendBandwidthEstimateChanged(DataRate::BytesPerSec(100)); + CongestionControlFeedbackGenerator generator(CreateEnvironment(&clock), + rtcp_sender.AsStdFunction()); + + int number_of_feedback_packets = 0; + DataSize total_feedback_size; + DataSize last_feedback_size; + EXPECT_CALL(rtcp_sender, Call) + .WillRepeatedly( + [&](std::vector> rtcp_packets) { + ASSERT_THAT(rtcp_packets, SizeIs(1)); + number_of_feedback_packets++; + last_feedback_size = + DataSize::Bytes(rtcp_packets[0]->BlockLength()); + total_feedback_size += last_feedback_size; + }); + Timestamp start_time = clock.CurrentTime(); + Timestamp last_process_time = clock.CurrentTime(); TimeDelta time_to_next_process = generator.Process(clock.CurrentTime()); - clock.AdvanceTime(kSmallTimeInterval); - time_to_next_process -= kSmallTimeInterval; - - Timestamp expected_feedback_time = clock.CurrentTime(); - EXPECT_CALL(rtcp_sender, Call).Times(2).WillRepeatedly(WithoutArgs([&] { - EXPECT_EQ(clock.CurrentTime(), expected_feedback_time); - // Next feedback is not expected to be sent until 250ms after the - // previouse due to low send bandwidth. - expected_feedback_time += TimeDelta::Millis(250); - })); - generator.OnReceivedPacket( - CreatePacket(clock.CurrentTime(), /*marker=*/true)); - clock.AdvanceTime(kSmallTimeInterval); - time_to_next_process -= kSmallTimeInterval; - generator.OnReceivedPacket( - CreatePacket(clock.CurrentTime(), /*marker=*/true)); - - clock.AdvanceTime(time_to_next_process); - time_to_next_process = generator.Process(clock.CurrentTime()); - clock.AdvanceTime(time_to_next_process); - time_to_next_process = generator.Process(clock.CurrentTime()); -} - -TEST(CongestionControlFeedbackGeneratorTest, - SendsFeedbackAfterMax250MsIfBweZero) { - test::ExplicitKeyValueConfig field_trials( - "WebRTC-RFC8888CongestionControlFeedback/max_send_delta:250ms/"); - MockFunction>)> - rtcp_sender; - SimulatedClock clock(123456); - constexpr TimeDelta kSmallTimeInterval = TimeDelta::Millis(2); - CongestionControlFeedbackGenerator generator( - CreateEnvironment(&clock, &field_trials), rtcp_sender.AsStdFunction()); - // Regardless of BWE, feedback is sent at least every 250ms. - generator.OnSendBandwidthEstimateChanged(DataRate::Zero()); - TimeDelta time_to_next_process = generator.Process(clock.CurrentTime()); - clock.AdvanceTime(kSmallTimeInterval); - time_to_next_process -= kSmallTimeInterval; - - Timestamp expected_feedback_time = clock.CurrentTime(); - EXPECT_CALL(rtcp_sender, Call).Times(2).WillRepeatedly(WithoutArgs([&] { - EXPECT_EQ(clock.CurrentTime(), expected_feedback_time); - // Next feedback is not expected to be sent until 250ms after the - // previouse due to low send bandwidth. - expected_feedback_time += TimeDelta::Millis(250); - })); - generator.OnReceivedPacket( - CreatePacket(clock.CurrentTime(), /*marker=*/true)); - clock.AdvanceTime(kSmallTimeInterval); - time_to_next_process -= kSmallTimeInterval; - generator.OnReceivedPacket( - CreatePacket(clock.CurrentTime(), /*marker=*/true)); - - clock.AdvanceTime(time_to_next_process); - time_to_next_process = generator.Process(clock.CurrentTime()); - clock.AdvanceTime(time_to_next_process); - time_to_next_process = generator.Process(clock.CurrentTime()); + uint16_t rtp_sequence_number = 0; + // Receive 60 packet per ms in 1s => 60'0000 packets. + while (clock.CurrentTime() < start_time + TimeDelta::Seconds(1)) { + for (int i = 0; i < 60; ++i) { + generator.OnReceivedPacket(CreatePacket(clock.CurrentTime(), + /*marker=*/true, /*ssrc=*/1234, + rtp_sequence_number++)); + } + if (clock.CurrentTime() >= last_process_time + time_to_next_process) { + last_process_time = clock.CurrentTime(); + time_to_next_process = generator.Process(clock.CurrentTime()); + } + clock.AdvanceTime(TimeDelta::Millis(1)); + } + EXPECT_LE(total_feedback_size, + DataSize::Bytes(500'000 / 8) + last_feedback_size); + EXPECT_LT(number_of_feedback_packets, 40); } TEST(CongestionControlFeedbackGeneratorTest, diff --git a/modules/remote_bitrate_estimator/congestion_control_feedback_tracker.cc b/modules/remote_bitrate_estimator/congestion_control_feedback_tracker.cc index ba56f07009..033b77411f 100644 --- a/modules/remote_bitrate_estimator/congestion_control_feedback_tracker.cc +++ b/modules/remote_bitrate_estimator/congestion_control_feedback_tracker.cc @@ -26,8 +26,16 @@ namespace webrtc { +constexpr int kMaxPacketsPerSsrc = 16384; + void CongestionControlFeedbackTracker::ReceivedPacket( const RtpPacketReceived& packet) { + if (packets_.size() > kMaxPacketsPerSsrc) { + RTC_LOG(LS_VERBOSE) + << "Unexpected number of packets without sending reports:" + << packets_.size(); + return; + } int64_t unwrapped_sequence_number = unwrapper_.Unwrap(packet.SequenceNumber()); if (last_sequence_number_in_feedback_ && @@ -39,9 +47,9 @@ void CongestionControlFeedbackTracker::ReceivedPacket( << static_cast(*last_sequence_number_in_feedback_); // TODO: bugs.webrtc.org/374550342 - According to spec, the old packets // should be reported again. But at the moment, we dont store history of - // packet we already reported and thus, they will be reported as lost. Note - // that this is likely not a problem in webrtc since the packets will also - // be removed from the send history when they are first reported as + // packet we already reported and thus, they will be reported as lost. + // Note that this is likely not a problem in webrtc since the packets will + // also be removed from the send history when they are first reported as // received. last_sequence_number_in_feedback_ = unwrapped_sequence_number - 1; } @@ -69,7 +77,9 @@ void CongestionControlFeedbackTracker::AddPacketsToFeedback( auto packet_it = packets_.begin(); uint32_t ssrc = packet_it->ssrc; for (int64_t sequence_number = *last_sequence_number_in_feedback_ + 1; - sequence_number <= packets_.back().unwrapped_sequence_number; + sequence_number <= packets_.back().unwrapped_sequence_number && + sequence_number <= + *last_sequence_number_in_feedback_ + kMaxPacketsPerSsrc; ++sequence_number) { RTC_DCHECK(packet_it != packets_.end()); RTC_DCHECK_EQ(ssrc, packet_it->ssrc);