From 1b77daea81865dc2c87ec19f8af5287b115daba2 Mon Sep 17 00:00:00 2001 From: Per K Date: Wed, 12 Apr 2023 15:40:04 +0200 Subject: [PATCH] Replace lock with SequenceChecker in PacketRouter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since PacketRouter now is only used on the worker thread, there is no need for a lock. Bug: webrtc:14502 Change-Id: I65778f68b7e211d7bc7388a4615888a49ceb5f59 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300964 Reviewed-by: Erik Språng Commit-Queue: Per Kjellander Reviewed-by: Markus Handell Cr-Commit-Position: refs/heads/main@{#39848} --- modules/pacing/packet_router.cc | 32 +++++++++++++++------------ modules/pacing/packet_router.h | 39 ++++++++++++++------------------- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index b28d9776dc..85490ef82d 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -19,12 +19,10 @@ #include "absl/types/optional.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" -#include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "modules/rtp_rtcp/source/rtp_rtcp_interface.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/system/unused.h" -#include "rtc_base/time_utils.h" #include "rtc_base/trace_event.h" namespace webrtc { @@ -37,6 +35,7 @@ PacketRouter::PacketRouter(uint16_t start_transport_seq) transport_seq_(start_transport_seq) {} PacketRouter::~PacketRouter() { + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(send_modules_map_.empty()); RTC_DCHECK(send_modules_list_.empty()); RTC_DCHECK(rtcp_feedback_senders_.empty()); @@ -47,7 +46,7 @@ PacketRouter::~PacketRouter() { void PacketRouter::AddSendRtpModule(RtpRtcpInterface* rtp_module, bool remb_candidate) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); AddSendRtpModuleToMap(rtp_module, rtp_module->SSRC()); if (absl::optional rtx_ssrc = rtp_module->RtxSsrc()) { @@ -68,6 +67,7 @@ void PacketRouter::AddSendRtpModule(RtpRtcpInterface* rtp_module, void PacketRouter::AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, uint32_t ssrc) { + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(send_modules_map_.find(ssrc) == send_modules_map_.end()); // Signal to module that the pacer thread is attached and can send packets. @@ -86,6 +86,7 @@ void PacketRouter::AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, } void PacketRouter::RemoveSendRtpModuleFromMap(uint32_t ssrc) { + RTC_DCHECK_RUN_ON(&thread_checker_); auto it = send_modules_map_.find(ssrc); RTC_DCHECK(it != send_modules_map_.end()); send_modules_list_.remove(it->second); @@ -93,7 +94,7 @@ void PacketRouter::RemoveSendRtpModuleFromMap(uint32_t ssrc) { } void PacketRouter::RemoveSendRtpModule(RtpRtcpInterface* rtp_module) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); MaybeRemoveRembModuleCandidate(rtp_module, /* media_sender = */ true); RemoveSendRtpModuleFromMap(rtp_module->SSRC()); @@ -112,7 +113,7 @@ void PacketRouter::RemoveSendRtpModule(RtpRtcpInterface* rtp_module) { void PacketRouter::AddReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender, bool remb_candidate) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(std::find(rtcp_feedback_senders_.begin(), rtcp_feedback_senders_.end(), rtcp_sender) == rtcp_feedback_senders_.end()); @@ -126,7 +127,7 @@ void PacketRouter::AddReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender, void PacketRouter::RemoveReceiveRtpModule( RtcpFeedbackSenderInterface* rtcp_sender) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); MaybeRemoveRembModuleCandidate(rtcp_sender, /* media_sender = */ false); auto it = std::find(rtcp_feedback_senders_.begin(), rtcp_feedback_senders_.end(), rtcp_sender); @@ -136,11 +137,11 @@ void PacketRouter::RemoveReceiveRtpModule( void PacketRouter::SendPacket(std::unique_ptr packet, const PacedPacketInfo& cluster_info) { + RTC_DCHECK_RUN_ON(&thread_checker_); TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("webrtc"), "PacketRouter::SendPacket", "sequence_number", packet->SequenceNumber(), "rtp_timestamp", packet->Timestamp()); - MutexLock lock(&modules_mutex_); // With the new pacer code path, transport sequence numbers are only set here, // on the pacer thread. Therefore we don't need atomics/synchronization. bool assign_transport_sequence_number = @@ -184,7 +185,7 @@ void PacketRouter::SendPacket(std::unique_ptr packet, } std::vector> PacketRouter::FetchFec() { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); std::vector> fec_packets = std::move(pending_fec_packets_); pending_fec_packets_.clear(); @@ -193,10 +194,10 @@ std::vector> PacketRouter::FetchFec() { std::vector> PacketRouter::GeneratePadding( DataSize size) { + RTC_DCHECK_RUN_ON(&thread_checker_); TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("webrtc"), "PacketRouter::GeneratePadding", "bytes", size.bytes()); - MutexLock lock(&modules_mutex_); // 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 @@ -238,7 +239,7 @@ std::vector> PacketRouter::GeneratePadding( void PacketRouter::OnAbortedRetransmissions( uint32_t ssrc, rtc::ArrayView sequence_numbers) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); auto it = send_modules_map_.find(ssrc); if (it != send_modules_map_.end()) { it->second->OnAbortedRetransmissions(sequence_numbers); @@ -246,7 +247,7 @@ void PacketRouter::OnAbortedRetransmissions( } absl::optional PacketRouter::GetRtxSsrcForMedia(uint32_t ssrc) const { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); auto it = send_modules_map_.find(ssrc); if (it != send_modules_map_.end() && it->second->SSRC() == ssrc) { // A module is registered with the given SSRC, and that SSRC is the main @@ -257,12 +258,12 @@ absl::optional PacketRouter::GetRtxSsrcForMedia(uint32_t ssrc) const { } uint16_t PacketRouter::CurrentTransportSequenceNumber() const { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); return transport_seq_ & 0xFFFF; } void PacketRouter::SendRemb(int64_t bitrate_bps, std::vector ssrcs) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); if (!active_remb_module_) { return; @@ -275,7 +276,7 @@ void PacketRouter::SendRemb(int64_t bitrate_bps, std::vector ssrcs) { void PacketRouter::SendCombinedRtcpPacket( std::vector> packets) { - MutexLock lock(&modules_mutex_); + RTC_DCHECK_RUN_ON(&thread_checker_); // Prefer send modules. for (RtpRtcpInterface* rtp_module : send_modules_list_) { @@ -308,6 +309,7 @@ void PacketRouter::AddRembModuleCandidate( void PacketRouter::MaybeRemoveRembModuleCandidate( RtcpFeedbackSenderInterface* candidate_module, bool media_sender) { + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(candidate_module); std::vector& candidates = media_sender ? sender_remb_candidates_ : receiver_remb_candidates_; @@ -325,12 +327,14 @@ void PacketRouter::MaybeRemoveRembModuleCandidate( } void PacketRouter::UnsetActiveRembModule() { + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_CHECK(active_remb_module_); active_remb_module_->UnsetRemb(); active_remb_module_ = nullptr; } void PacketRouter::DetermineActiveRembModule() { + RTC_DCHECK_RUN_ON(&thread_checker_); // Sender modules take precedence over receiver modules, because SRs (sender // reports) are sent more frequently than RR (receiver reports). // When adding the first sender module, we should change the active REMB diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 68b82c6bd4..805f422112 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -20,12 +20,12 @@ #include #include +#include "api/sequence_checker.h" #include "api/transport/network_types.h" #include "modules/pacing/pacing_controller.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" -#include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -74,45 +74,40 @@ class PacketRouter : public PacingController::PacketSender { private: void AddRembModuleCandidate(RtcpFeedbackSenderInterface* candidate_module, - bool media_sender) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); + bool media_sender); 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_); - void AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); - void RemoveSendRtpModuleFromMap(uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_mutex_); + bool media_sender); + void UnsetActiveRembModule(); + void DetermineActiveRembModule(); + void AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, uint32_t ssrc); + void RemoveSendRtpModuleFromMap(uint32_t ssrc); - mutable Mutex modules_mutex_; + SequenceChecker thread_checker_; // Ssrc to RtpRtcpInterface module; std::unordered_map send_modules_map_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(thread_checker_); std::list send_modules_list_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(thread_checker_); // The last module used to send media. - RtpRtcpInterface* last_send_module_ RTC_GUARDED_BY(modules_mutex_); + RtpRtcpInterface* last_send_module_ RTC_GUARDED_BY(thread_checker_); // Rtcp modules of the rtp receivers. std::vector rtcp_feedback_senders_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(thread_checker_); // 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(thread_checker_); std::vector receiver_remb_candidates_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(thread_checker_); RtcpFeedbackSenderInterface* active_remb_module_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(thread_checker_); - uint64_t transport_seq_ RTC_GUARDED_BY(modules_mutex_); + uint64_t transport_seq_ RTC_GUARDED_BY(thread_checker_); - // TODO(bugs.webrtc.org/10809): Replace lock with a sequence checker once the - // process thread is gone. std::vector> pending_fec_packets_ - RTC_GUARDED_BY(modules_mutex_); + RTC_GUARDED_BY(thread_checker_); }; } // namespace webrtc #endif // MODULES_PACING_PACKET_ROUTER_H_