From 9cbdd6976ab1da896c9f0e60cc4fa15e487e79d2 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Wed, 10 Jun 2020 12:11:35 +0000 Subject: [PATCH] Revert "Migrate modules/pacing to webrtc::Mutex." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 11ae285df916db70158cb9808260ebae1f7db012. Reason for revert: downstream test failed. Original change's description: > Migrate modules/pacing to webrtc::Mutex. > > Bug: webrtc:11567 > Change-Id: I5624d7f2528d584ba92a66e5ae0097ab2e0724d8 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176852 > Reviewed-by: Erik Språng > Commit-Queue: Markus Handell > Cr-Commit-Position: refs/heads/master@{#31484} TBR=sprang@webrtc.org,handellm@webrtc.org Change-Id: If3b31d8b7b7ba94bc6fffe5a441150cd59252078 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:11567 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176854 Reviewed-by: Markus Handell Commit-Queue: Markus Handell Cr-Commit-Position: refs/heads/master@{#31486} --- modules/pacing/BUILD.gn | 1 - modules/pacing/paced_sender.cc | 34 +++++++++--------- modules/pacing/paced_sender.h | 5 ++- modules/pacing/packet_router.cc | 22 ++++++------ modules/pacing/packet_router.h | 44 +++++++++++------------ modules/pacing/task_queue_paced_sender.cc | 4 +-- modules/pacing/task_queue_paced_sender.h | 5 ++- 7 files changed, 55 insertions(+), 60 deletions(-) diff --git a/modules/pacing/BUILD.gn b/modules/pacing/BUILD.gn index b19c304e1f..7e8efb93cb 100644 --- a/modules/pacing/BUILD.gn +++ b/modules/pacing/BUILD.gn @@ -49,7 +49,6 @@ rtc_library("pacing") { "../../rtc_base:rtc_base_approved", "../../rtc_base:rtc_task_queue", "../../rtc_base/experiments:field_trial_parser", - "../../rtc_base/synchronization:mutex", "../../rtc_base/synchronization:sequence_checker", "../../rtc_base/task_utils:to_queued_task", "../../system_wrappers", diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 57dbc4f248..a0e76761e7 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -58,13 +58,13 @@ PacedSender::~PacedSender() { } void PacedSender::CreateProbeCluster(DataRate bitrate, int cluster_id) { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); return pacing_controller_.CreateProbeCluster(bitrate, cluster_id); } void PacedSender::Pause() { { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.Pause(); } @@ -77,7 +77,7 @@ void PacedSender::Pause() { void PacedSender::Resume() { { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.Resume(); } @@ -90,7 +90,7 @@ void PacedSender::Resume() { void PacedSender::SetCongestionWindow(DataSize congestion_window_size) { { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.SetCongestionWindow(congestion_window_size); } MaybeWakupProcessThread(); @@ -98,7 +98,7 @@ void PacedSender::SetCongestionWindow(DataSize congestion_window_size) { void PacedSender::UpdateOutstandingData(DataSize outstanding_data) { { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.UpdateOutstandingData(outstanding_data); } MaybeWakupProcessThread(); @@ -106,7 +106,7 @@ void PacedSender::UpdateOutstandingData(DataSize outstanding_data) { void PacedSender::SetPacingRates(DataRate pacing_rate, DataRate padding_rate) { { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.SetPacingRates(pacing_rate, padding_rate); } MaybeWakupProcessThread(); @@ -117,7 +117,7 @@ void PacedSender::EnqueuePackets( { TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("webrtc"), "PacedSender::EnqueuePackets"); - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); for (auto& packet : packets) { TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("webrtc"), "PacedSender::EnqueuePackets::Loop", "sequence_number", @@ -131,42 +131,42 @@ void PacedSender::EnqueuePackets( } void PacedSender::SetAccountForAudioPackets(bool account_for_audio) { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.SetAccountForAudioPackets(account_for_audio); } void PacedSender::SetIncludeOverhead() { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.SetIncludeOverhead(); } void PacedSender::SetTransportOverhead(DataSize overhead_per_packet) { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.SetTransportOverhead(overhead_per_packet); } TimeDelta PacedSender::ExpectedQueueTime() const { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); return pacing_controller_.ExpectedQueueTime(); } DataSize PacedSender::QueueSizeData() const { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); return pacing_controller_.QueueSizeData(); } absl::optional PacedSender::FirstSentPacketTime() const { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); return pacing_controller_.FirstSentPacketTime(); } TimeDelta PacedSender::OldestPacketWaitTime() const { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); return pacing_controller_.OldestPacketWaitTime(); } int64_t PacedSender::TimeUntilNextProcess() { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); Timestamp next_send_time = pacing_controller_.NextSendTime(); TimeDelta sleep_time = @@ -178,7 +178,7 @@ int64_t PacedSender::TimeUntilNextProcess() { } void PacedSender::Process() { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.ProcessPackets(); } @@ -198,7 +198,7 @@ void PacedSender::MaybeWakupProcessThread() { void PacedSender::SetQueueTimeLimit(TimeDelta limit) { { - MutexLock lock(&mutex_); + rtc::CritScope cs(&critsect_); pacing_controller_.SetQueueTimeLimit(limit); } MaybeWakupProcessThread(); diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index fb4309509d..cc83b403ba 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -33,7 +33,6 @@ #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "modules/utility/include/process_thread.h" #include "rtc_base/critical_section.h" -#include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -158,9 +157,9 @@ class PacedSender : public Module, PacedSender* const delegate_; } module_proxy_{this}; - mutable Mutex mutex_; + rtc::CriticalSection critsect_; const PacingController::ProcessMode process_mode_; - PacingController pacing_controller_ RTC_GUARDED_BY(mutex_); + PacingController pacing_controller_ RTC_GUARDED_BY(critsect_); Clock* const clock_; ProcessThread* const process_thread_; diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index e75b5a337a..c4ea5df541 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -55,7 +55,7 @@ PacketRouter::~PacketRouter() { void PacketRouter::AddSendRtpModule(RtpRtcpInterface* rtp_module, bool remb_candidate) { - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); AddSendRtpModuleToMap(rtp_module, rtp_module->SSRC()); if (absl::optional rtx_ssrc = rtp_module->RtxSsrc()) { @@ -97,7 +97,7 @@ void PacketRouter::RemoveSendRtpModuleFromMap(uint32_t ssrc) { } void PacketRouter::RemoveSendRtpModule(RtpRtcpInterface* rtp_module) { - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); MaybeRemoveRembModuleCandidate(rtp_module, /* media_sender = */ true); RemoveSendRtpModuleFromMap(rtp_module->SSRC()); @@ -115,7 +115,7 @@ void PacketRouter::RemoveSendRtpModule(RtpRtcpInterface* rtp_module) { void PacketRouter::AddReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender, bool remb_candidate) { - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); RTC_DCHECK(std::find(rtcp_feedback_senders_.begin(), rtcp_feedback_senders_.end(), rtcp_sender) == rtcp_feedback_senders_.end()); @@ -129,7 +129,7 @@ void PacketRouter::AddReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender, void PacketRouter::RemoveReceiveRtpModule( RtcpFeedbackSenderInterface* rtcp_sender) { - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); MaybeRemoveRembModuleCandidate(rtcp_sender, /* media_sender = */ false); auto it = std::find(rtcp_feedback_senders_.begin(), rtcp_feedback_senders_.end(), rtcp_sender); @@ -143,7 +143,7 @@ void PacketRouter::SendPacket(std::unique_ptr packet, "sequence_number", packet->SequenceNumber(), "rtp_timestamp", packet->Timestamp()); - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); // With the new pacer code path, transport sequence numbers are only set here, // on the pacer thread. Therefore we don't need atomics/synchronization. if (packet->HasExtension()) { @@ -178,7 +178,7 @@ std::vector> PacketRouter::GeneratePadding( TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("webrtc"), "PacketRouter::GeneratePadding", "bytes", size.bytes()); - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); // First try on the last rtp module to have sent media. This increases the // the chance that any payload based padding will be useful as it will be // somewhat distributed over modules according the packet rate, even if it @@ -219,7 +219,7 @@ std::vector> PacketRouter::GeneratePadding( } uint16_t PacketRouter::CurrentTransportSequenceNumber() const { - MutexLock lock(&modules_mutex_); + rtc::CritScope lock(&modules_crit_); return transport_seq_ & 0xFFFF; } @@ -233,7 +233,7 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, int64_t now_ms = rtc::TimeMillis(); { - MutexLock lock(&remb_mutex_); + rtc::CritScope lock(&remb_crit_); // If we already have an estimate, check if the new total estimate is below // kSendThresholdPercent of the previous estimate. @@ -266,7 +266,7 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, void PacketRouter::SetMaxDesiredReceiveBitrate(int64_t bitrate_bps) { RTC_DCHECK_GE(bitrate_bps, 0); { - MutexLock lock(&remb_mutex_); + rtc::CritScope lock(&remb_crit_); max_bitrate_bps_ = bitrate_bps; if (rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs && last_send_bitrate_bps_ > 0 && @@ -280,7 +280,7 @@ void PacketRouter::SetMaxDesiredReceiveBitrate(int64_t bitrate_bps) { bool PacketRouter::SendRemb(int64_t bitrate_bps, const std::vector& ssrcs) { - MutexLock lock(&modules_mutex_); + rtc::CritScope lock(&modules_crit_); if (!active_remb_module_) { return false; @@ -295,7 +295,7 @@ bool PacketRouter::SendRemb(int64_t bitrate_bps, bool PacketRouter::SendCombinedRtcpPacket( std::vector> packets) { - MutexLock lock(&modules_mutex_); + rtc::CritScope cs(&modules_crit_); // Prefer send modules. for (RtpRtcpInterface* rtp_module : send_modules_list_) { diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 73837f2ffe..4c716ddfe8 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -28,7 +28,6 @@ #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/constructor_magic.h" #include "rtc_base/critical_section.h" -#include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -84,49 +83,48 @@ class PacketRouter : public RemoteBitrateObserver, private: void AddRembModuleCandidate(RtcpFeedbackSenderInterface* candidate_module, bool media_sender) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); + RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); void MaybeRemoveRembModuleCandidate( RtcpFeedbackSenderInterface* candidate_module, - bool media_sender) RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); - void UnsetActiveRembModule() RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); - void DetermineActiveRembModule() RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); + bool media_sender) RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + void UnsetActiveRembModule() RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + void DetermineActiveRembModule() RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); void AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); + RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); void RemoveSendRtpModuleFromMap(uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); + RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); - mutable Mutex modules_mutex_; + rtc::CriticalSection modules_crit_; // Ssrc to RtpRtcpInterface module; std::unordered_map send_modules_map_ - RTC_GUARDED_BY(modules_mutex_); - std::list send_modules_list_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(modules_crit_); + std::list send_modules_list_ RTC_GUARDED_BY(modules_crit_); // The last module used to send media. - RtpRtcpInterface* last_send_module_ RTC_GUARDED_BY(modules_mutex_); + RtpRtcpInterface* last_send_module_ RTC_GUARDED_BY(modules_crit_); // Rtcp modules of the rtp receivers. std::vector rtcp_feedback_senders_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(modules_crit_); - // TODO(eladalon): remb_mutex_ only ever held from one function, and it's not + // TODO(eladalon): remb_crit_ only ever held from one function, and it's not // clear if that function can actually be called from more than one thread. - Mutex remb_mutex_; + rtc::CriticalSection remb_crit_; // The last time a REMB was sent. - int64_t last_remb_time_ms_ RTC_GUARDED_BY(remb_mutex_); - int64_t last_send_bitrate_bps_ RTC_GUARDED_BY(remb_mutex_); + int64_t last_remb_time_ms_ RTC_GUARDED_BY(remb_crit_); + int64_t last_send_bitrate_bps_ RTC_GUARDED_BY(remb_crit_); // The last bitrate update. - int64_t bitrate_bps_ RTC_GUARDED_BY(remb_mutex_); - int64_t max_bitrate_bps_ RTC_GUARDED_BY(remb_mutex_); + int64_t bitrate_bps_ RTC_GUARDED_BY(remb_crit_); + int64_t max_bitrate_bps_ RTC_GUARDED_BY(remb_crit_); // Candidates for the REMB module can be RTP sender/receiver modules, with // the sender modules taking precedence. std::vector sender_remb_candidates_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(modules_crit_); std::vector receiver_remb_candidates_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(modules_crit_); RtcpFeedbackSenderInterface* active_remb_module_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(modules_crit_); - uint64_t transport_seq_ RTC_GUARDED_BY(modules_mutex_); + uint64_t transport_seq_ RTC_GUARDED_BY(modules_crit_); RTC_DISALLOW_COPY_AND_ASSIGN(PacketRouter); }; diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc index db748f30b4..fccc1a6e64 100644 --- a/modules/pacing/task_queue_paced_sender.cc +++ b/modules/pacing/task_queue_paced_sender.cc @@ -188,7 +188,7 @@ TimeDelta TaskQueuePacedSender::OldestPacketWaitTime() const { } void TaskQueuePacedSender::OnStatsUpdated(const Stats& stats) { - MutexLock lock(&stats_mutex_); + rtc::CritScope cs(&stats_crit_); current_stats_ = stats; } @@ -299,7 +299,7 @@ void TaskQueuePacedSender::MaybeUpdateStats(bool is_scheduled_call) { } TaskQueuePacedSender::Stats TaskQueuePacedSender::GetStats() const { - MutexLock lock(&stats_mutex_); + rtc::CritScope cs(&stats_crit_); return current_stats_; } diff --git a/modules/pacing/task_queue_paced_sender.h b/modules/pacing/task_queue_paced_sender.h index 9787b8beee..c4ee5466e7 100644 --- a/modules/pacing/task_queue_paced_sender.h +++ b/modules/pacing/task_queue_paced_sender.h @@ -30,7 +30,6 @@ #include "modules/pacing/rtp_packet_pacer.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/critical_section.h" -#include "rtc_base/synchronization/mutex.h" #include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/task_queue.h" #include "rtc_base/thread_annotations.h" @@ -156,8 +155,8 @@ class TaskQueuePacedSender : public RtpPacketPacer, public RtpPacketSender { // never drain. bool is_shutdown_ RTC_GUARDED_BY(task_queue_); - mutable Mutex stats_mutex_; - Stats current_stats_ RTC_GUARDED_BY(stats_mutex_); + rtc::CriticalSection stats_crit_; + Stats current_stats_ RTC_GUARDED_BY(stats_crit_); rtc::TaskQueue task_queue_; };