From 24e704f14818c744098fd22fe648133c9da33c23 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 10 Aug 2023 19:20:44 +0200 Subject: [PATCH] Cleanup calculating time between RTCP reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move that calculation into dedicated function, move comment why it is calculated the way it is into the same function. Cleanup that comment - remove parts unused by current code, in particular remove description of code that was deleted a while ago Use more strict types for the calculation to make it clearer. Replace DCHECK result can't be zero with a clamp to ensure it can't be zero, because with large bitrates it may. Reland of https://webrtc-review.googlesource.com/c/src/+/315143 Bug: None Change-Id: I41ce383a2f19d489e4cae0b1bf1f720e0ffdd17a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/315460 Reviewed-by: Åsa Persson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40538} --- modules/rtp_rtcp/source/rtcp_sender.cc | 129 ++++++++-------------- modules/rtp_rtcp/source/rtcp_sender.h | 8 +- modules/rtp_rtcp/source/rtp_rtcp_config.h | 3 - modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 3 +- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 4 +- 5 files changed, 54 insertions(+), 93 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index e057005b16..9ca092c0cf 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -20,6 +20,7 @@ #include "absl/types/optional.h" #include "api/rtc_event_log/rtc_event_log.h" #include "api/rtp_headers.h" +#include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "logging/rtc_event_log/events/rtc_event_rtcp_packet_outgoing.h" @@ -91,7 +92,7 @@ class RTCPSender::PacketSender { RTCPSender::FeedbackState::FeedbackState() : packets_sent(0), media_bytes_sent(0), - send_bitrate(0), + send_bitrate(DataRate::Zero()), remote_sr(0), receiver(nullptr) {} @@ -360,65 +361,7 @@ int32_t RTCPSender::SetCNAME(absl::string_view c_name) { return 0; } -bool RTCPSender::TimeToSendRTCPReport(bool sendKeyframeBeforeRTP) const { - /* - For audio we use a configurable interval (default: 5 seconds) - - For video we use a configurable interval (default: 1 second) for a BW - smaller than 360 kbit/s, technicaly we break the max 5% RTCP BW for - video below 10 kbit/s but that should be extremely rare - - - From RFC 3550 - - MAX RTCP BW is 5% if the session BW - A send report is approximately 65 bytes inc CNAME - A receiver report is approximately 28 bytes - - The RECOMMENDED value for the reduced minimum in seconds is 360 - divided by the session bandwidth in kilobits/second. This minimum - is smaller than 5 seconds for bandwidths greater than 72 kb/s. - - If the participant has not yet sent an RTCP packet (the variable - initial is true), the constant Tmin is set to half of the configured - interval. - - The interval between RTCP packets is varied randomly over the - range [0.5,1.5] times the calculated interval to avoid unintended - synchronization of all participants - - if we send - If the participant is a sender (we_sent true), the constant C is - set to the average RTCP packet size (avg_rtcp_size) divided by 25% - of the RTCP bandwidth (rtcp_bw), and the constant n is set to the - number of senders. - - if we receive only - If we_sent is not true, the constant C is set - to the average RTCP packet size divided by 75% of the RTCP - bandwidth. The constant n is set to the number of receivers - (members - senders). If the number of senders is greater than - 25%, senders and receivers are treated together. - - reconsideration NOT required for peer-to-peer - "timer reconsideration" is - employed. This algorithm implements a simple back-off mechanism - which causes users to hold back RTCP packet transmission if the - group sizes are increasing. - - n = number of members - C = avg_size/(rtcpBW/4) - - 3. The deterministic calculated interval Td is set to max(Tmin, n*C). - - 4. The calculated interval T is set to a number uniformly distributed - between 0.5 and 1.5 times the deterministic calculated interval. - - 5. The resulting value of T is divided by e-3/2=1.21828 to compensate - for the fact that the timer reconsideration algorithm converges to - a value of the RTCP bandwidth below the intended average - */ - +bool RTCPSender::TimeToSendRTCPReport(bool send_keyframe_before_rtp) const { Timestamp now = clock_->CurrentTime(); MutexLock lock(&mutex_rtcp_sender_); @@ -428,10 +371,10 @@ bool RTCPSender::TimeToSendRTCPReport(bool sendKeyframeBeforeRTP) const { if (method_ == RtcpMode::kOff) return false; - if (!audio_ && sendKeyframeBeforeRTP) { - // for video key-frames we want to send the RTCP before the large key-frame + if (!audio_ && send_keyframe_before_rtp) { + // For video key-frames we want to send the RTCP before the large key-frame // if we have a 100 ms margin - now += RTCP_SEND_BEFORE_KEY_FRAME; + now += TimeDelta::Millis(100); } return now >= *next_time_to_send_rtcp_; @@ -759,6 +702,44 @@ absl::optional RTCPSender::ComputeCompoundRTCPPacket( return absl::nullopt; } +TimeDelta RTCPSender::ComputeTimeUntilNextReport(DataRate send_bitrate) { + /* + For audio we use a configurable interval (default: 5 seconds) + + For video we use a configurable interval (default: 1 second) + for a BW smaller than ~200 kbit/s, technicaly we break the max 5% RTCP + BW for video but that should be extremely rare + + From RFC 3550, https://www.rfc-editor.org/rfc/rfc3550#section-6.2 + + The RECOMMENDED value for the reduced minimum in seconds is 360 + divided by the session bandwidth in kilobits/second. This minimum + is smaller than 5 seconds for bandwidths greater than 72 kb/s. + + The interval between RTCP packets is varied randomly over the + range [0.5,1.5] times the calculated interval to avoid unintended + synchronization of all participants + */ + + TimeDelta min_interval = report_interval_; + + if (!audio_ && sending_ && send_bitrate > DataRate::BitsPerSec(72'000)) { + // Calculate bandwidth for video; 360 / send bandwidth in kbit/s per + // https://www.rfc-editor.org/rfc/rfc3550#section-6.2 recommendation. + min_interval = std::min(TimeDelta::Seconds(360) / send_bitrate.kbps(), + report_interval_); + } + + // The interval between RTCP packets is varied randomly over the + // range [1/2,3/2] times the calculated interval. + int min_interval_int = rtc::dchecked_cast(min_interval.ms()); + TimeDelta time_to_next = TimeDelta::Millis( + random_.Rand(min_interval_int * 1 / 2, min_interval_int * 3 / 2)); + + // To be safer clamp the result. + return std::max(time_to_next, TimeDelta::Millis(1)); +} + void RTCPSender::PrepareReport(const FeedbackState& feedback_state) { bool generate_report; if (IsFlagPresent(kRtcpSr) || IsFlagPresent(kRtcpRr)) { @@ -783,26 +764,8 @@ void RTCPSender::PrepareReport(const FeedbackState& feedback_state) { SetFlag(kRtcpAnyExtendedReports, true); } - // generate next time to send an RTCP report - TimeDelta min_interval = report_interval_; - - if (!audio_ && sending_) { - // Calculate bandwidth for video; 360 / send bandwidth in kbit/s. - int send_bitrate_kbit = feedback_state.send_bitrate / 1000; - if (send_bitrate_kbit != 0) { - min_interval = std::min(TimeDelta::Millis(360000 / send_bitrate_kbit), - report_interval_); - } - } - - // The interval between RTCP packets is varied randomly over the - // range [1/2,3/2] times the calculated interval. - int min_interval_int = rtc::dchecked_cast(min_interval.ms()); - TimeDelta time_to_next = TimeDelta::Millis( - random_.Rand(min_interval_int * 1 / 2, min_interval_int * 3 / 2)); - - RTC_DCHECK(!time_to_next.IsZero()); - SetNextRtcpSendEvaluationDuration(time_to_next); + SetNextRtcpSendEvaluationDuration( + ComputeTimeUntilNextReport(feedback_state.send_bitrate)); // RtcpSender expected to be used for sending either just sender reports // or just receiver reports. diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 6dae9dbd61..0ceec9a64a 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -20,6 +20,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "api/call/transport.h" +#include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "api/video/video_bitrate_allocation.h" @@ -92,7 +93,7 @@ class RTCPSender final { uint32_t packets_sent; size_t media_bytes_sent; - uint32_t send_bitrate; + DataRate send_bitrate; uint32_t remote_sr; NtpTime last_rr; @@ -141,7 +142,7 @@ class RTCPSender final { int32_t SetCNAME(absl::string_view cName) RTC_LOCKS_EXCLUDED(mutex_rtcp_sender_); - bool TimeToSendRTCPReport(bool sendKeyframeBeforeRTP = false) const + bool TimeToSendRTCPReport(bool send_keyframe_before_rtp = false) const RTC_LOCKS_EXCLUDED(mutex_rtcp_sender_); int32_t SendRTCP(const FeedbackState& feedback_state, @@ -192,6 +193,9 @@ class RTCPSender final { const uint16_t* nack_list, PacketSender& sender) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_rtcp_sender_); + TimeDelta ComputeTimeUntilNextReport(DataRate send_bitrate) + RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_rtcp_sender_); + // Determine which RTCP messages should be sent and setup flags. void PrepareReport(const FeedbackState& feedback_state) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_rtcp_sender_); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_config.h b/modules/rtp_rtcp/source/rtp_rtcp_config.h index 3e6aa3baae..0b87d6d065 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_config.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_config.h @@ -11,14 +11,11 @@ #ifndef MODULES_RTP_RTCP_SOURCE_RTP_RTCP_CONFIG_H_ #define MODULES_RTP_RTCP_SOURCE_RTP_RTCP_CONFIG_H_ -#include "api/units/time_delta.h" - // Configuration file for RTP utilities (RTPSender, RTPReceiver ...) namespace webrtc { constexpr int kDefaultMaxReorderingThreshold = 50; // In sequence numbers. constexpr int kRtcpMaxNackFields = 253; -constexpr TimeDelta RTCP_SEND_BEFORE_KEY_FRAME = TimeDelta::Millis(100); constexpr int RTCP_MAX_REPORT_BLOCKS = 31; // RFC 3550 page 37 } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 5a7624f42b..3f9e093ff0 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -278,8 +278,7 @@ RTCPSender::FeedbackState ModuleRtpRtcpImpl::GetFeedbackState() { rtp_stats.transmitted.packets + rtx_stats.transmitted.packets; state.media_bytes_sent = rtp_stats.transmitted.payload_bytes + rtx_stats.transmitted.payload_bytes; - state.send_bitrate = - rtp_sender_->packet_sender.GetSendRates().Sum().bps(); + state.send_bitrate = rtp_sender_->packet_sender.GetSendRates().Sum(); } state.receiver = &rtcp_receiver_; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index 62c5ad38f5..800ec77d3e 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -260,9 +260,7 @@ RTCPSender::FeedbackState ModuleRtpRtcpImpl2::GetFeedbackState() { state.media_bytes_sent = rtp_stats.transmitted.payload_bytes + rtx_stats.transmitted.payload_bytes; state.send_bitrate = - rtp_sender_->packet_sender.GetSendRates(clock_->CurrentTime()) - .Sum() - .bps(); + rtp_sender_->packet_sender.GetSendRates(clock_->CurrentTime()).Sum(); } state.receiver = &rtcp_receiver_;