From bf46cfef2201acdc0c98c00c6e0e484ff4bf42da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 11 May 2020 18:22:02 +0200 Subject: [PATCH] Refactors send rate statistics in RtpSenderEgress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When FEC generation is moved to egress, we'll need to poll bitrates from there instead of the RtpVideoSender. In preparation, refactoring some getter methods. For context, see https://webrtc-review.googlesource.com/c/src/+/173708 Bug: webrtc:11340 Change-Id: Ibc27362361ee9640d9fce676fc8e1093a579344f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174202 Commit-Queue: Erik SprÃ¥ng Reviewed-by: Danil Chapovalov Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#31214} --- call/rtp_video_sender.cc | 8 ++- modules/rtp_rtcp/include/rtp_rtcp.h | 6 ++- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 44 +++++++++++++--- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 1 + modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 16 ++++-- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 3 ++ modules/rtp_rtcp/source/rtp_sender_egress.cc | 55 +++++++++++--------- modules/rtp_rtcp/source/rtp_sender_egress.h | 7 ++- 8 files changed, 94 insertions(+), 46 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index ffe2d61b39..8c31a848aa 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -836,16 +836,14 @@ int RtpVideoSender::ProtectionRequest(const FecProtectionParams* delta_params, *sent_nack_rate_bps = 0; *sent_fec_rate_bps = 0; for (const RtpStreamSender& stream : rtp_streams_) { - uint32_t not_used = 0; - uint32_t module_nack_rate = 0; if (stream.fec_generator) { stream.fec_generator->SetProtectionParameters(*delta_params, *key_params); *sent_fec_rate_bps += stream.fec_generator->CurrentFecRate().bps(); } *sent_video_rate_bps += stream.sender_video->VideoBitrateSent(); - stream.rtp_rtcp->BitrateSent(¬_used, /*video_rate=*/nullptr, - /*fec_rate=*/nullptr, &module_nack_rate); - *sent_nack_rate_bps += module_nack_rate; + *sent_nack_rate_bps += + stream.rtp_rtcp->GetSendRates()[RtpPacketMediaType::kRetransmission] + .bps(); } return 0; } diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 598c09e0d4..f91f0d13a3 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -285,12 +285,16 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // bitrate estimate since the stream participates in the bitrate allocation. virtual void SetAsPartOfAllocation(bool part_of_allocation) = 0; - // Fetches the current send bitrates in bits/s. + // TODO(sprang): Remove when all call sites have been moved to + // GetSendRates(). Fetches the current send bitrates in bits/s. virtual void BitrateSent(uint32_t* total_rate, uint32_t* video_rate, uint32_t* fec_rate, uint32_t* nack_rate) const = 0; + // Returns bitrate sent (post-pacing) per packet type. + virtual RtpSendRates GetSendRates() const = 0; + virtual RTPSender* RtpSender() = 0; virtual const RTPSender* RtpSender() const = 0; diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 48bb842d29..049ff5c506 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -17,6 +17,7 @@ #include #include +#include "absl/algorithm/container.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "absl/types/variant.h" @@ -211,12 +212,15 @@ class RtcpBandwidthObserver { virtual ~RtcpBandwidthObserver() {} }; -enum class RtpPacketMediaType { - kAudio, // Audio media packets. - kVideo, // Video media packets. - kRetransmission, // RTX (usually) packets send as response to NACK. - kForwardErrorCorrection, // FEC packets. - kPadding // RTX or plain padding sent to maintain BWE. +// NOTE! |kNumMediaTypes| must be kept in sync with RtpPacketMediaType! +static constexpr size_t kNumMediaTypes = 5; +enum class RtpPacketMediaType : size_t { + kAudio, // Audio media packets. + kVideo, // Video media packets. + kRetransmission, // Retransmisions, sent as response to NACK. + kForwardErrorCorrection, // FEC packets. + kPadding = kNumMediaTypes - 1, // RTX or plain padding sent to maintain BWE. + // Again, don't forget to udate |kNumMediaTypes| if you add another value! }; struct RtpPacketSendInfo { @@ -382,6 +386,34 @@ struct StreamDataCounters { RtpPacketCounter fec; // Number of redundancy packets/bytes. }; +class RtpSendRates { + template + constexpr std::array make_zero_array( + std::index_sequence) { + return {{(static_cast(Is), DataRate::Zero())...}}; + } + + public: + RtpSendRates() + : send_rates_( + make_zero_array(std::make_index_sequence())) {} + RtpSendRates(const RtpSendRates& rhs) = default; + RtpSendRates& operator=(const RtpSendRates&) = default; + + DataRate& operator[](RtpPacketMediaType type) { + return send_rates_[static_cast(type)]; + } + const DataRate& operator[](RtpPacketMediaType type) const { + return send_rates_[static_cast(type)]; + } + DataRate Sum() const { + return absl::c_accumulate(send_rates_, DataRate::Zero()); + } + + private: + std::array send_rates_; +}; + // Callback, called whenever byte/packet counts have been updated. class StreamDataCountersCallback { public: diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 4ad982021b..5a333fe847 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -95,6 +95,7 @@ class MockRtpRtcp : public RtpRtcp { uint32_t* fec_rate, uint32_t* nack_rate), (const override)); + MOCK_METHOD(RtpSendRates, GetSendRates, (), (const override)); MOCK_METHOD(int, EstimatedReceiveBandwidth, (uint32_t * available_bandwidth), diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 4f84b0247d..fb6f8a3f8f 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -290,7 +290,7 @@ RTCPSender::FeedbackState ModuleRtpRtcpImpl::GetFeedbackState() { state.media_bytes_sent = rtp_stats.transmitted.payload_bytes + rtx_stats.transmitted.payload_bytes; state.send_bitrate = - rtp_sender_->packet_sender.SendBitrate().bps(); + rtp_sender_->packet_sender.GetSendRates().Sum().bps(); } state.module = this; @@ -702,12 +702,17 @@ void ModuleRtpRtcpImpl::BitrateSent(uint32_t* total_rate, uint32_t* video_rate, uint32_t* fec_rate, uint32_t* nack_rate) const { - *total_rate = rtp_sender_->packet_sender.SendBitrate().bps(); + RtpSendRates send_rates = rtp_sender_->packet_sender.GetSendRates(); + *total_rate = send_rates.Sum().bps(); if (video_rate) *video_rate = 0; if (fec_rate) *fec_rate = 0; - *nack_rate = rtp_sender_->packet_sender.NackOverheadRate().bps(); + *nack_rate = send_rates[RtpPacketMediaType::kRetransmission].bps(); +} + +RtpSendRates ModuleRtpRtcpImpl::GetSendRates() const { + return rtp_sender_->packet_sender.GetSendRates(); } void ModuleRtpRtcpImpl::OnRequestSendReport() { @@ -803,12 +808,13 @@ const RTPSender* ModuleRtpRtcpImpl::RtpSender() const { DataRate ModuleRtpRtcpImpl::SendRate() const { RTC_DCHECK(rtp_sender_); - return rtp_sender_->packet_sender.SendBitrate(); + return rtp_sender_->packet_sender.GetSendRates().Sum(); } DataRate ModuleRtpRtcpImpl::NackOverheadRate() const { RTC_DCHECK(rtp_sender_); - return rtp_sender_->packet_sender.NackOverheadRate(); + return rtp_sender_->packet_sender + .GetSendRates()[RtpPacketMediaType::kRetransmission]; } } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 8bda0e0f0c..debb433297 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -264,6 +264,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { uint32_t* fec_rate, uint32_t* nackRate) const override; + RtpSendRates GetSendRates() const override; + void OnReceivedNack( const std::vector& nack_sequence_numbers) override; void OnReceivedRtcpReportBlocks( @@ -294,6 +296,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { Clock* clock() const { return clock_; } + // TODO(sprang): Remove when usage is gone. DataRate SendRate() const; DataRate NackOverheadRate() const; diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc index a64a5bddb6..77803deda9 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -79,9 +79,8 @@ RtpSenderEgress::RtpSenderEgress(const RtpRtcp::Configuration& config, max_delay_it_(send_delays_.end()), sum_delays_ms_(0), total_packet_send_delay_ms_(0), - total_bitrate_sent_(kBitrateStatisticsWindowMs, - RateStatistics::kBpsScale), - nack_bitrate_sent_(kBitrateStatisticsWindowMs, RateStatistics::kBpsScale), + send_rates_(kNumMediaTypes, + {kBitrateStatisticsWindowMs, RateStatistics::kBpsScale}), rtp_sequence_number_map_(need_rtp_packet_infos_ ? std::make_unique( kRtpSequenceNumberMapMaxEntries) @@ -99,16 +98,20 @@ void RtpSenderEgress::SendPacket(RtpPacketToSend* packet, if (is_audio_) { #if BWE_TEST_LOGGING_COMPILE_TIME_ENABLE BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "AudioTotBitrate_kbps", now_ms, - SendBitrate().kbps(), packet_ssrc); - BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "AudioNackBitrate_kbps", now_ms, - NackOverheadRate().kbps(), packet_ssrc); + GetSendRates().Sum().kbps(), packet_ssrc); + BWE_TEST_LOGGING_PLOT_WITH_SSRC( + 1, "AudioNackBitrate_kbps", now_ms, + GetSendRates()[RtpPacketMediaType::kRetransmission].kbps(), + packet_ssrc); #endif } else { #if BWE_TEST_LOGGING_COMPILE_TIME_ENABLE BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "VideoTotBitrate_kbps", now_ms, - SendBitrate().kbps(), packet_ssrc); - BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "VideoNackBitrate_kbps", now_ms, - NackOverheadRate().kbps(), packet_ssrc); + GetSendRates().Sum().kbps(), packet_ssrc); + BWE_TEST_LOGGING_PLOT_WITH_SSRC( + 1, "VideoNackBitrate_kbps", now_ms, + GetSendRates()[RtpPacketMediaType::kRetransmission].kbps(), + packet_ssrc); #endif } @@ -203,21 +206,22 @@ void RtpSenderEgress::ProcessBitrateAndNotifyObservers() { return; rtc::CritScope lock(&lock_); - int64_t now_ms = clock_->TimeInMilliseconds(); - bitrate_callback_->Notify(total_bitrate_sent_.Rate(now_ms).value_or(0), - nack_bitrate_sent_.Rate(now_ms).value_or(0), ssrc_); + RtpSendRates send_rates = GetSendRates(); + bitrate_callback_->Notify( + send_rates.Sum().bps(), + send_rates[RtpPacketMediaType::kRetransmission].bps(), ssrc_); } -DataRate RtpSenderEgress::SendBitrate() const { - rtc::CritScope cs(&lock_); - return DataRate::BitsPerSec( - total_bitrate_sent_.Rate(clock_->TimeInMilliseconds()).value_or(0)); -} - -DataRate RtpSenderEgress::NackOverheadRate() const { - rtc::CritScope cs(&lock_); - return DataRate::BitsPerSec( - nack_bitrate_sent_.Rate(clock_->TimeInMilliseconds()).value_or(0)); +RtpSendRates RtpSenderEgress::GetSendRates() const { + rtc::CritScope lock(&lock_); + const int64_t now_ms = clock_->TimeInMilliseconds(); + RtpSendRates current_rates; + for (size_t i = 0; i < kNumMediaTypes; ++i) { + RtpPacketMediaType type = static_cast(i); + current_rates[type] = + DataRate::BitsPerSec(send_rates_[i].Rate(now_ms).value_or(0)); + } + return current_rates; } void RtpSenderEgress::GetDataCounters(StreamDataCounters* rtp_stats, @@ -432,8 +436,6 @@ void RtpSenderEgress::UpdateRtpStats(const RtpPacketToSend& packet) { StreamDataCounters* counters = packet.Ssrc() == rtx_ssrc_ ? &rtx_rtp_stats_ : &rtp_stats_; - total_bitrate_sent_.Update(packet.size(), now_ms); - if (counters->first_packet_time_ms == -1) { counters->first_packet_time_ms = now_ms; } @@ -444,10 +446,13 @@ void RtpSenderEgress::UpdateRtpStats(const RtpPacketToSend& packet) { if (packet.packet_type() == RtpPacketMediaType::kRetransmission) { counters->retransmitted.AddPacket(packet); - nack_bitrate_sent_.Update(packet.size(), now_ms); } counters->transmitted.AddPacket(packet); + RTC_DCHECK(packet.packet_type().has_value()); + send_rates_[static_cast(*packet.packet_type())].Update(packet.size(), + now_ms); + if (rtp_stats_callback_) { rtp_stats_callback_->DataCountersUpdated(*counters, packet.Ssrc()); } diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.h b/modules/rtp_rtcp/source/rtp_sender_egress.h index 131534039e..298f57eff0 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.h +++ b/modules/rtp_rtcp/source/rtp_sender_egress.h @@ -57,8 +57,7 @@ class RtpSenderEgress { absl::optional FlexFecSsrc() const { return flexfec_ssrc_; } void ProcessBitrateAndNotifyObservers(); - DataRate SendBitrate() const; - DataRate NackOverheadRate() const; + RtpSendRates GetSendRates() const; void GetDataCounters(StreamDataCounters* rtp_stats, StreamDataCounters* rtx_stats) const; @@ -129,8 +128,8 @@ class RtpSenderEgress { uint64_t total_packet_send_delay_ms_ RTC_GUARDED_BY(lock_); StreamDataCounters rtp_stats_ RTC_GUARDED_BY(lock_); StreamDataCounters rtx_rtp_stats_ RTC_GUARDED_BY(lock_); - RateStatistics total_bitrate_sent_ RTC_GUARDED_BY(lock_); - RateStatistics nack_bitrate_sent_ RTC_GUARDED_BY(lock_); + // One element per value in RtpPacketMediaType, with index matching value. + std::vector send_rates_ RTC_GUARDED_BY(lock_); // Maps sent packets' sequence numbers to a tuple consisting of: // 1. The timestamp, without the randomizing offset mandated by the RFC.