From 1fbfecd81fba36477c1fb7c6003ddc48d9a638b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 26 Aug 2019 19:00:05 +0200 Subject: [PATCH] Use a pass-through pacer instead of special-cased pacerless mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL removes the old non-paced code path and instead uses a helper class to just immediately pass the packet through the same code path as when an actual pacer is used. Bug: webrtc:10633 Change-Id: Id9a3ee4719829ad07710f5468e5452aa4bc8570b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150530 Commit-Queue: Erik Språng Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#28963} --- modules/rtp_rtcp/source/rtp_sender.cc | 245 ++++-------------- modules/rtp_rtcp/source/rtp_sender.h | 18 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 29 +-- 3 files changed, 69 insertions(+), 223 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index b068894c98..a61a2cbe0d 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -103,6 +103,21 @@ bool HasBweExtension(const RtpHeaderExtensionMap& extensions_map) { } // namespace +RTPSender::NonPacedPacketSender::NonPacedPacketSender(RTPSender* rtp_sender) + : transport_sequence_number_(0), rtp_sender_(rtp_sender) {} +RTPSender::NonPacedPacketSender::~NonPacedPacketSender() = default; + +void RTPSender::NonPacedPacketSender::EnqueuePacket( + std::unique_ptr packet) { + if (!packet->SetExtension( + ++transport_sequence_number_)) { + --transport_sequence_number_; + } + packet->ReserveExtension(); + packet->ReserveExtension(); + rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); +} + RTPSender::RTPSender(const RtpRtcp::Configuration& config) : clock_(config.clock), random_(clock_->TimeInMicroseconds()), @@ -110,7 +125,10 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) flexfec_ssrc_(config.flexfec_sender ? absl::make_optional(config.flexfec_sender->ssrc()) : absl::nullopt), - paced_sender_(config.paced_sender), + non_paced_packet_sender_( + config.paced_sender ? nullptr : new NonPacedPacketSender(this)), + paced_sender_(config.paced_sender ? config.paced_sender + : non_paced_packet_sender_.get()), transport_sequence_number_allocator_( config.transport_sequence_number_allocator), transport_feedback_observer_(config.transport_feedback_callback), @@ -159,6 +177,7 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) // Random start, 16 bits. Can't be 0. sequence_number_rtx_ = random_.Rand(1, kMaxInitRtpSeqNumber); sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber); + RTC_DCHECK(paced_sender_); } RTPSender::~RTPSender() { @@ -297,56 +316,34 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id) { const int32_t packet_size = static_cast(stored_packet->packet_size); const bool rtx = (RtxStatus() & kRtxRetransmitted) > 0; - if (paced_sender_) { - std::unique_ptr packet = - packet_history_.GetPacketAndMarkAsPending( - packet_id, [&](const RtpPacketToSend& stored_packet) { - // Check if we're overusing retransmission bitrate. - // TODO(sprang): Add histograms for nack success or failure - // reasons. - std::unique_ptr retransmit_packet; - if (retransmission_rate_limiter_ && - !retransmission_rate_limiter_->TryUseRate(packet_size)) { - return retransmit_packet; - } - if (rtx) { - retransmit_packet = BuildRtxPacket(stored_packet); - } else { - retransmit_packet = - absl::make_unique(stored_packet); - } - if (retransmit_packet) { - retransmit_packet->set_retransmitted_sequence_number( - stored_packet.SequenceNumber()); - } - return retransmit_packet; - }); - if (!packet) { - return -1; - } - packet->set_packet_type(RtpPacketToSend::Type::kRetransmission); - paced_sender_->EnqueuePacket(std::move(packet)); - - return packet_size; - } - - // TODO(sprang): Replace this whole code-path with a pass-through pacer. - // Check if we're overusing retransmission bitrate. - // TODO(sprang): Add histograms for nack success or failure reasons. - if (retransmission_rate_limiter_ && - !retransmission_rate_limiter_->TryUseRate(packet_size)) { - return -1; - } - std::unique_ptr packet = - packet_history_.GetPacketAndSetSendTime(packet_id); + packet_history_.GetPacketAndMarkAsPending( + packet_id, [&](const RtpPacketToSend& stored_packet) { + // Check if we're overusing retransmission bitrate. + // TODO(sprang): Add histograms for nack success or failure + // reasons. + std::unique_ptr retransmit_packet; + if (retransmission_rate_limiter_ && + !retransmission_rate_limiter_->TryUseRate(packet_size)) { + return retransmit_packet; + } + if (rtx) { + retransmit_packet = BuildRtxPacket(stored_packet); + } else { + retransmit_packet = + absl::make_unique(stored_packet); + } + if (retransmit_packet) { + retransmit_packet->set_retransmitted_sequence_number( + stored_packet.SequenceNumber()); + } + return retransmit_packet; + }); if (!packet) { - // Packet could theoretically time out between the first check and this one. - return 0; - } - - if (!PrepareAndSendPacket(std::move(packet), rtx, true, PacedPacketInfo())) return -1; + } + packet->set_packet_type(RtpPacketToSend::Type::kRetransmission); + paced_sender_->EnqueuePacket(std::move(packet)); return packet_size; } @@ -528,83 +525,6 @@ bool RTPSender::SupportsRtxPayloadPadding() const { (rtx_ & kRtxRedundantPayloads); } -bool RTPSender::PrepareAndSendPacket(std::unique_ptr packet, - bool send_over_rtx, - bool is_retransmit, - const PacedPacketInfo& pacing_info) { - RTC_DCHECK(packet); - int64_t capture_time_ms = packet->capture_time_ms(); - RtpPacketToSend* packet_to_send = packet.get(); - - std::unique_ptr packet_rtx; - if (send_over_rtx) { - packet_rtx = BuildRtxPacket(*packet); - if (!packet_rtx) - return false; - packet_to_send = packet_rtx.get(); - } - - // Bug webrtc:7859. While FEC is invoked from rtp_sender_video, and not after - // the pacer, these modifications of the header below are happening after the - // FEC protection packets are calculated. This will corrupt recovered packets - // at the same place. It's not an issue for extensions, which are present in - // all the packets (their content just may be incorrect on recovered packets). - // In case of VideoTimingExtension, since it's present not in every packet, - // data after rtp header may be corrupted if these packets are protected by - // the FEC. - int64_t now_ms = clock_->TimeInMilliseconds(); - int64_t diff_ms = now_ms - capture_time_ms; - packet_to_send->SetExtension(kTimestampTicksPerMs * - diff_ms); - packet_to_send->SetExtension( - AbsoluteSendTime::MsTo24Bits(now_ms)); - - if (packet_to_send->HasExtension()) { - if (populate_network2_timestamp_) { - packet_to_send->set_network2_time_ms(now_ms); - } else { - packet_to_send->set_pacer_exit_time_ms(now_ms); - } - } - - PacketOptions options; - // If we are sending over RTX, it also means this is a retransmission. - // E.g. RTPSender::TrySendRedundantPayloads calls PrepareAndSendPacket with - // send_over_rtx = true but is_retransmit = false. - options.is_retransmit = is_retransmit || send_over_rtx; - bool has_transport_seq_num; - { - rtc::CritScope lock(&send_critsect_); - has_transport_seq_num = - UpdateTransportSequenceNumber(packet_to_send, &options.packet_id); - options.included_in_allocation = - has_transport_seq_num || force_part_of_allocation_; - options.included_in_feedback = has_transport_seq_num; - } - if (has_transport_seq_num) { - AddPacketToTransportFeedback(options.packet_id, *packet_to_send, - pacing_info); - } - options.application_data.assign(packet_to_send->application_data().begin(), - packet_to_send->application_data().end()); - - if (!is_retransmit && !send_over_rtx) { - UpdateDelayStatistics(packet->capture_time_ms(), now_ms, packet->Ssrc()); - UpdateOnSendPacket(options.packet_id, packet->capture_time_ms(), - packet->Ssrc()); - } - - if (!SendPacketToNetwork(*packet_to_send, options, pacing_info)) - return false; - - { - rtc::CritScope lock(&send_critsect_); - media_has_been_sent_ = true; - } - UpdateRtpStats(*packet_to_send, send_over_rtx, is_retransmit); - return true; -} - void RTPSender::UpdateRtpStats(const RtpPacketToSend& packet, bool is_rtx, bool is_retransmit) { @@ -752,77 +672,18 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, RTC_DCHECK(packet); int64_t now_ms = clock_->TimeInMilliseconds(); - uint32_t ssrc = packet->Ssrc(); - if (paced_sender_) { - auto packet_type = packet->packet_type(); - RTC_CHECK(packet_type) << "Packet type must be set before sending."; + auto packet_type = packet->packet_type(); + RTC_CHECK(packet_type) << "Packet type must be set before sending."; - if (packet->capture_time_ms() <= 0) { - packet->set_capture_time_ms(now_ms); - } - - packet->set_allow_retransmission(storage == - StorageType::kAllowRetransmission); - paced_sender_->EnqueuePacket(std::move(packet)); - - return true; + if (packet->capture_time_ms() <= 0) { + packet->set_capture_time_ms(now_ms); } - PacketOptions options; - options.is_retransmit = false; + packet->set_allow_retransmission(storage == + StorageType::kAllowRetransmission); + paced_sender_->EnqueuePacket(std::move(packet)); - // |capture_time_ms| <= 0 is considered invalid. - // TODO(holmer): This should be changed all over Video Engine so that negative - // time is consider invalid, while 0 is considered a valid time. - if (packet->capture_time_ms() > 0) { - packet->SetExtension( - kTimestampTicksPerMs * (now_ms - packet->capture_time_ms())); - - if (populate_network2_timestamp_ && - packet->HasExtension()) { - packet->set_network2_time_ms(now_ms); - } - } - packet->SetExtension(AbsoluteSendTime::MsTo24Bits(now_ms)); - - bool has_transport_seq_num; - { - rtc::CritScope lock(&send_critsect_); - has_transport_seq_num = - UpdateTransportSequenceNumber(packet.get(), &options.packet_id); - options.included_in_allocation = - has_transport_seq_num || force_part_of_allocation_; - options.included_in_feedback = has_transport_seq_num; - } - if (has_transport_seq_num) { - AddPacketToTransportFeedback(options.packet_id, *packet.get(), - PacedPacketInfo()); - } - options.application_data.assign(packet->application_data().begin(), - packet->application_data().end()); - - UpdateDelayStatistics(packet->capture_time_ms(), now_ms, packet->Ssrc()); - UpdateOnSendPacket(options.packet_id, packet->capture_time_ms(), - packet->Ssrc()); - - bool sent = SendPacketToNetwork(*packet, options, PacedPacketInfo()); - - if (sent) { - { - rtc::CritScope lock(&send_critsect_); - media_has_been_sent_ = true; - } - UpdateRtpStats(*packet, false, false); - } - - // To support retransmissions, we store the media packet as sent in the - // packet history (even if send failed). - if (storage == kAllowRetransmission) { - RTC_DCHECK_EQ(ssrc, SSRC()); - packet_history_.PutRtpPacket(std::move(packet), storage, now_ms); - } - - return sent; + return true; } void RTPSender::RecomputeMaxSendDelay() { diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index f384f75856..dfade3dd34 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -173,10 +173,19 @@ class RTPSender { // time. typedef std::map SendDelayMap; - bool PrepareAndSendPacket(std::unique_ptr packet, - bool send_over_rtx, - bool is_retransmit, - const PacedPacketInfo& pacing_info); + // Helper class that redirects packets directly to the send part of this class + // without passing through an actual paced sender. + class NonPacedPacketSender : public RtpPacketSender { + public: + explicit NonPacedPacketSender(RTPSender* rtp_sender); + virtual ~NonPacedPacketSender(); + + void EnqueuePacket(std::unique_ptr packet) override; + + private: + uint16_t transport_sequence_number_; + RTPSender* const rtp_sender_; + }; std::unique_ptr BuildRtxPacket( const RtpPacketToSend& packet); @@ -215,6 +224,7 @@ class RTPSender { const absl::optional flexfec_ssrc_; + const std::unique_ptr non_paced_packet_sender_; RtpPacketSender* const paced_sender_; TransportSequenceNumberAllocator* const transport_sequence_number_allocator_; TransportFeedbackObserver* const transport_feedback_observer_; diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 67cad44d72..2ae5891a85 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -64,7 +64,7 @@ const uint16_t kSeqNum = 33; const uint32_t kSsrc = 725242; const uint32_t kRtxSsrc = 12345; const uint32_t kFlexFecSsrc = 45678; -const uint16_t kTransportSequenceNumber = 0xaabbu; +const uint16_t kTransportSequenceNumber = 1; const uint64_t kStartTime = 123456789; const size_t kMaxPaddingSize = 224u; const uint8_t kPayloadData[] = {47, 11, 32, 93, 89}; @@ -175,12 +175,6 @@ class MockRtpPacketPacer : public RtpPacketSender { MOCK_METHOD1(SetAccountForAudioPackets, void(bool account_for_audio)); }; -class MockTransportSequenceNumberAllocator - : public TransportSequenceNumberAllocator { - public: - MOCK_METHOD0(AllocateSequenceNumber, uint16_t()); -}; - class MockSendSideDelayObserver : public SendSideDelayObserver { public: MOCK_METHOD4(SendSideDelayUpdated, void(int, int, uint64_t, uint32_t)); @@ -232,7 +226,6 @@ class RtpSenderTest : public ::testing::TestWithParam { config.local_media_ssrc = kSsrc; config.rtx_send_ssrc = kRtxSsrc; config.flexfec_sender = &flexfec_sender_; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; @@ -246,7 +239,6 @@ class RtpSenderTest : public ::testing::TestWithParam { SimulatedClock fake_clock_; NiceMock mock_rtc_event_log_; MockRtpPacketPacer mock_paced_sender_; - StrictMock seq_num_allocator_; StrictMock send_packet_observer_; StrictMock feedback_observer_; RateLimiter retransmission_rate_limiter_; @@ -468,7 +460,6 @@ TEST_P(RtpSenderTestWithoutPacer, config.clock = &fake_clock_; config.outgoing_transport = &transport_; config.local_media_ssrc = kSsrc; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.transport_feedback_callback = &feedback_observer_; config.event_log = &mock_rtc_event_log_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; @@ -478,8 +469,6 @@ TEST_P(RtpSenderTestWithoutPacer, EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId)); - EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) - .WillOnce(Return(kTransportSequenceNumber)); const size_t expected_bytes = GetParam().with_overhead @@ -507,7 +496,6 @@ TEST_P(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { config.clock = &fake_clock_; config.outgoing_transport = &transport_; config.local_media_ssrc = kSsrc; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.transport_feedback_callback = &feedback_observer_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; @@ -518,8 +506,6 @@ TEST_P(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId)); - EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) - .WillOnce(Return(kTransportSequenceNumber)); EXPECT_CALL(send_packet_observer_, OnSendPacket(kTransportSequenceNumber, _, _)) .Times(1); @@ -549,7 +535,6 @@ TEST_P(RtpSenderTestWithoutPacer, PacketOptionsNoRetransmission) { config.clock = &fake_clock_; config.outgoing_transport = &transport_; config.local_media_ssrc = kSsrc; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.transport_feedback_callback = &feedback_observer_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; @@ -566,8 +551,6 @@ TEST_P(RtpSenderTestWithoutPacer, SetUpRtpSender(false, false); rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId); - EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) - .WillOnce(Return(kTransportSequenceNumber)); EXPECT_CALL(send_packet_observer_, OnSendPacket).Times(1); SendGenericPacket(); EXPECT_TRUE(transport_.last_options_.included_in_feedback); @@ -579,8 +562,6 @@ TEST_P( SetUpRtpSender(false, false); rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId); - EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) - .WillOnce(Return(kTransportSequenceNumber)); EXPECT_CALL(send_packet_observer_, OnSendPacket).Times(1); SendGenericPacket(); EXPECT_TRUE(transport_.last_options_.included_in_allocation); @@ -684,8 +665,6 @@ TEST_P(RtpSenderTestWithoutPacer, OnSendPacketUpdated) { EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId)); - EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) - .WillOnce(Return(kTransportSequenceNumber)); EXPECT_CALL(send_packet_observer_, OnSendPacket(kTransportSequenceNumber, _, _)) .Times(1); @@ -699,7 +678,6 @@ TEST_P(RtpSenderTest, SendsPacketsWithTransportSequenceNumber) { config.outgoing_transport = &transport_; config.paced_sender = &mock_paced_sender_; config.local_media_ssrc = kSsrc; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.transport_feedback_callback = &feedback_observer_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; @@ -824,6 +802,7 @@ TEST_P(RtpSenderTest, WritesNetwork2ToTimingExtensionWithoutPacer) { const VideoSendTiming kVideoTiming = {0u, 0u, 0u, 0u, 0u, 0u, true}; packet->SetExtension(kVideoTiming); EXPECT_TRUE(rtp_sender_->AssignSequenceNumber(packet.get())); + packet->set_packet_type(RtpPacketToSend::Type::kVideo); const int kPropagateTimeMs = 10; fake_clock_.AdvanceTimeMilliseconds(kPropagateTimeMs); @@ -1182,7 +1161,6 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { config.paced_sender = &mock_paced_sender_; config.local_media_ssrc = kSsrc; config.flexfec_sender = &flexfec_sender_; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; @@ -1268,7 +1246,6 @@ TEST_P(RtpSenderTest, NoFlexfecForTimingFrames) { config.outgoing_transport = &transport_; config.paced_sender = &mock_paced_sender_; config.flexfec_sender = &flexfec_sender; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; @@ -1394,7 +1371,6 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { config.outgoing_transport = &transport_; config.local_media_ssrc = kSsrc; config.flexfec_sender = &flexfec_sender; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_; @@ -1663,7 +1639,6 @@ TEST_P(RtpSenderTest, FecOverheadRate) { config.paced_sender = &mock_paced_sender_; config.local_media_ssrc = kSsrc; config.flexfec_sender = &flexfec_sender; - config.transport_sequence_number_allocator = &seq_num_allocator_; config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.retransmission_rate_limiter = &retransmission_rate_limiter_;