From 1e75df26e3a29b3bed19e6a28981eaccaa2b6b77 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Tue, 19 Jan 2021 13:30:23 +0100 Subject: [PATCH] Remove lock from UlpfecReceiverImpl and replace with a sequence checker. Also making some more state const. Instances of this class are currently constructed and used on the "worker thread" but as part of the work for bug webrtc:11993, the instances will be moved over to the network thread. Since the class as is does not require synchronization, that is a good property to make explicit now and then make sure we maintain it in the transition. Bug: webrtc:11993 Change-Id: Id587a746ce0a4363b9e9871ae1749549f8ef3e24 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/202681 Commit-Queue: Tommi Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#33035} --- modules/rtp_rtcp/source/ulpfec_receiver_impl.cc | 15 +++++++-------- modules/rtp_rtcp/source/ulpfec_receiver_impl.h | 16 +++++++++------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index fee0b9c4da..16b87ba727 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -37,12 +37,13 @@ UlpfecReceiverImpl::UlpfecReceiverImpl( fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {} UlpfecReceiverImpl::~UlpfecReceiverImpl() { + RTC_DCHECK_RUN_ON(&sequence_checker_); received_packets_.clear(); fec_->ResetState(&recovered_packets_); } FecPacketCounter UlpfecReceiverImpl::GetPacketCounter() const { - MutexLock lock(&mutex_); + RTC_DCHECK_RUN_ON(&sequence_checker_); return packet_counter_; } @@ -77,6 +78,10 @@ FecPacketCounter UlpfecReceiverImpl::GetPacketCounter() const { bool UlpfecReceiverImpl::AddReceivedRedPacket( const RtpPacketReceived& rtp_packet, uint8_t ulpfec_payload_type) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + // TODO(bugs.webrtc.org/11993): We get here via Call::DeliverRtp, so should be + // moved to the network thread. + if (rtp_packet.Ssrc() != ssrc_) { RTC_LOG(LS_WARNING) << "Received RED packet with different SSRC than expected; dropping."; @@ -87,7 +92,6 @@ bool UlpfecReceiverImpl::AddReceivedRedPacket( "packet size; dropping."; return false; } - MutexLock lock(&mutex_); static constexpr uint8_t kRedHeaderLength = 1; @@ -151,7 +155,7 @@ bool UlpfecReceiverImpl::AddReceivedRedPacket( // TODO(nisse): Drop always-zero return value. int32_t UlpfecReceiverImpl::ProcessReceivedFec() { - mutex_.Lock(); + RTC_DCHECK_RUN_ON(&sequence_checker_); // If we iterate over |received_packets_| and it contains a packet that cause // us to recurse back to this function (for example a RED packet encapsulating @@ -168,10 +172,8 @@ int32_t UlpfecReceiverImpl::ProcessReceivedFec() { // Send received media packet to VCM. if (!received_packet->is_fec) { ForwardErrorCorrection::Packet* packet = received_packet->pkt; - mutex_.Unlock(); recovered_packet_callback_->OnRecoveredPacket(packet->data.data(), packet->data.size()); - mutex_.Lock(); // Create a packet with the buffer to modify it. RtpPacketReceived rtp_packet; const uint8_t* const original_data = packet->data.cdata(); @@ -208,13 +210,10 @@ int32_t UlpfecReceiverImpl::ProcessReceivedFec() { // Set this flag first; in case the recovered packet carries a RED // header, OnRecoveredPacket will recurse back here. recovered_packet->returned = true; - mutex_.Unlock(); recovered_packet_callback_->OnRecoveredPacket(packet->data.data(), packet->data.size()); - mutex_.Lock(); } - mutex_.Unlock(); return 0; } diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h index 2bed042747..fc7fe387e7 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h @@ -22,7 +22,8 @@ #include "modules/rtp_rtcp/include/ulpfec_receiver.h" #include "modules/rtp_rtcp/source/forward_error_correction.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" -#include "rtc_base/synchronization/mutex.h" +#include "rtc_base/synchronization/sequence_checker.h" +#include "rtc_base/system/no_unique_address.h" namespace webrtc { @@ -44,17 +45,18 @@ class UlpfecReceiverImpl : public UlpfecReceiver { const uint32_t ssrc_; const RtpHeaderExtensionMap extensions_; - mutable Mutex mutex_; - RecoveredPacketReceiver* recovered_packet_callback_; - std::unique_ptr fec_; + RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_; + RecoveredPacketReceiver* const recovered_packet_callback_; + const std::unique_ptr fec_; // TODO(nisse): The AddReceivedRedPacket method adds one or two packets to // this list at a time, after which it is emptied by ProcessReceivedFec. It // will make things simpler to merge AddReceivedRedPacket and // ProcessReceivedFec into a single method, and we can then delete this list. std::vector> - received_packets_; - ForwardErrorCorrection::RecoveredPacketList recovered_packets_; - FecPacketCounter packet_counter_; + received_packets_ RTC_GUARDED_BY(&sequence_checker_); + ForwardErrorCorrection::RecoveredPacketList recovered_packets_ + RTC_GUARDED_BY(&sequence_checker_); + FecPacketCounter packet_counter_ RTC_GUARDED_BY(&sequence_checker_); }; } // namespace webrtc