From 198d0d7fd580e1f384b876d8fecc88d0cd92534e Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 21 Mar 2023 13:18:37 +0100 Subject: [PATCH] Remove mutexes from remote bitrate estimators They are called only by ReceivedSideCongestionController that already ensures all access is synchronized. Bug: None Change-Id: I0f87e24e3fbb0bd8f6ff679fb949d2373c554fba Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269300 Reviewed-by: Per Kjellander Auto-Submit: Danil Chapovalov Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#39622} --- modules/remote_bitrate_estimator/BUILD.gn | 19 +-- .../remote_bitrate_estimator_abs_send_time.cc | 137 ++++++++---------- .../remote_bitrate_estimator_abs_send_time.h | 17 +-- .../remote_bitrate_estimator_single_stream.cc | 5 - .../remote_bitrate_estimator_single_stream.h | 20 +-- 5 files changed, 85 insertions(+), 113 deletions(-) diff --git a/modules/remote_bitrate_estimator/BUILD.gn b/modules/remote_bitrate_estimator/BUILD.gn index 0f1e5b3b3f..9d8f9bbc29 100644 --- a/modules/remote_bitrate_estimator/BUILD.gn +++ b/modules/remote_bitrate_estimator/BUILD.gn @@ -32,14 +32,6 @@ rtc_library("remote_bitrate_estimator") { "test/bwe_test_logging.h", ] - if (rtc_enable_bwe_test_logging) { - defines = [ "BWE_TEST_LOGGING_COMPILE_TIME_ENABLE=1" ] - - sources += [ "test/bwe_test_logging.cc" ] - } else { - defines = [ "BWE_TEST_LOGGING_COMPILE_TIME_ENABLE=0" ] - } - deps = [ "../../api:field_trials_view", "../../api:network_state_predictor_api", @@ -56,9 +48,6 @@ rtc_library("remote_bitrate_estimator") { "../../modules/rtp_rtcp:rtp_rtcp_format", "../../rtc_base:checks", "../../rtc_base:logging", - "../../rtc_base:macromagic", - "../../rtc_base:platform_thread", - "../../rtc_base:race_checker", "../../rtc_base:rate_statistics", "../../rtc_base:rtc_numerics", "../../rtc_base:safe_minmax", @@ -73,6 +62,14 @@ rtc_library("remote_bitrate_estimator") { "//third_party/abseil-cpp/absl/strings", "//third_party/abseil-cpp/absl/types:optional", ] + + if (rtc_enable_bwe_test_logging) { + defines = [ "BWE_TEST_LOGGING_COMPILE_TIME_ENABLE=1" ] + sources += [ "test/bwe_test_logging.cc" ] + deps += [ "../../rtc_base:platform_thread" ] + } else { + defines = [ "BWE_TEST_LOGGING_COMPILE_TIME_ENABLE=0" ] + } } if (!build_with_chromium) { diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index e9fb1b99f6..17a8315bd4 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -25,7 +25,6 @@ #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" -#include "rtc_base/thread_annotations.h" #include "system_wrappers/include/metrics.h" namespace webrtc { @@ -215,7 +214,6 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacket( int64_t arrival_time_ms, size_t payload_size, const RTPHeader& header) { - RTC_DCHECK_RUNS_SERIALIZED(&network_race_); if (!header.extension.hasAbsoluteSendTime) { RTC_LOG(LS_WARNING) << "RemoteBitrateEstimatorAbsSendTimeImpl: Incoming packet " @@ -271,85 +269,81 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( int size_delta = 0; bool update_estimate = false; DataRate target_bitrate = DataRate::Zero(); - std::vector ssrcs; - { - MutexLock lock(&mutex_); - TimeoutStreams(now); - RTC_DCHECK(inter_arrival_); - RTC_DCHECK(estimator_); - ssrcs_.insert_or_assign(ssrc, now); + TimeoutStreams(now); + RTC_DCHECK(inter_arrival_); + RTC_DCHECK(estimator_); + ssrcs_.insert_or_assign(ssrc, now); - // For now only try to detect probes while we don't have a valid estimate. - // We currently assume that only packets larger than 200 bytes are paced by - // the sender. - static constexpr DataSize kMinProbePacketSize = DataSize::Bytes(200); - if (payload_size > kMinProbePacketSize && - (!remote_rate_.ValidEstimate() || - now - first_packet_time_ < kInitialProbingInterval)) { - // TODO(holmer): Use a map instead to get correct order? - if (total_probes_received_ < kMaxProbePackets) { - TimeDelta send_delta = TimeDelta::Millis(-1); - TimeDelta recv_delta = TimeDelta::Millis(-1); - if (!probes_.empty()) { - send_delta = send_time - probes_.back().send_time; - recv_delta = arrival_time - probes_.back().recv_time; - } - RTC_LOG(LS_INFO) << "Probe packet received: send time=" - << send_time.ms() - << " ms, recv time=" << arrival_time.ms() - << " ms, send delta=" << send_delta.ms() - << " ms, recv delta=" << recv_delta.ms() << " ms."; + // For now only try to detect probes while we don't have a valid estimate. + // We currently assume that only packets larger than 200 bytes are paced by + // the sender. + static constexpr DataSize kMinProbePacketSize = DataSize::Bytes(200); + if (payload_size > kMinProbePacketSize && + (!remote_rate_.ValidEstimate() || + now - first_packet_time_ < kInitialProbingInterval)) { + // TODO(holmer): Use a map instead to get correct order? + if (total_probes_received_ < kMaxProbePackets) { + TimeDelta send_delta = TimeDelta::Millis(-1); + TimeDelta recv_delta = TimeDelta::Millis(-1); + if (!probes_.empty()) { + send_delta = send_time - probes_.back().send_time; + recv_delta = arrival_time - probes_.back().recv_time; } - probes_.emplace_back(send_time, arrival_time, payload_size); - ++total_probes_received_; - // Make sure that a probe which updated the bitrate immediately has an - // effect by calling the OnReceiveBitrateChanged callback. - if (ProcessClusters(now) == ProbeResult::kBitrateUpdated) - update_estimate = true; - } - if (inter_arrival_->ComputeDeltas(timestamp, arrival_time.ms(), now.ms(), - payload_size.bytes(), &ts_delta, &t_delta, - &size_delta)) { - double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift); - estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State(), - arrival_time.ms()); - detector_.Detect(estimator_->offset(), ts_delta_ms, - estimator_->num_of_deltas(), arrival_time.ms()); + RTC_LOG(LS_INFO) << "Probe packet received: send time=" << send_time.ms() + << " ms, recv time=" << arrival_time.ms() + << " ms, send delta=" << send_delta.ms() + << " ms, recv delta=" << recv_delta.ms() << " ms."; } + probes_.emplace_back(send_time, arrival_time, payload_size); + ++total_probes_received_; + // Make sure that a probe which updated the bitrate immediately has an + // effect by calling the OnReceiveBitrateChanged callback. + if (ProcessClusters(now) == ProbeResult::kBitrateUpdated) + update_estimate = true; + } + if (inter_arrival_->ComputeDeltas(timestamp, arrival_time.ms(), now.ms(), + payload_size.bytes(), &ts_delta, &t_delta, + &size_delta)) { + double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift); + estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State(), + arrival_time.ms()); + detector_.Detect(estimator_->offset(), ts_delta_ms, + estimator_->num_of_deltas(), arrival_time.ms()); + } - if (!update_estimate) { - // Check if it's time for a periodic update or if we should update because - // of an over-use. - if (last_update_.IsInfinite() || - now.ms() - last_update_.ms() > - remote_rate_.GetFeedbackInterval().ms()) { + if (!update_estimate) { + // Check if it's time for a periodic update or if we should update because + // of an over-use. + if (last_update_.IsInfinite() || + now.ms() - last_update_.ms() > + remote_rate_.GetFeedbackInterval().ms()) { + update_estimate = true; + } else if (detector_.State() == BandwidthUsage::kBwOverusing) { + absl::optional incoming_rate = + incoming_bitrate_.Rate(arrival_time.ms()); + if (incoming_rate && remote_rate_.TimeToReduceFurther( + now, DataRate::BitsPerSec(*incoming_rate))) { update_estimate = true; - } else if (detector_.State() == BandwidthUsage::kBwOverusing) { - absl::optional incoming_rate = - incoming_bitrate_.Rate(arrival_time.ms()); - if (incoming_rate && remote_rate_.TimeToReduceFurther( - now, DataRate::BitsPerSec(*incoming_rate))) { - update_estimate = true; - } } } - - if (update_estimate) { - // The first overuse should immediately trigger a new estimate. - // We also have to update the estimate immediately if we are overusing - // and the target bitrate is too high compared to what we are receiving. - const RateControlInput input( - detector_.State(), OptionalRateFromOptionalBps( - incoming_bitrate_.Rate(arrival_time.ms()))); - target_bitrate = remote_rate_.Update(&input, now); - update_estimate = remote_rate_.ValidEstimate(); - ssrcs = Keys(ssrcs_); - } } + + if (update_estimate) { + // The first overuse should immediately trigger a new estimate. + // We also have to update the estimate immediately if we are overusing + // and the target bitrate is too high compared to what we are receiving. + const RateControlInput input( + detector_.State(), + OptionalRateFromOptionalBps(incoming_bitrate_.Rate(arrival_time.ms()))); + target_bitrate = remote_rate_.Update(&input, now); + update_estimate = remote_rate_.ValidEstimate(); + } + if (update_estimate) { last_update_ = now; - observer_->OnReceiveBitrateChanged(ssrcs, target_bitrate.bps()); + observer_->OnReceiveBitrateChanged(Keys(ssrcs_), + target_bitrate.bps()); } } @@ -378,18 +372,15 @@ void RemoteBitrateEstimatorAbsSendTime::TimeoutStreams(Timestamp now) { void RemoteBitrateEstimatorAbsSendTime::OnRttUpdate(int64_t avg_rtt_ms, int64_t /*max_rtt_ms*/) { - MutexLock lock(&mutex_); remote_rate_.SetRtt(TimeDelta::Millis(avg_rtt_ms)); } void RemoteBitrateEstimatorAbsSendTime::RemoveStream(uint32_t ssrc) { - MutexLock lock(&mutex_); ssrcs_.erase(ssrc); } DataRate RemoteBitrateEstimatorAbsSendTime::LatestEstimate() const { // Currently accessed only from the worker thread (see Call::GetStats()). - MutexLock lock(&mutex_); if (!remote_rate_.ValidEstimate() || ssrcs_.empty()) { return DataRate::Zero(); } diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h index fd33c84b04..7db2aea67d 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h @@ -31,10 +31,7 @@ #include "modules/remote_bitrate_estimator/overuse_detector.h" #include "modules/remote_bitrate_estimator/overuse_estimator.h" #include "rtc_base/checks.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" #include "system_wrappers/include/clock.h" namespace webrtc { @@ -102,15 +99,12 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { const Cluster* FindBestProbe(const std::list& clusters) const; // Returns true if a probe which changed the estimate was detected. - ProbeResult ProcessClusters(Timestamp now) - RTC_EXCLUSIVE_LOCKS_REQUIRED(&mutex_); + ProbeResult ProcessClusters(Timestamp now); - bool IsBitrateImproving(DataRate probe_bitrate) const - RTC_EXCLUSIVE_LOCKS_REQUIRED(&mutex_); + bool IsBitrateImproving(DataRate probe_bitrate) const; - void TimeoutStreams(Timestamp now) RTC_EXCLUSIVE_LOCKS_REQUIRED(&mutex_); + void TimeoutStreams(Timestamp now); - rtc::RaceChecker network_race_; Clock* const clock_; const FieldTrialBasedConfig field_trials_; RemoteBitrateObserver* const observer_; @@ -125,9 +119,8 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { Timestamp last_update_ = Timestamp::MinusInfinity(); bool uma_recorded_ = false; - mutable Mutex mutex_; - std::map ssrcs_ RTC_GUARDED_BY(&mutex_); - AimdRateControl remote_rate_ RTC_GUARDED_BY(&mutex_); + std::map ssrcs_; + AimdRateControl remote_rate_; }; } // namespace webrtc 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 8f15912a49..049734969a 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -94,7 +94,6 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( uint32_t rtp_timestamp = header.timestamp + header.extension.transmissionTimeOffset; int64_t now_ms = clock_->TimeInMilliseconds(); - MutexLock lock(&mutex_); SsrcOveruseEstimatorMap::iterator it = overuse_detectors_.find(ssrc); if (it == overuse_detectors_.end()) { // This is a new SSRC. Adding to map. @@ -156,7 +155,6 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( } TimeDelta RemoteBitrateEstimatorSingleStream::Process() { - MutexLock lock(&mutex_); int64_t now_ms = clock_->TimeInMilliseconds(); int64_t next_process_time_ms = last_process_time_ + process_interval_ms_; if (last_process_time_ == -1 || now_ms >= next_process_time_ms) { @@ -210,12 +208,10 @@ void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t now_ms) { void RemoteBitrateEstimatorSingleStream::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { - MutexLock lock(&mutex_); remote_rate_.SetRtt(TimeDelta::Millis(avg_rtt_ms)); } void RemoteBitrateEstimatorSingleStream::RemoveStream(unsigned int ssrc) { - MutexLock lock(&mutex_); SsrcOveruseEstimatorMap::iterator it = overuse_detectors_.find(ssrc); if (it != overuse_detectors_.end()) { delete it->second; @@ -224,7 +220,6 @@ void RemoteBitrateEstimatorSingleStream::RemoveStream(unsigned int ssrc) { } DataRate RemoteBitrateEstimatorSingleStream::LatestEstimate() const { - MutexLock lock(&mutex_); if (!remote_rate_.ValidEstimate() || overuse_detectors_.empty()) { return DataRate::Zero(); } 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 699f259d48..5e2961d901 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h @@ -24,8 +24,6 @@ #include "modules/remote_bitrate_estimator/aimd_rate_control.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/rate_statistics.h" -#include "rtc_base/synchronization/mutex.h" -#include "rtc_base/thread_annotations.h" namespace webrtc { @@ -59,21 +57,19 @@ class RemoteBitrateEstimatorSingleStream : public RemoteBitrateEstimator { typedef std::map SsrcOveruseEstimatorMap; // Triggers a new estimate calculation. - void UpdateEstimate(int64_t time_now) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + void UpdateEstimate(int64_t time_now); - void GetSsrcs(std::vector* ssrcs) const - RTC_SHARED_LOCKS_REQUIRED(mutex_); + void GetSsrcs(std::vector* ssrcs) const; Clock* const clock_; const FieldTrialBasedConfig field_trials_; - SsrcOveruseEstimatorMap overuse_detectors_ RTC_GUARDED_BY(mutex_); - RateStatistics incoming_bitrate_ RTC_GUARDED_BY(mutex_); - uint32_t last_valid_incoming_bitrate_ RTC_GUARDED_BY(mutex_); - AimdRateControl remote_rate_ RTC_GUARDED_BY(mutex_); - RemoteBitrateObserver* const observer_ RTC_GUARDED_BY(mutex_); - mutable Mutex mutex_; + SsrcOveruseEstimatorMap overuse_detectors_; + RateStatistics incoming_bitrate_; + uint32_t last_valid_incoming_bitrate_; + AimdRateControl remote_rate_; + RemoteBitrateObserver* const observer_; int64_t last_process_time_; - int64_t process_interval_ms_ RTC_GUARDED_BY(mutex_); + int64_t process_interval_ms_; bool uma_recorded_; };