From 9a09ed73c58672d93c8967064d270fb54076000b Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 28 Jul 2023 15:28:15 +0200 Subject: [PATCH] Cleanup RemoteBitrateEstimatorSingleStream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Store Detectos in a map by value instead of by unccessary pointer Bug: None Change-Id: Iab9904aafca02d9f9ae6633c87de860a5bd62ac7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/313621 Auto-Submit: Danil Chapovalov Reviewed-by: Björn Terelius Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40499} --- .../remote_bitrate_estimator_single_stream.cc | 95 ++++++------------- .../remote_bitrate_estimator_single_stream.h | 16 +++- 2 files changed, 42 insertions(+), 69 deletions(-) diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index 077315fb3e..070b2fc11b 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -28,6 +28,10 @@ namespace webrtc { namespace { + +constexpr int kTimestampGroupLengthMs = 5; +constexpr double kTimestampToMs = 1.0 / 90.0; + absl::optional OptionalRateFromOptionalBps( absl::optional bitrate_bps) { if (bitrate_bps) { @@ -38,19 +42,8 @@ absl::optional OptionalRateFromOptionalBps( } } // namespace -enum { kTimestampGroupLengthMs = 5 }; -static const double kTimestampToMs = 1.0 / 90.0; - -struct RemoteBitrateEstimatorSingleStream::Detector { - explicit Detector(int64_t last_packet_time_ms) - : last_packet_time_ms(last_packet_time_ms), - inter_arrival(90 * kTimestampGroupLengthMs, kTimestampToMs) {} - - int64_t last_packet_time_ms; - InterArrival inter_arrival; - OveruseEstimator estimator; - OveruseDetector detector; -}; +RemoteBitrateEstimatorSingleStream::Detector::Detector() + : inter_arrival(90 * kTimestampGroupLengthMs, kTimestampToMs) {} RemoteBitrateEstimatorSingleStream::RemoteBitrateEstimatorSingleStream( RemoteBitrateObserver* observer, @@ -66,13 +59,8 @@ RemoteBitrateEstimatorSingleStream::RemoteBitrateEstimatorSingleStream( RTC_LOG(LS_INFO) << "RemoteBitrateEstimatorSingleStream: Instantiating."; } -RemoteBitrateEstimatorSingleStream::~RemoteBitrateEstimatorSingleStream() { - while (!overuse_detectors_.empty()) { - SsrcOveruseEstimatorMap::iterator it = overuse_detectors_.begin(); - delete it->second; - overuse_detectors_.erase(it); - } -} +RemoteBitrateEstimatorSingleStream::~RemoteBitrateEstimatorSingleStream() = + default; void RemoteBitrateEstimatorSingleStream::IncomingPacket( const RtpPacketReceived& rtp_packet) { @@ -89,20 +77,8 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( uint32_t rtp_timestamp = rtp_packet.Timestamp() + transmission_time_offset.value_or(0); int64_t now_ms = clock_->TimeInMilliseconds(); - SsrcOveruseEstimatorMap::iterator it = overuse_detectors_.find(ssrc); - if (it == overuse_detectors_.end()) { - // This is a new SSRC. Adding to map. - // TODO(holmer): If the channel changes SSRC the old SSRC will still be - // around in this map until the channel is deleted. This is OK since the - // callback will no longer be called for the old SSRC. This will be - // automatically cleaned up when we have one RemoteBitrateEstimator per REMB - // group. - std::pair insert_result = - overuse_detectors_.insert(std::make_pair(ssrc, new Detector(now_ms))); - it = insert_result.first; - } - Detector* estimator = it->second; - estimator->last_packet_time_ms = now_ms; + Detector& estimator = overuse_detectors_[ssrc]; + estimator.last_packet_time_ms = now_ms; // Check if incoming bitrate estimate is valid, and if it needs to be reset. absl::optional incoming_bitrate = incoming_bitrate_.Rate(now_ms); @@ -118,21 +94,20 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( size_t payload_size = rtp_packet.payload_size() + rtp_packet.padding_size(); incoming_bitrate_.Update(payload_size, now_ms); - const BandwidthUsage prior_state = estimator->detector.State(); + const BandwidthUsage prior_state = estimator.detector.State(); uint32_t timestamp_delta = 0; int64_t time_delta = 0; int size_delta = 0; - if (estimator->inter_arrival.ComputeDeltas( + if (estimator.inter_arrival.ComputeDeltas( rtp_timestamp, rtp_packet.arrival_time().ms(), now_ms, payload_size, ×tamp_delta, &time_delta, &size_delta)) { double timestamp_delta_ms = timestamp_delta * kTimestampToMs; - estimator->estimator.Update(time_delta, timestamp_delta_ms, size_delta, - estimator->detector.State(), now_ms); - estimator->detector.Detect(estimator->estimator.offset(), - timestamp_delta_ms, - estimator->estimator.num_of_deltas(), now_ms); + estimator.estimator.Update(time_delta, timestamp_delta_ms, size_delta, + estimator.detector.State(), now_ms); + estimator.detector.Detect(estimator.estimator.offset(), timestamp_delta_ms, + estimator.estimator.num_of_deltas(), now_ms); } - if (estimator->detector.State() == BandwidthUsage::kBwOverusing) { + if (estimator.detector.State() == BandwidthUsage::kBwOverusing) { absl::optional incoming_bitrate_bps = incoming_bitrate_.Rate(now_ms); if (incoming_bitrate_bps && @@ -162,21 +137,19 @@ TimeDelta RemoteBitrateEstimatorSingleStream::Process() { void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t now_ms) { BandwidthUsage bw_state = BandwidthUsage::kBwNormal; - SsrcOveruseEstimatorMap::iterator it = overuse_detectors_.begin(); + auto it = overuse_detectors_.begin(); while (it != overuse_detectors_.end()) { - const int64_t time_of_last_received_packet = - it->second->last_packet_time_ms; + const int64_t time_of_last_received_packet = it->second.last_packet_time_ms; if (time_of_last_received_packet >= 0 && now_ms - time_of_last_received_packet > kStreamTimeOutMs) { // This over-use detector hasn't received packets for `kStreamTimeOutMs` // milliseconds and is considered stale. - delete it->second; overuse_detectors_.erase(it++); } else { // Make sure that we trigger an over-use if any of the over-use detectors // is detecting over-use. - if (it->second->detector.State() > bw_state) { - bw_state = it->second->detector.State(); + if (it->second.detector.State() > bw_state) { + bw_state = it->second.detector.State(); } ++it; } @@ -193,10 +166,8 @@ void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t now_ms) { if (remote_rate_.ValidEstimate()) { process_interval_ms_ = remote_rate_.GetFeedbackInterval().ms(); RTC_DCHECK_GT(process_interval_ms_, 0); - std::vector ssrcs; - GetSsrcs(&ssrcs); if (observer_) - observer_->OnReceiveBitrateChanged(ssrcs, target_bitrate); + observer_->OnReceiveBitrateChanged(GetSsrcs(), target_bitrate); } } @@ -205,12 +176,8 @@ void RemoteBitrateEstimatorSingleStream::OnRttUpdate(int64_t avg_rtt_ms, remote_rate_.SetRtt(TimeDelta::Millis(avg_rtt_ms)); } -void RemoteBitrateEstimatorSingleStream::RemoveStream(unsigned int ssrc) { - SsrcOveruseEstimatorMap::iterator it = overuse_detectors_.find(ssrc); - if (it != overuse_detectors_.end()) { - delete it->second; - overuse_detectors_.erase(it); - } +void RemoteBitrateEstimatorSingleStream::RemoveStream(uint32_t ssrc) { + overuse_detectors_.erase(ssrc); } DataRate RemoteBitrateEstimatorSingleStream::LatestEstimate() const { @@ -220,15 +187,13 @@ DataRate RemoteBitrateEstimatorSingleStream::LatestEstimate() const { return remote_rate_.LatestEstimate(); } -void RemoteBitrateEstimatorSingleStream::GetSsrcs( - std::vector* ssrcs) const { - RTC_DCHECK(ssrcs); - ssrcs->resize(overuse_detectors_.size()); - int i = 0; - for (SsrcOveruseEstimatorMap::const_iterator it = overuse_detectors_.begin(); - it != overuse_detectors_.end(); ++it, ++i) { - (*ssrcs)[i] = it->first; +std::vector RemoteBitrateEstimatorSingleStream::GetSsrcs() const { + std::vector ssrcs; + ssrcs.reserve(overuse_detectors_.size()); + for (const auto& [ssrc, unused] : overuse_detectors_) { + ssrcs.push_back(ssrc); } + return ssrcs; } } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h index 05ec03d5a1..d0ca675cb3 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h @@ -23,6 +23,9 @@ #include "api/units/timestamp.h" #include "modules/remote_bitrate_estimator/aimd_rate_control.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" +#include "modules/remote_bitrate_estimator/inter_arrival.h" +#include "modules/remote_bitrate_estimator/overuse_detector.h" +#include "modules/remote_bitrate_estimator/overuse_estimator.h" #include "rtc_base/rate_statistics.h" namespace webrtc { @@ -50,18 +53,23 @@ class RemoteBitrateEstimatorSingleStream : public RemoteBitrateEstimator { DataRate LatestEstimate() const override; private: - struct Detector; + struct Detector { + Detector(); - typedef std::map SsrcOveruseEstimatorMap; + int64_t last_packet_time_ms; + InterArrival inter_arrival; + OveruseEstimator estimator; + OveruseDetector detector; + }; // Triggers a new estimate calculation. void UpdateEstimate(int64_t time_now); - void GetSsrcs(std::vector* ssrcs) const; + std::vector GetSsrcs() const; Clock* const clock_; const FieldTrialBasedConfig field_trials_; - SsrcOveruseEstimatorMap overuse_detectors_; + std::map overuse_detectors_; RateStatistics incoming_bitrate_; uint32_t last_valid_incoming_bitrate_; AimdRateControl remote_rate_;