diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index ef9d10ae55..b27c73c2f0 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -28,7 +28,8 @@ constexpr int kRembSendIntervalMs = 200; } // namespace PacketRouter::PacketRouter() - : last_remb_time_ms_(rtc::TimeMillis()), + : last_send_module_(nullptr), + last_remb_time_ms_(rtc::TimeMillis()), last_send_bitrate_bps_(0), bitrate_bps_(0), max_bitrate_bps_(std::numeric_limits::max()), @@ -67,6 +68,9 @@ void PacketRouter::RemoveSendRtpModule(RtpRtcp* rtp_module) { std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), rtp_module); RTC_DCHECK(it != rtp_send_modules_.end()); rtp_send_modules_.erase(it); + if (last_send_module_ == rtp_module) { + last_send_module_ = nullptr; + } } void PacketRouter::AddReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender, @@ -100,9 +104,16 @@ bool PacketRouter::TimeToSendPacket(uint32_t ssrc, const PacedPacketInfo& pacing_info) { rtc::CritScope cs(&modules_crit_); for (auto* rtp_module : rtp_send_modules_) { - if (!rtp_module->SendingMedia()) + if (!rtp_module->SendingMedia()) { continue; + } if (ssrc == rtp_module->SSRC() || ssrc == rtp_module->FlexfecSsrc()) { + if ((rtp_module->RtxSendStatus() & kRtxRedundantPayloads) && + rtp_module->HasBweExtensions()) { + // This is now the last module to send media, and has the desired + // properties needed for payload based padding. Cache it for later use. + last_send_module_ = rtp_module; + } return rtp_module->TimeToSendPacket(ssrc, sequence_number, capture_timestamp, retransmission, pacing_info); @@ -115,6 +126,23 @@ size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send, const PacedPacketInfo& pacing_info) { size_t total_bytes_sent = 0; rtc::CritScope cs(&modules_crit_); + // 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 + // will be more skewed towards the highest bitrate stream. At the very least + // this prevents sending payload padding on a disabled stream where it's + // guaranteed not to be useful. + if (last_send_module_ != nullptr) { + RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), + last_send_module_) != rtp_send_modules_.end()); + RTC_DCHECK(last_send_module_->HasBweExtensions()); + total_bytes_sent += last_send_module_->TimeToSendPadding( + bytes_to_send - total_bytes_sent, pacing_info); + if (total_bytes_sent >= bytes_to_send) { + return total_bytes_sent; + } + } + // Rtp modules are ordered by which stream can most benefit from padding. for (RtpRtcp* module : rtp_send_modules_) { if (module->SendingMedia() && module->HasBweExtensions()) { @@ -225,13 +253,15 @@ bool PacketRouter::SendTransportFeedback(rtcp::TransportFeedback* packet) { // Prefer send modules. for (auto* rtp_module : rtp_send_modules_) { packet->SetSenderSsrc(rtp_module->SSRC()); - if (rtp_module->SendFeedbackPacket(*packet)) + if (rtp_module->SendFeedbackPacket(*packet)) { return true; + } } for (auto* rtcp_sender : rtcp_feedback_senders_) { packet->SetSenderSsrc(rtcp_sender->SSRC()); - if (rtcp_sender->SendFeedbackPacket(*packet)) + if (rtcp_sender->SendFeedbackPacket(*packet)) { return true; + } } return false; } diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 22a578c0bf..f01e9ef493 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -94,6 +94,8 @@ class PacketRouter : public PacedSender::PacketSender, rtc::CriticalSection modules_crit_; // Rtp and Rtcp modules of the rtp senders. std::list rtp_send_modules_ RTC_GUARDED_BY(modules_crit_); + // The last module used to send media. + RtpRtcp* last_send_module_ RTC_GUARDED_BY(modules_crit_); // Rtcp modules of the rtp receivers. std::vector rtcp_feedback_senders_ RTC_GUARDED_BY(modules_crit_); diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index ec91f793d6..34b533bbe4 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -271,6 +271,84 @@ TEST(PacketRouterTest, TimeToSendPadding) { packet_router.RemoveSendRtpModule(&rtp_2); } +TEST(PacketRouterTest, PadsOnLastActiveMediaStream) { + PacketRouter packet_router; + + const uint16_t kSsrc1 = 1234; + const uint16_t kSsrc2 = 4567; + const uint16_t kSsrc3 = 8901; + + // First two rtp modules send media and have rtx. + NiceMock rtp_1; + EXPECT_CALL(rtp_1, RtxSendStatus()) + .WillRepeatedly(Return(kRtxRedundantPayloads)); + EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1)); + EXPECT_CALL(rtp_1, SendingMedia()).WillRepeatedly(Return(true)); + EXPECT_CALL(rtp_1, HasBweExtensions()).WillRepeatedly(Return(true)); + + NiceMock rtp_2; + EXPECT_CALL(rtp_2, RtxSendStatus()) + .WillRepeatedly(Return(kRtxRedundantPayloads)); + EXPECT_CALL(rtp_2, SSRC()).WillRepeatedly(Return(kSsrc2)); + EXPECT_CALL(rtp_2, SendingMedia()).WillRepeatedly(Return(true)); + EXPECT_CALL(rtp_2, HasBweExtensions()).WillRepeatedly(Return(true)); + + // Third module is sending media, but does not support rtx. + NiceMock rtp_3; + EXPECT_CALL(rtp_3, RtxSendStatus()).WillRepeatedly(Return(kRtxOff)); + EXPECT_CALL(rtp_3, SSRC()).WillRepeatedly(Return(kSsrc3)); + EXPECT_CALL(rtp_3, SendingMedia()).WillRepeatedly(Return(true)); + EXPECT_CALL(rtp_3, HasBweExtensions()).WillRepeatedly(Return(true)); + + packet_router.AddSendRtpModule(&rtp_1, false); + packet_router.AddSendRtpModule(&rtp_2, false); + packet_router.AddSendRtpModule(&rtp_3, false); + + const size_t kPaddingBytes = 100; + + // Initially, padding will be sent on last added rtp module that sends media + // and supports rtx. + EXPECT_CALL(rtp_2, TimeToSendPadding(kPaddingBytes, _)) + .Times(1) + .WillOnce(Return(kPaddingBytes)); + packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + + // Send media on first module. Padding should be sent on that module. + EXPECT_CALL(rtp_1, TimeToSendPacket(kSsrc1, _, _, _, _)); + packet_router.TimeToSendPacket(kSsrc1, 0, 0, false, PacedPacketInfo()); + + EXPECT_CALL(rtp_1, TimeToSendPadding(kPaddingBytes, _)) + .Times(1) + .WillOnce(Return(kPaddingBytes)); + packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + + // Send media on second module. Padding should be sent there. + EXPECT_CALL(rtp_2, TimeToSendPacket(kSsrc2, _, _, _, _)); + packet_router.TimeToSendPacket(kSsrc2, 0, 0, false, PacedPacketInfo()); + + EXPECT_CALL(rtp_2, TimeToSendPadding(kPaddingBytes, _)) + .Times(1) + .WillOnce(Return(kPaddingBytes)); + packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + + // Remove second module, padding should now fall back to first module. + packet_router.RemoveSendRtpModule(&rtp_2); + EXPECT_CALL(rtp_1, TimeToSendPadding(kPaddingBytes, _)) + .Times(1) + .WillOnce(Return(kPaddingBytes)); + packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + + // Remove first module too, leaving only the one without rtx. + packet_router.RemoveSendRtpModule(&rtp_1); + + EXPECT_CALL(rtp_3, TimeToSendPadding(kPaddingBytes, _)) + .Times(1) + .WillOnce(Return(kPaddingBytes)); + packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + + packet_router.RemoveSendRtpModule(&rtp_3); +} + TEST(PacketRouterTest, SenderOnlyFunctionsRespectSendingMedia) { PacketRouter packet_router; NiceMock rtp;