From 8b7ca4abb20179a952669cb123a1bdfac5ba89c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 17 May 2018 13:43:35 +0200 Subject: [PATCH] Make packet router send padding on rtp module that last sent media. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we prefer the last added rtp module that supports rtx, and assume this is the HD stream. If we suffer a network degradation and stop sending HD, the current behavior will trigger RTX padding on an inactive stream, which is not very useful. With this change, we will prefer the rtp module that last sent media, which will spread the load a bit across active media streams, but will be biased toward the one with highest packet rate. Bug: webrtc:8975 Change-Id: Id52865ccd5263722c66d327b8c80457f63b90385 Reviewed-on: https://webrtc-review.googlesource.com/77360 Commit-Queue: Erik Språng Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#23281} --- modules/pacing/packet_router.cc | 38 ++++++++++-- modules/pacing/packet_router.h | 2 + modules/pacing/packet_router_unittest.cc | 78 ++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 4 deletions(-) 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;