From 30e2d6ee00bba1cf4f6e50270747ac226795dd5a Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Tue, 9 Oct 2018 18:27:36 +0200 Subject: [PATCH] Moves locking outside function in RtpSender. This CL moves the action of acquiring the lock outside UpdateTransportSequenceNumber. This prepares for an upcoming CL where the lock is used outside this call at the call sites and avoids the lock-unlock overhead that would otherwise occur. Also removing the const declaration as it modifies the state of transport_sequence_number_allocator_. Bug: webrtc:9796 Change-Id: I0bd4a0fd2fdbf6291867eb913690c61269eab8c5 Reviewed-on: https://webrtc-review.googlesource.com/c/102684 Commit-Queue: Sebastian Jansson Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#25068} --- modules/rtp_rtcp/source/rtp_sender.cc | 29 ++++++++++++++++++++------- modules/rtp_rtcp/source/rtp_sender.h | 4 ++-- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 9f10c40e57..c79f33d089 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -619,10 +619,13 @@ size_t RTPSender::SendPadData(size_t bytes, PacketOptions options; // Padding packets are never retransmissions. options.is_retransmit = false; - bool has_transport_seq_num = - UpdateTransportSequenceNumber(&padding_packet, &options.packet_id); + bool has_transport_seq_num; + { + rtc::CritScope lock(&send_critsect_); + has_transport_seq_num = + UpdateTransportSequenceNumber(&padding_packet, &options.packet_id); + } padding_packet.SetPadding(padding_bytes_in_packet, &random_); - if (has_transport_seq_num) { AddPacketToTransportFeedback(options.packet_id, padding_packet, pacing_info); @@ -838,7 +841,13 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr packet, // E.g. RTPSender::TrySendRedundantPayloads calls PrepareAndSendPacket with // send_over_rtx = true but is_retransmit = false. options.is_retransmit = is_retransmit || send_over_rtx; - if (UpdateTransportSequenceNumber(packet_to_send, &options.packet_id)) { + bool has_transport_seq_num; + { + rtc::CritScope lock(&send_critsect_); + has_transport_seq_num = + UpdateTransportSequenceNumber(packet_to_send, &options.packet_id); + } + if (has_transport_seq_num) { AddPacketToTransportFeedback(options.packet_id, *packet_to_send, pacing_info); } @@ -971,7 +980,14 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, PacketOptions options; options.is_retransmit = false; - if (UpdateTransportSequenceNumber(packet.get(), &options.packet_id)) { + + bool has_transport_seq_num; + { + rtc::CritScope lock(&send_critsect_); + has_transport_seq_num = + UpdateTransportSequenceNumber(packet.get(), &options.packet_id); + } + if (has_transport_seq_num) { AddPacketToTransportFeedback(options.packet_id, *packet.get(), PacedPacketInfo()); } @@ -1181,10 +1197,9 @@ bool RTPSender::AssignSequenceNumber(RtpPacketToSend* packet) { } bool RTPSender::UpdateTransportSequenceNumber(RtpPacketToSend* packet, - int* packet_id) const { + int* packet_id) { RTC_DCHECK(packet); RTC_DCHECK(packet_id); - rtc::CritScope lock(&send_critsect_); if (!rtp_header_extension_map_.IsRegistered(TransportSequenceNumber::kId)) return false; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 5a93d7a520..1adf6fe8c7 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -247,8 +247,8 @@ class RTPSender { int64_t capture_time_ms, uint32_t ssrc); - bool UpdateTransportSequenceNumber(RtpPacketToSend* packet, - int* packet_id) const; + bool UpdateTransportSequenceNumber(RtpPacketToSend* packet, int* packet_id) + RTC_EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); void UpdateRtpStats(const RtpPacketToSend& packet, bool is_rtx,