From 920abcc9bccb2009b64fb59edeedcf17faf7689b Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 26 Jul 2023 12:33:06 +0200 Subject: [PATCH] In RtpSenderVideo::UpdateConditionalRetransmit use typed time and framerate instead of plain ints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:13757 Change-Id: If2df5418dacd2b95387fa74a9bc226426b207aee Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/313041 Reviewed-by: Åsa Persson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40483} --- modules/rtp_rtcp/BUILD.gn | 3 +- modules/rtp_rtcp/source/rtp_sender_video.cc | 35 ++++++++++--------- modules/rtp_rtcp/source/rtp_sender_video.h | 18 ++++------ .../source/rtp_sender_video_unittest.cc | 6 ++-- 4 files changed, 29 insertions(+), 33 deletions(-) diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 96c43bded3..4a627e15a1 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -288,6 +288,7 @@ rtc_library("rtp_rtcp") { "../../api/transport/rtp:dependency_descriptor", "../../api/transport/rtp:rtp_source", "../../api/units:data_rate", + "../../api/units:frequency", "../../api/units:time_delta", "../../api/units:timestamp", "../../api/video:encoded_frame", @@ -313,6 +314,7 @@ rtc_library("rtp_rtcp") { "../../rtc_base:copy_on_write_buffer", "../../rtc_base:divide_round", "../../rtc_base:event_tracer", + "../../rtc_base:frequency_tracker", "../../rtc_base:gtest_prod", "../../rtc_base:logging", "../../rtc_base:macromagic", @@ -321,7 +323,6 @@ rtc_library("rtp_rtcp") { "../../rtc_base:race_checker", "../../rtc_base:random", "../../rtc_base:rate_limiter", - "../../rtc_base:rate_statistics", "../../rtc_base:rtc_numerics", "../../rtc_base:safe_conversions", "../../rtc_base:safe_minmax", diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 41aff1869a..f29c5ead0f 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -24,6 +24,9 @@ #include "absl/strings/match.h" #include "api/crypto/frame_encryptor_interface.h" #include "api/transport/rtp/dependency_descriptor.h" +#include "api/units/frequency.h" +#include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/absolute_capture_time_sender.h" @@ -45,7 +48,8 @@ namespace webrtc { namespace { constexpr size_t kRedForFecHeaderLength = 1; -constexpr int64_t kMaxUnretransmittableFrameIntervalMs = 33 * 4; +constexpr TimeDelta kMaxUnretransmittableFrameInterval = + TimeDelta::Millis(33 * 4); void BuildRedPayload(const RtpPacketToSend& media_packet, RtpPacketToSend* red_packet) { @@ -781,8 +785,7 @@ bool RTPSenderVideo::AllowRetransmission( MutexLock lock(&stats_mutex_); // Media packet storage. if ((retransmission_settings & kConditionallyRetransmitHigherLayers) && - UpdateConditionalRetransmit(temporal_id, - expected_retransmission_time.ms())) { + UpdateConditionalRetransmit(temporal_id, expected_retransmission_time)) { retransmission_settings |= kRetransmitHigherLayers; } @@ -815,39 +818,37 @@ uint8_t RTPSenderVideo::GetTemporalId(const RTPVideoHeader& header) { bool RTPSenderVideo::UpdateConditionalRetransmit( uint8_t temporal_id, - int64_t expected_retransmission_time_ms) { - int64_t now_ms = clock_->TimeInMilliseconds(); + TimeDelta expected_retransmission_time) { + Timestamp now = clock_->CurrentTime(); // Update stats for any temporal layer. TemporalLayerStats* current_layer_stats = &frame_stats_by_temporal_layer_[temporal_id]; - current_layer_stats->frame_rate_fp1000s.Update(1, now_ms); - int64_t tl_frame_interval = now_ms - current_layer_stats->last_frame_time_ms; - current_layer_stats->last_frame_time_ms = now_ms; + current_layer_stats->frame_rate.Update(now); + TimeDelta tl_frame_interval = now - current_layer_stats->last_frame_time; + current_layer_stats->last_frame_time = now; // Conditional retransmit only applies to upper layers. if (temporal_id != kNoTemporalIdx && temporal_id > 0) { - if (tl_frame_interval >= kMaxUnretransmittableFrameIntervalMs) { + if (tl_frame_interval >= kMaxUnretransmittableFrameInterval) { // Too long since a retransmittable frame in this layer, enable NACK // protection. return true; } else { // Estimate when the next frame of any lower layer will be sent. - const int64_t kUndefined = std::numeric_limits::max(); - int64_t expected_next_frame_time = kUndefined; + Timestamp expected_next_frame_time = Timestamp::PlusInfinity(); for (int i = temporal_id - 1; i >= 0; --i) { TemporalLayerStats* stats = &frame_stats_by_temporal_layer_[i]; - absl::optional rate = stats->frame_rate_fp1000s.Rate(now_ms); - if (rate) { - int64_t tl_next = stats->last_frame_time_ms + 1000000 / *rate; - if (tl_next - now_ms > -expected_retransmission_time_ms && + absl::optional rate = stats->frame_rate.Rate(now); + if (rate > Frequency::Zero()) { + Timestamp tl_next = stats->last_frame_time + 1 / *rate; + if (tl_next - now > -expected_retransmission_time && tl_next < expected_next_frame_time) { expected_next_frame_time = tl_next; } } } - if (expected_next_frame_time == kUndefined || - expected_next_frame_time - now_ms > expected_retransmission_time_ms) { + if (expected_next_frame_time - now > expected_retransmission_time) { // The next frame in a lower layer is expected at a later time (or // unable to tell due to lack of data) than a retransmission is // estimated to be able to arrive, so allow this packet to be nacked. diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index a9edc2d712..acf4d94e29 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -24,6 +24,8 @@ #include "api/task_queue/task_queue_base.h" #include "api/task_queue/task_queue_factory.h" #include "api/transport/rtp/dependency_descriptor.h" +#include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "api/video/video_codec_type.h" #include "api/video/video_frame_type.h" #include "api/video/video_layers_allocation.h" @@ -36,9 +38,9 @@ #include "modules/rtp_rtcp/source/rtp_video_header.h" #include "modules/rtp_rtcp/source/video_fec_generator.h" #include "rtc_base/bitrate_tracker.h" +#include "rtc_base/frequency_tracker.h" #include "rtc_base/one_time_event.h" #include "rtc_base/race_checker.h" -#include "rtc_base/rate_statistics.h" #include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" @@ -62,7 +64,7 @@ enum RetransmissionMode : uint8_t { class RTPSenderVideo : public RTPVideoFrameSenderInterface { public: - static constexpr int64_t kTLRateWindowSizeMs = 2500; + static constexpr TimeDelta kTLRateWindowSize = TimeDelta::Millis(2'500); struct Config { Config() = default; @@ -154,14 +156,8 @@ class RTPSenderVideo : public RTPVideoFrameSenderInterface { private: struct TemporalLayerStats { - TemporalLayerStats() - : frame_rate_fp1000s(kTLRateWindowSizeMs, 1000 * 1000), - last_frame_time_ms(0) {} - // Frame rate, in frames per 1000 seconds. This essentially turns the fps - // value into a fixed point value with three decimals. Improves precision at - // low frame rates. - RateStatistics frame_rate_fp1000s; - int64_t last_frame_time_ms; + FrequencyTracker frame_rate{kTLRateWindowSize}; + Timestamp last_frame_time = Timestamp::Zero(); }; enum class SendVideoLayersAllocation { @@ -189,7 +185,7 @@ class RTPSenderVideo : public RTPVideoFrameSenderInterface { bool red_enabled() const { return red_payload_type_.has_value(); } bool UpdateConditionalRetransmit(uint8_t temporal_id, - int64_t expected_retransmission_time_ms) + TimeDelta expected_retransmission_time) RTC_EXCLUSIVE_LOCKS_REQUIRED(stats_mutex_); void MaybeUpdateCurrentPlayoutDelay(const RTPVideoHeader& header) diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index 8844b620f1..7ce96e0a12 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -414,8 +414,7 @@ TEST_F(RtpSenderVideoTest, ConditionalRetransmit) { // Fill averaging window to prevent rounding errors. constexpr int kNumRepetitions = - (RTPSenderVideo::kTLRateWindowSizeMs + (kFrameInterval.ms() / 2)) / - kFrameInterval.ms(); + RTPSenderVideo::kTLRateWindowSize / kFrameInterval; constexpr int kPattern[] = {0, 2, 1, 2}; auto& vp8_header = header.video_type_header.emplace(); for (size_t i = 0; i < arraysize(kPattern) * kNumRepetitions; ++i) { @@ -466,8 +465,7 @@ TEST_F(RtpSenderVideoTest, ConditionalRetransmitLimit) { // Fill averaging window to prevent rounding errors. constexpr int kNumRepetitions = - (RTPSenderVideo::kTLRateWindowSizeMs + (kFrameInterval.ms() / 2)) / - kFrameInterval.ms(); + RTPSenderVideo::kTLRateWindowSize / kFrameInterval; constexpr int kPattern[] = {0, 2, 2, 2}; auto& vp8_header = header.video_type_header.emplace(); for (size_t i = 0; i < arraysize(kPattern) * kNumRepetitions; ++i) {