From 5f1d406cc9c1d3429fd1e816ef5bedd3e62627ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 12 Aug 2021 11:34:03 +0200 Subject: [PATCH] Move legacy RtpRtcpModule to deferred sequencing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:11340 Change-Id: I45ba6c37fe7fec8de756dee1eb914aceebbeae93 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228437 Commit-Queue: Erik Språng Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#34734} --- .../deprecated_rtp_sender_egress.cc | 14 ++++++- .../deprecated/deprecated_rtp_sender_egress.h | 5 ++- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 42 +++++++++++++++---- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 3 +- .../rtp_rtcp/source/rtp_rtcp_impl_unittest.cc | 5 +-- 5 files changed, 52 insertions(+), 17 deletions(-) diff --git a/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc b/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc index c542557526..201a1ce05c 100644 --- a/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc @@ -36,14 +36,24 @@ bool IsDisabled(absl::string_view name, } // namespace DEPRECATED_RtpSenderEgress::NonPacedPacketSender::NonPacedPacketSender( - DEPRECATED_RtpSenderEgress* sender) - : transport_sequence_number_(0), sender_(sender) {} + DEPRECATED_RtpSenderEgress* sender, + PacketSequencer* sequence_number_assigner) + : transport_sequence_number_(0), + sender_(sender), + sequence_number_assigner_(sequence_number_assigner) { + RTC_DCHECK(sequence_number_assigner_); +} DEPRECATED_RtpSenderEgress::NonPacedPacketSender::~NonPacedPacketSender() = default; void DEPRECATED_RtpSenderEgress::NonPacedPacketSender::EnqueuePackets( std::vector> packets) { for (auto& packet : packets) { + // Assign sequence numbers, but not for flexfec which is already running on + // an internally maintained sequence number series. + if (packet->Ssrc() != sender_->FlexFecSsrc()) { + sequence_number_assigner_->Sequence(*packet); + } if (!packet->SetExtension( ++transport_sequence_number_)) { --transport_sequence_number_; diff --git a/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.h b/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.h index 4aeb43056a..fc440d14f2 100644 --- a/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.h +++ b/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.h @@ -20,6 +20,7 @@ #include "api/rtc_event_log/rtc_event_log.h" #include "api/units/data_rate.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/packet_sequencer.h" #include "modules/rtp_rtcp/source/rtp_packet_history.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "modules/rtp_rtcp/source/rtp_rtcp_interface.h" @@ -36,7 +37,8 @@ class DEPRECATED_RtpSenderEgress { // without passing through an actual paced sender. class NonPacedPacketSender : public RtpPacketSender { public: - explicit NonPacedPacketSender(DEPRECATED_RtpSenderEgress* sender); + NonPacedPacketSender(DEPRECATED_RtpSenderEgress* sender, + PacketSequencer* sequence_number_assigner); virtual ~NonPacedPacketSender(); void EnqueuePackets( @@ -45,6 +47,7 @@ class DEPRECATED_RtpSenderEgress { private: uint16_t transport_sequence_number_; DEPRECATED_RtpSenderEgress* const sender_; + PacketSequencer* sequence_number_assigner_; }; DEPRECATED_RtpSenderEgress(const RtpRtcpInterface::Configuration& config, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index e3fd8abff8..f6f9afb427 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -49,12 +49,12 @@ ModuleRtpRtcpImpl::RtpSenderContext::RtpSenderContext( /*require_marker_before_media_padding=*/!config.audio, config.clock), packet_sender(config, &packet_history), - non_paced_sender(&packet_sender), + non_paced_sender(&packet_sender, &sequencer_), packet_generator( config, &packet_history, config.paced_sender ? config.paced_sender : &non_paced_sender, - &sequencer_) {} + /*packet_sequencer=*/nullptr) {} std::unique_ptr RtpRtcp::DEPRECATED_Create( const Configuration& configuration) { @@ -255,30 +255,41 @@ void ModuleRtpRtcpImpl::SetStartTimestamp(const uint32_t timestamp) { } uint16_t ModuleRtpRtcpImpl::SequenceNumber() const { - return rtp_sender_->packet_generator.SequenceNumber(); + MutexLock lock(&rtp_sender_->sequencer_mutex); + return rtp_sender_->sequencer_.media_sequence_number(); } // Set SequenceNumber, default is a random number. void ModuleRtpRtcpImpl::SetSequenceNumber(const uint16_t seq_num) { - rtp_sender_->packet_generator.SetSequenceNumber(seq_num); + MutexLock lock(&rtp_sender_->sequencer_mutex); + rtp_sender_->sequencer_.set_media_sequence_number(seq_num); } void ModuleRtpRtcpImpl::SetRtpState(const RtpState& rtp_state) { + MutexLock lock(&rtp_sender_->sequencer_mutex); rtp_sender_->packet_generator.SetRtpState(rtp_state); + rtp_sender_->sequencer_.SetRtpState(rtp_state); rtcp_sender_.SetTimestampOffset(rtp_state.start_timestamp); } void ModuleRtpRtcpImpl::SetRtxState(const RtpState& rtp_state) { + MutexLock lock(&rtp_sender_->sequencer_mutex); rtp_sender_->packet_generator.SetRtxRtpState(rtp_state); + rtp_sender_->sequencer_.set_rtx_sequence_number(rtp_state.sequence_number); } RtpState ModuleRtpRtcpImpl::GetRtpState() const { + MutexLock lock(&rtp_sender_->sequencer_mutex); RtpState state = rtp_sender_->packet_generator.GetRtpState(); + rtp_sender_->sequencer_.PopulateRtpState(state); return state; } RtpState ModuleRtpRtcpImpl::GetRtxState() const { - return rtp_sender_->packet_generator.GetRtxRtpState(); + MutexLock lock(&rtp_sender_->sequencer_mutex); + RtpState state = rtp_sender_->packet_generator.GetRtxRtpState(); + state.sequence_number = rtp_sender_->sequencer_.rtx_sequence_number(); + return state; } void ModuleRtpRtcpImpl::SetRid(const std::string& rid) { @@ -410,6 +421,21 @@ bool ModuleRtpRtcpImpl::TrySendPacket(RtpPacketToSend* packet, if (!rtp_sender_->packet_generator.SendingMedia()) { return false; } + { + MutexLock lock(&rtp_sender_->sequencer_mutex); + if (packet->packet_type() == RtpPacketMediaType::kPadding && + packet->Ssrc() == rtp_sender_->packet_generator.SSRC() && + !rtp_sender_->sequencer_.CanSendPaddingOnMediaSsrc()) { + // New media packet preempted this generated padding packet, discard it. + return false; + } + bool is_flexfec = + packet->packet_type() == RtpPacketMediaType::kForwardErrorCorrection && + packet->Ssrc() == rtp_sender_->packet_generator.FlexfecSsrc(); + if (!is_flexfec) { + rtp_sender_->sequencer_.Sequence(*packet); + } + } rtp_sender_->packet_sender.SendPacket(packet, pacing_info); return true; } @@ -444,12 +470,10 @@ bool ModuleRtpRtcpImpl::SupportsRtxPayloadPadding() const { std::vector> ModuleRtpRtcpImpl::GeneratePadding(size_t target_size_bytes) { RTC_DCHECK(rtp_sender_); - // `can_send_padding_on_media_ssrc` set to false but is ignored at this - // point, RTPSender will internally query `sequencer_` while holding the - // send lock. + MutexLock lock(&rtp_sender_->sequencer_mutex); return rtp_sender_->packet_generator.GeneratePadding( target_size_bytes, rtp_sender_->packet_sender.MediaHasBeenSent(), - /*can_send_padding_on_media_ssrc=*/false); + rtp_sender_->sequencer_.CanSendPaddingOnMediaSsrc()); } std::vector diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index c5d0b3a91e..81b1170b13 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -275,7 +275,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { // Storage of packets, for retransmissions and padding, if applicable. RtpPacketHistory packet_history; // Handles sequence number assignment and padding timestamp generation. - PacketSequencer sequencer_; + mutable Mutex sequencer_mutex; + PacketSequencer sequencer_ RTC_GUARDED_BY(sequencer_mutex); // Handles final time timestamping/stats/etc and handover to Transport. DEPRECATED_RtpSenderEgress packet_sender; // If no paced sender configured, this class will be used to pass packets diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc index 0902e90d51..70c9a1cf40 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -566,6 +566,7 @@ TEST_F(RtpRtcpImplTest, StoresPacketInfoForSentPackets) { const uint32_t kStartTimestamp = 1u; SetUp(); sender_.impl_->SetStartTimestamp(kStartTimestamp); + sender_.impl_->SetSequenceNumber(1); PacedPacketInfo pacing_info; RtpPacketToSend packet(nullptr); @@ -574,7 +575,6 @@ TEST_F(RtpRtcpImplTest, StoresPacketInfoForSentPackets) { // Single-packet frame. packet.SetTimestamp(1); - packet.SetSequenceNumber(1); packet.set_first_packet_of_frame(true); packet.SetMarker(true); sender_.impl_->TrySendPacket(&packet, pacing_info); @@ -589,16 +589,13 @@ TEST_F(RtpRtcpImplTest, StoresPacketInfoForSentPackets) { // Three-packet frame. packet.SetTimestamp(2); - packet.SetSequenceNumber(2); packet.set_first_packet_of_frame(true); packet.SetMarker(false); sender_.impl_->TrySendPacket(&packet, pacing_info); - packet.SetSequenceNumber(3); packet.set_first_packet_of_frame(false); sender_.impl_->TrySendPacket(&packet, pacing_info); - packet.SetSequenceNumber(4); packet.SetMarker(true); sender_.impl_->TrySendPacket(&packet, pacing_info);