diff --git a/audio/channel_send.cc b/audio/channel_send.cc index f57858c344..ed2776de78 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -385,20 +385,6 @@ class RtpPacketSenderProxy : public RtpPacketSender { rtp_packet_pacer_->EnqueuePacket(std::move(packet)); } - // Implements RtpPacketSender. - void InsertPacket(Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission) override { - rtc::CritScope lock(&crit_); - if (rtp_packet_pacer_) { - rtp_packet_pacer_->InsertPacket(priority, ssrc, sequence_number, - capture_time_ms, bytes, retransmission); - } - } - private: rtc::ThreadChecker thread_checker_; rtc::CriticalSection crit_; diff --git a/modules/pacing/mock/mock_paced_sender.h b/modules/pacing/mock/mock_paced_sender.h index 34ef24afb9..fbbac3a876 100644 --- a/modules/pacing/mock/mock_paced_sender.h +++ b/modules/pacing/mock/mock_paced_sender.h @@ -11,6 +11,7 @@ #ifndef MODULES_PACING_MOCK_MOCK_PACED_SENDER_H_ #define MODULES_PACING_MOCK_MOCK_PACED_SENDER_H_ +#include #include #include "modules/pacing/paced_sender.h" @@ -23,20 +24,12 @@ class MockPacedSender : public PacedSender { public: MockPacedSender() : PacedSender(Clock::GetRealTimeClock(), nullptr, nullptr) {} - MOCK_METHOD6(SendPacket, - bool(Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission)); + MOCK_METHOD1(EnqueuePacket, void(std::unique_ptr packet)); MOCK_METHOD2(CreateProbeCluster, void(DataRate, int)); - MOCK_METHOD1(SetEstimatedBitrate, void(uint32_t)); MOCK_METHOD2(SetPacingRates, void(DataRate, DataRate)); - MOCK_CONST_METHOD0(QueueInMs, int64_t()); - MOCK_CONST_METHOD0(QueueInPackets, int()); - MOCK_CONST_METHOD0(ExpectedQueueTimeMs, int64_t()); - MOCK_METHOD0(GetApplicationLimitedRegionStartTime, absl::optional()); + MOCK_CONST_METHOD0(OldestPacketWaitTime, TimeDelta()); + MOCK_CONST_METHOD0(QueueSizePackets, size_t()); + MOCK_CONST_METHOD0(ExpectedQueueTime, TimeDelta()); MOCK_METHOD0(Process, void()); }; diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 83a8da3e84..7891897d5f 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -83,17 +83,6 @@ void PacedSender::SetPacingRates(DataRate pacing_rate, DataRate padding_rate) { pacing_controller_.SetPacingRates(pacing_rate, padding_rate); } -void PacedSender::InsertPacket(RtpPacketSender::Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission) { - rtc::CritScope cs(&critsect_); - pacing_controller_.InsertPacket(priority, ssrc, sequence_number, - capture_time_ms, bytes, retransmission); -} - void PacedSender::EnqueuePacket(std::unique_ptr packet) { rtc::CritScope cs(&critsect_); pacing_controller_.EnqueuePacket(std::move(packet)); diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 7b14480ed9..30fcdab4cf 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -65,14 +65,6 @@ class PacedSender : public Module, // Methods implementing RtpPacketSender. - // Adds the packet information to the queue and calls TimeToSendPacket - // when it's time to send. - void InsertPacket(RtpPacketSender::Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission) override; // Adds the packet to the queue and calls PacketRouter::SendPacket() when // it's time to send. void EnqueuePacket(std::unique_ptr packet) override; diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 8c5761d206..f4fca6cfb6 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -42,15 +42,6 @@ namespace test { // Mock callback implementing the raw api. class MockCallback : public PacketRouter { public: - MOCK_METHOD5(TimeToSendPacket, - RtpPacketSendResult(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info)); - MOCK_METHOD2(TimeToSendPadding, - size_t(size_t bytes, const PacedPacketInfo& pacing_info)); - MOCK_METHOD2(SendPacket, void(std::unique_ptr packet, const PacedPacketInfo& cluster_info)); diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index 3c97163234..3ee7410e3d 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -184,15 +184,6 @@ void PacingController::SetPacingRates(DataRate pacing_rate, << " padding_budget_kbps=" << padding_rate.kbps(); } -void PacingController::InsertPacket(RtpPacketSender::Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission) { - RTC_NOTREACHED(); -} - void PacingController::EnqueuePacket(std::unique_ptr packet) { RTC_DCHECK(pacing_bitrate_ > DataRate::Zero()) << "SetPacingRate must be called before InsertPacket."; diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h index 50d0de030e..1b05444c3b 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h @@ -76,14 +76,6 @@ class PacingController { ~PacingController(); - // Adds the packet information to the queue and calls TimeToSendPacket - // when it's time to send. - void InsertPacket(RtpPacketSender::Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission); // Adds the packet to the queue and calls PacketRouter::SendPacket() when // it's time to send. void EnqueuePacket(std::unique_ptr packet); diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index a14c65e719..b0069f0711 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -54,7 +54,7 @@ void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) { rtp_module) == rtp_send_modules_.end()); // Put modules which can use regular payload packets (over rtx) instead of // padding first as it's less of a waste - if ((rtp_module->RtxSendStatus() & kRtxRedundantPayloads) > 0) { + if (rtp_module->SupportsRtxPayloadPadding()) { rtp_send_modules_.push_front(rtp_module); } else { rtp_send_modules_.push_back(rtp_module); @@ -102,29 +102,6 @@ void PacketRouter::RemoveReceiveRtpModule( rtcp_feedback_senders_.erase(it); } -RtpPacketSendResult PacketRouter::TimeToSendPacket( - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_timestamp, - bool retransmission, - const PacedPacketInfo& pacing_info) { - rtc::CritScope cs(&modules_crit_); - RtpRtcp* rtp_module = FindRtpModule(ssrc); - if (rtp_module == nullptr || !rtp_module->SendingMedia()) { - return RtpPacketSendResult::kPacketNotFound; - } - - RtpPacketSendResult result = rtp_module->TimeToSendPacket( - ssrc, sequence_number, capture_timestamp, retransmission, pacing_info); - if (result == RtpPacketSendResult::kSuccess && - rtp_module->SupportsRtxPayloadPadding()) { - // 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 result; -} - RtpRtcp* PacketRouter::FindRtpModule(uint32_t ssrc) { auto it = rtp_module_cache_map_.find(ssrc); if (it != rtp_module_cache_map_.end()) { @@ -150,7 +127,7 @@ void PacketRouter::SendPacket(std::unique_ptr packet, // With the new pacer code path, transport sequence numbers are only set here, // on the pacer thread. Therefore we don't need atomics/synchronization. if (packet->IsExtensionReserved()) { - packet->SetExtension(++transport_seq_); + packet->SetExtension(AllocateSequenceNumber()); } auto it = rtp_module_cache_map_.find(packet->Ssrc()); @@ -175,41 +152,6 @@ void PacketRouter::SendPacket(std::unique_ptr packet, << packet->SequenceNumber(); } -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 && - last_send_module_->SupportsRtxPayloadPadding()) { - RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), - last_send_module_) != rtp_send_modules_.end()); - 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. - // Don't require RTX payload padding in the general case. - for (RtpRtcp* module : rtp_send_modules_) { - if (module->SupportsPadding()) { - size_t bytes_sent = module->TimeToSendPadding( - bytes_to_send - total_bytes_sent, pacing_info); - total_bytes_sent += bytes_sent; - if (total_bytes_sent >= bytes_to_send) - break; - } - } - return total_bytes_sent; -} - std::vector> PacketRouter::GeneratePadding( size_t target_size_bytes) { rtc::CritScope cs(&modules_crit_); diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 309d4382df..c50905b58c 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -53,19 +53,9 @@ class PacketRouter : public TransportSequenceNumberAllocator, bool remb_candidate); void RemoveReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender); - virtual RtpPacketSendResult TimeToSendPacket( - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_timestamp, - bool retransmission, - const PacedPacketInfo& packet_info); - virtual void SendPacket(std::unique_ptr packet, const PacedPacketInfo& cluster_info); - virtual size_t TimeToSendPadding(size_t bytes, - const PacedPacketInfo& packet_info); - virtual std::vector> GeneratePadding( size_t target_size_bytes); diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index ff1c6f988a..08d76b234f 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -42,7 +42,6 @@ using ::testing::Le; using ::testing::NiceMock; using ::testing::Property; using ::testing::Return; -using ::testing::ReturnPointee; using ::testing::SaveArg; constexpr int kProbeMinProbes = 5; @@ -50,217 +49,54 @@ constexpr int kProbeMinBytes = 1000; } // namespace -TEST(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPacket) { - PacketRouter packet_router; +class PacketRouterTest : public ::testing::Test { + public: + PacketRouterTest() { + const int kTransportSequenceNumberExtensionId = 1; + extension_manager.Register(kRtpExtensionTransportSequenceNumber, + kTransportSequenceNumberExtensionId); + } - constexpr uint16_t ssrc = 1234; - constexpr uint16_t sequence_number = 17; - constexpr uint64_t timestamp = 7890; - constexpr bool retransmission = false; - const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes); + protected: + std::unique_ptr BuildRtpPacket(uint32_t ssrc) { + std::unique_ptr packet = + absl::make_unique(&extension_manager); + packet->SetSsrc(ssrc); + return packet; + } - EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, - packet_router.TimeToSendPacket(ssrc, sequence_number, timestamp, - retransmission, paced_info)); -} - -TEST(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPadding) { - PacketRouter packet_router; + PacketRouter packet_router_; + RtpHeaderExtensionMap extension_manager; +}; +TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_GeneratePadding) { constexpr size_t bytes = 300; const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes); - EXPECT_EQ(packet_router.TimeToSendPadding(bytes, paced_info), 0u); + EXPECT_TRUE(packet_router_.GeneratePadding(bytes).empty()); } -TEST(PacketRouterTest, Sanity_NoModuleRegistered_OnReceiveBitrateChanged) { - PacketRouter packet_router; - +TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_OnReceiveBitrateChanged) { const std::vector ssrcs = {1, 2, 3}; constexpr uint32_t bitrate_bps = 10000; - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_bps); + packet_router_.OnReceiveBitrateChanged(ssrcs, bitrate_bps); } -TEST(PacketRouterTest, Sanity_NoModuleRegistered_SendRemb) { - PacketRouter packet_router; - +TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendRemb) { const std::vector ssrcs = {1, 2, 3}; constexpr uint32_t bitrate_bps = 10000; - EXPECT_FALSE(packet_router.SendRemb(bitrate_bps, ssrcs)); + EXPECT_FALSE(packet_router_.SendRemb(bitrate_bps, ssrcs)); } -TEST(PacketRouterTest, Sanity_NoModuleRegistered_SendTransportFeedback) { - PacketRouter packet_router; - +TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendTransportFeedback) { rtcp::TransportFeedback feedback; - EXPECT_FALSE(packet_router.SendTransportFeedback(&feedback)); + EXPECT_FALSE(packet_router_.SendTransportFeedback(&feedback)); } -TEST(PacketRouterTest, TimeToSendPacket) { - PacketRouter packet_router; - NiceMock rtp_1; - NiceMock rtp_2; - - packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.AddSendRtpModule(&rtp_2, false); - - const uint16_t kSsrc1 = 1234; - uint16_t sequence_number = 17; - uint64_t timestamp = 7890; - bool retransmission = false; - - // Send on the first module by letting rtp_1 be sending with correct ssrc. - ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); - EXPECT_CALL(rtp_1, TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - Field(&PacedPacketInfo::probe_cluster_id, 1))) - .Times(1) - .WillOnce(Return(RtpPacketSendResult::kSuccess)); - EXPECT_CALL(rtp_2, TimeToSendPacket).Times(0); - EXPECT_EQ(RtpPacketSendResult::kSuccess, - packet_router.TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); - - // Send on the second module by letting rtp_2 be sending, but not rtp_1. - ++sequence_number; - timestamp += 30; - retransmission = true; - const uint16_t kSsrc2 = 4567; - ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(false)); - ON_CALL(rtp_2, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_2, SSRC).WillByDefault(Return(kSsrc2)); - EXPECT_CALL(rtp_1, TimeToSendPacket).Times(0); - EXPECT_CALL(rtp_2, TimeToSendPacket( - kSsrc2, sequence_number, timestamp, retransmission, - Field(&PacedPacketInfo::probe_cluster_id, 2))) - .Times(1) - .WillOnce(Return(RtpPacketSendResult::kSuccess)); - EXPECT_EQ(RtpPacketSendResult::kSuccess, - packet_router.TimeToSendPacket( - kSsrc2, sequence_number, timestamp, retransmission, - PacedPacketInfo(2, kProbeMinProbes, kProbeMinBytes))); - - // No module is sending, hence no packet should be sent. - ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(false)); - ON_CALL(rtp_2, SendingMedia).WillByDefault(Return(false)); - EXPECT_CALL(rtp_1, TimeToSendPacket).Times(0); - EXPECT_CALL(rtp_2, TimeToSendPacket).Times(0); - EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, - packet_router.TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); - - // Add a packet with incorrect ssrc and test it's dropped in the router. - ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); - ON_CALL(rtp_2, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_2, SSRC).WillByDefault(Return(kSsrc2)); - EXPECT_CALL(rtp_1, TimeToSendPacket).Times(0); - EXPECT_CALL(rtp_2, TimeToSendPacket).Times(0); - EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, - packet_router.TimeToSendPacket( - kSsrc1 + kSsrc2, sequence_number, timestamp, retransmission, - PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); - - packet_router.RemoveSendRtpModule(&rtp_1); - - // rtp_1 has been removed, try sending a packet on that ssrc and make sure - // it is dropped as expected by not expecting any calls to rtp_1. - ON_CALL(rtp_2, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_2, SSRC).WillByDefault(Return(kSsrc2)); - EXPECT_CALL(rtp_2, TimeToSendPacket).Times(0); - EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, - packet_router.TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); - - packet_router.RemoveSendRtpModule(&rtp_2); -} - -TEST(PacketRouterTest, TimeToSendPadding) { - PacketRouter packet_router; - - const uint16_t kSsrc1 = 1234; - const uint16_t kSsrc2 = 4567; - - NiceMock rtp_1; - EXPECT_CALL(rtp_1, RtxSendStatus()).WillOnce(Return(kRtxOff)); - EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1)); - NiceMock rtp_2; - // rtp_2 will be prioritized for padding. - EXPECT_CALL(rtp_2, RtxSendStatus()).WillOnce(Return(kRtxRedundantPayloads)); - EXPECT_CALL(rtp_2, SSRC()).WillRepeatedly(Return(kSsrc2)); - packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.AddSendRtpModule(&rtp_2, false); - - // Default configuration, sending padding on all modules sending media, - // ordered by priority (based on rtx mode). - const size_t requested_padding_bytes = 1000; - const size_t sent_padding_bytes = 890; - EXPECT_CALL(rtp_2, SupportsPadding).Times(1).WillOnce(Return(true)); - EXPECT_CALL(rtp_2, - TimeToSendPadding(requested_padding_bytes, - Field(&PacedPacketInfo::probe_cluster_id, 111))) - .Times(1) - .WillOnce(Return(sent_padding_bytes)); - EXPECT_CALL(rtp_1, SupportsPadding).WillOnce(Return(true)); - EXPECT_CALL(rtp_1, - TimeToSendPadding(requested_padding_bytes - sent_padding_bytes, - Field(&PacedPacketInfo::probe_cluster_id, 111))) - .Times(1) - .WillOnce(Return(requested_padding_bytes - sent_padding_bytes)); - EXPECT_EQ(requested_padding_bytes, - packet_router.TimeToSendPadding( - requested_padding_bytes, - PacedPacketInfo(111, kProbeMinBytes, kProbeMinBytes))); - - // Let only the lower priority module be sending and verify the padding - // request is routed there. - EXPECT_CALL(rtp_2, SupportsPadding).WillOnce(Return(false)); - EXPECT_CALL(rtp_2, TimeToSendPadding(requested_padding_bytes, _)).Times(0); - EXPECT_CALL(rtp_1, SupportsPadding).WillOnce(Return(true)); - EXPECT_CALL(rtp_1, TimeToSendPadding(_, _)) - .Times(1) - .WillOnce(Return(sent_padding_bytes)); - EXPECT_EQ(sent_padding_bytes, - packet_router.TimeToSendPadding( - requested_padding_bytes, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); - - // No sending module at all. - EXPECT_CALL(rtp_1, SupportsPadding).WillOnce(Return(false)); - EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes, _)).Times(0); - EXPECT_CALL(rtp_2, SupportsPadding).WillOnce(Return(false)); - EXPECT_CALL(rtp_2, TimeToSendPadding(_, _)).Times(0); - EXPECT_EQ(0u, packet_router.TimeToSendPadding( - requested_padding_bytes, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); - - packet_router.RemoveSendRtpModule(&rtp_1); - - // rtp_1 has been removed, try sending padding and make sure rtp_1 isn't asked - // to send by not expecting any calls. Instead verify rtp_2 is called. - EXPECT_CALL(rtp_2, SupportsPadding).WillOnce(Return(true)); - EXPECT_CALL(rtp_2, TimeToSendPadding(requested_padding_bytes, _)).Times(1); - EXPECT_EQ(0u, packet_router.TimeToSendPadding( - requested_padding_bytes, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); - - packet_router.RemoveSendRtpModule(&rtp_2); -} - -TEST(PacketRouterTest, GeneratePaddingPicksCorrectModule) { - PacketRouter packet_router; - +TEST_F(PacketRouterTest, GeneratePaddingPicksCorrectModule) { // Two RTP modules. The first (prioritized due to rtx) isn't sending media so // should not be called. const uint16_t kSsrc1 = 1234; @@ -276,8 +112,8 @@ TEST(PacketRouterTest, GeneratePaddingPicksCorrectModule) { ON_CALL(rtp_2, SSRC()).WillByDefault(Return(kSsrc2)); ON_CALL(rtp_2, SupportsPadding).WillByDefault(Return(true)); - packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.AddSendRtpModule(&rtp_2, false); + packet_router_.AddSendRtpModule(&rtp_1, false); + packet_router_.AddSendRtpModule(&rtp_2, false); const size_t kPaddingSize = 123; const size_t kExpectedPaddingPackets = 1; @@ -287,187 +123,244 @@ TEST(PacketRouterTest, GeneratePaddingPicksCorrectModule) { return std::vector>( kExpectedPaddingPackets); }); - auto generated_padding = packet_router.GeneratePadding(kPaddingSize); + auto generated_padding = packet_router_.GeneratePadding(kPaddingSize); EXPECT_EQ(generated_padding.size(), kExpectedPaddingPackets); - packet_router.RemoveSendRtpModule(&rtp_1); - packet_router.RemoveSendRtpModule(&rtp_2); + packet_router_.RemoveSendRtpModule(&rtp_1); + packet_router_.RemoveSendRtpModule(&rtp_2); } -TEST(PacketRouterTest, PadsOnLastActiveMediaStream) { - PacketRouter packet_router; - +TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { 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, SupportsPadding).WillRepeatedly(Return(true)); EXPECT_CALL(rtp_1, SupportsRtxPayloadPadding).WillRepeatedly(Return(true)); + EXPECT_CALL(rtp_1, TrySendPacket).WillRepeatedly(Return(false)); + EXPECT_CALL( + rtp_1, + TrySendPacket( + ::testing::Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc1)), _)) + .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, SupportsPadding).WillRepeatedly(Return(true)); EXPECT_CALL(rtp_2, SupportsRtxPayloadPadding).WillRepeatedly(Return(true)); + EXPECT_CALL(rtp_2, TrySendPacket).WillRepeatedly(Return(false)); + EXPECT_CALL( + rtp_2, + TrySendPacket( + ::testing::Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc2)), _)) + .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, SupportsPadding).WillRepeatedly(Return(true)); - EXPECT_CALL(rtp_3, SupportsRtxPayloadPadding).WillRepeatedly(Return(true)); + EXPECT_CALL(rtp_3, SupportsRtxPayloadPadding).WillRepeatedly(Return(false)); + EXPECT_CALL(rtp_3, TrySendPacket).WillRepeatedly(Return(false)); + EXPECT_CALL( + rtp_3, + TrySendPacket( + ::testing::Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc3)), _)) + .WillRepeatedly(Return(true)); - packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.AddSendRtpModule(&rtp_2, false); - packet_router.AddSendRtpModule(&rtp_3, false); + 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, _)) + EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce(Return(kPaddingBytes)); - packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); + }); + packet_router_.GeneratePadding(kPaddingBytes); // 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()); + packet_router_.SendPacket(BuildRtpPacket(kSsrc1), PacedPacketInfo()); - EXPECT_CALL(rtp_1, TimeToSendPadding(kPaddingBytes, _)) + EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce(Return(kPaddingBytes)); - packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); + }); + packet_router_.GeneratePadding(kPaddingBytes); // Send media on second module. Padding should be sent there. - EXPECT_CALL(rtp_2, TimeToSendPacket(kSsrc2, _, _, _, _)); - packet_router.TimeToSendPacket(kSsrc2, 0, 0, false, PacedPacketInfo()); + packet_router_.SendPacket(BuildRtpPacket(kSsrc2), PacedPacketInfo()); - EXPECT_CALL(rtp_2, TimeToSendPadding(kPaddingBytes, _)) + EXPECT_CALL(rtp_2, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce(Return(kPaddingBytes)); - packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); + }); + packet_router_.GeneratePadding(kPaddingBytes); // Remove second module, padding should now fall back to first module. - packet_router.RemoveSendRtpModule(&rtp_2); - EXPECT_CALL(rtp_1, TimeToSendPadding(kPaddingBytes, _)) + packet_router_.RemoveSendRtpModule(&rtp_2); + EXPECT_CALL(rtp_1, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce(Return(kPaddingBytes)); - packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); + }); + packet_router_.GeneratePadding(kPaddingBytes); // Remove first module too, leaving only the one without rtx. - packet_router.RemoveSendRtpModule(&rtp_1); + packet_router_.RemoveSendRtpModule(&rtp_1); - EXPECT_CALL(rtp_3, TimeToSendPadding(kPaddingBytes, _)) + EXPECT_CALL(rtp_3, GeneratePadding(kPaddingBytes)) .Times(1) - .WillOnce(Return(kPaddingBytes)); - packet_router.TimeToSendPadding(kPaddingBytes, PacedPacketInfo()); + .WillOnce([](size_t target_size_bytes) { + return std::vector>(); + }); + packet_router_.GeneratePadding(kPaddingBytes); - packet_router.RemoveSendRtpModule(&rtp_3); + packet_router_.RemoveSendRtpModule(&rtp_3); } -TEST(PacketRouterTest, SenderOnlyFunctionsRespectSendingMedia) { - PacketRouter packet_router; - NiceMock rtp; - packet_router.AddSendRtpModule(&rtp, false); - static const uint16_t kSsrc = 1234; - EXPECT_CALL(rtp, SSRC()).WillRepeatedly(Return(kSsrc)); - EXPECT_CALL(rtp, SendingMedia()).WillRepeatedly(Return(false)); - - // Verify that TimeToSendPacket does not end up in a receiver. - EXPECT_CALL(rtp, TimeToSendPacket(_, _, _, _, _)).Times(0); - EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, - packet_router.TimeToSendPacket( - kSsrc, 1, 1, false, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); - // Verify that TimeToSendPadding does not end up in a receiver. - EXPECT_CALL(rtp, TimeToSendPadding(_, _)).Times(0); - EXPECT_EQ(0u, packet_router.TimeToSendPadding( - 200, PacedPacketInfo(PacedPacketInfo::kNotAProbe, - kProbeMinBytes, kProbeMinBytes))); - - packet_router.RemoveSendRtpModule(&rtp); -} - -TEST(PacketRouterTest, AllocateSequenceNumbers) { - PacketRouter packet_router; - +TEST_F(PacketRouterTest, AllocateSequenceNumbers) { const uint16_t kStartSeq = 0xFFF0; const size_t kNumPackets = 32; - packet_router.SetTransportWideSequenceNumber(kStartSeq - 1); + packet_router_.SetTransportWideSequenceNumber(kStartSeq - 1); for (size_t i = 0; i < kNumPackets; ++i) { - uint16_t seq = packet_router.AllocateSequenceNumber(); + uint16_t seq = packet_router_.AllocateSequenceNumber(); uint32_t expected_unwrapped_seq = static_cast(kStartSeq) + i; EXPECT_EQ(static_cast(expected_unwrapped_seq & 0xFFFF), seq); } } -TEST(PacketRouterTest, SendTransportFeedback) { - PacketRouter packet_router; +TEST_F(PacketRouterTest, SendTransportFeedback) { NiceMock rtp_1; NiceMock rtp_2; - packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.AddReceiveRtpModule(&rtp_2, false); + packet_router_.AddSendRtpModule(&rtp_1, false); + packet_router_.AddReceiveRtpModule(&rtp_2, false); rtcp::TransportFeedback feedback; EXPECT_CALL(rtp_1, SendFeedbackPacket(_)).Times(1).WillOnce(Return(true)); - packet_router.SendTransportFeedback(&feedback); - packet_router.RemoveSendRtpModule(&rtp_1); + packet_router_.SendTransportFeedback(&feedback); + packet_router_.RemoveSendRtpModule(&rtp_1); EXPECT_CALL(rtp_2, SendFeedbackPacket(_)).Times(1).WillOnce(Return(true)); - packet_router.SendTransportFeedback(&feedback); - packet_router.RemoveReceiveRtpModule(&rtp_2); + packet_router_.SendTransportFeedback(&feedback); + packet_router_.RemoveReceiveRtpModule(&rtp_2); +} + +TEST_F(PacketRouterTest, SendPacketWithoutTransportSequenceNumbers) { + NiceMock rtp_1; + packet_router_.AddSendRtpModule(&rtp_1, false); + + const uint16_t kSsrc1 = 1234; + ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(true)); + ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); + + // Send a packet without TransportSequenceNumber extension registered, + // packets sent should not have the extension set. + RtpHeaderExtensionMap extension_manager; + auto packet = absl::make_unique(&extension_manager); + packet->SetSsrc(kSsrc1); + EXPECT_CALL( + rtp_1, + TrySendPacket( + Property(&RtpPacketToSend::HasExtension, + false), + _)) + .WillOnce(Return(true)); + packet_router_.SendPacket(std::move(packet), PacedPacketInfo()); + + packet_router_.RemoveSendRtpModule(&rtp_1); +} + +TEST_F(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) { + NiceMock rtp_1; + NiceMock rtp_2; + + packet_router_.AddSendRtpModule(&rtp_1, false); + packet_router_.AddSendRtpModule(&rtp_2, false); + + const uint16_t kSsrc1 = 1234; + const uint16_t kSsrc2 = 2345; + + ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); + ON_CALL(rtp_2, SSRC).WillByDefault(Return(kSsrc2)); + + // Transport sequence numbers start at 1, for historical reasons. + uint16_t transport_sequence_number = 1; + + auto packet = BuildRtpPacket(kSsrc1); + EXPECT_TRUE(packet->ReserveExtension()); + EXPECT_CALL( + rtp_1, + TrySendPacket( + Property(&RtpPacketToSend::GetExtension, + transport_sequence_number), + _)) + .WillOnce(Return(true)); + packet_router_.SendPacket(std::move(packet), PacedPacketInfo()); + + ++transport_sequence_number; + packet = BuildRtpPacket(kSsrc2); + EXPECT_TRUE(packet->ReserveExtension()); + + // There will be a failed attempt to send on kSsrc1 before trying + // the correct RTP module. + EXPECT_CALL(rtp_1, TrySendPacket).WillOnce(Return(false)); + EXPECT_CALL( + rtp_2, + TrySendPacket( + Property(&RtpPacketToSend::GetExtension, + transport_sequence_number), + _)) + .WillOnce(Return(true)); + packet_router_.SendPacket(std::move(packet), PacedPacketInfo()); + + packet_router_.RemoveSendRtpModule(&rtp_1); + packet_router_.RemoveSendRtpModule(&rtp_2); } #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -TEST(PacketRouterTest, DoubleRegistrationOfSendModuleDisallowed) { - PacketRouter packet_router; +TEST_F(PacketRouterTest, DoubleRegistrationOfSendModuleDisallowed) { NiceMock module; constexpr bool remb_candidate = false; // Value irrelevant. - packet_router.AddSendRtpModule(&module, remb_candidate); - EXPECT_DEATH(packet_router.AddSendRtpModule(&module, remb_candidate), ""); + packet_router_.AddSendRtpModule(&module, remb_candidate); + EXPECT_DEATH(packet_router_.AddSendRtpModule(&module, remb_candidate), ""); // Test tear-down - packet_router.RemoveSendRtpModule(&module); + packet_router_.RemoveSendRtpModule(&module); } -TEST(PacketRouterTest, DoubleRegistrationOfReceiveModuleDisallowed) { - PacketRouter packet_router; +TEST_F(PacketRouterTest, DoubleRegistrationOfReceiveModuleDisallowed) { NiceMock module; constexpr bool remb_candidate = false; // Value irrelevant. - packet_router.AddReceiveRtpModule(&module, remb_candidate); - EXPECT_DEATH(packet_router.AddReceiveRtpModule(&module, remb_candidate), ""); + packet_router_.AddReceiveRtpModule(&module, remb_candidate); + EXPECT_DEATH(packet_router_.AddReceiveRtpModule(&module, remb_candidate), ""); // Test tear-down - packet_router.RemoveReceiveRtpModule(&module); + packet_router_.RemoveReceiveRtpModule(&module); } -TEST(PacketRouterTest, RemovalOfNeverAddedSendModuleDisallowed) { - PacketRouter packet_router; +TEST_F(PacketRouterTest, RemovalOfNeverAddedSendModuleDisallowed) { NiceMock module; - EXPECT_DEATH(packet_router.RemoveSendRtpModule(&module), ""); + EXPECT_DEATH(packet_router_.RemoveSendRtpModule(&module), ""); } -TEST(PacketRouterTest, RemovalOfNeverAddedReceiveModuleDisallowed) { - PacketRouter packet_router; +TEST_F(PacketRouterTest, RemovalOfNeverAddedReceiveModuleDisallowed) { NiceMock module; - EXPECT_DEATH(packet_router.RemoveReceiveRtpModule(&module), ""); + EXPECT_DEATH(packet_router_.RemoveReceiveRtpModule(&module), ""); } #endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) @@ -931,87 +824,4 @@ TEST(PacketRouterRembTest, ReceiveModuleTakesOverWhenLastSendModuleRemoved) { packet_router.RemoveReceiveRtpModule(&receive_module); } -TEST(PacketRouterTest, SendPacketWithoutTransportSequenceNumbers) { - PacketRouter packet_router; - NiceMock rtp_1; - packet_router.AddSendRtpModule(&rtp_1, false); - - const uint16_t kSsrc1 = 1234; - ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); - - // Send a packet without TransportSequenceNumber extension registered, - // packets sent should not have the extension set. - RtpHeaderExtensionMap extension_manager; - auto packet = absl::make_unique(&extension_manager); - packet->SetSsrc(kSsrc1); - EXPECT_CALL( - rtp_1, - TrySendPacket( - Property(&RtpPacketToSend::HasExtension, - false), - _)) - .WillOnce(Return(true)); - packet_router.SendPacket(std::move(packet), PacedPacketInfo()); - - packet_router.RemoveSendRtpModule(&rtp_1); -} - -TEST(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) { - PacketRouter packet_router; - NiceMock rtp_1; - NiceMock rtp_2; - - packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.AddSendRtpModule(&rtp_2, false); - - const uint16_t kSsrc1 = 1234; - const uint16_t kSsrc2 = 2345; - - ON_CALL(rtp_1, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_1, SSRC).WillByDefault(Return(kSsrc1)); - ON_CALL(rtp_2, SendingMedia).WillByDefault(Return(true)); - ON_CALL(rtp_2, SSRC).WillByDefault(Return(kSsrc2)); - - RtpHeaderExtensionMap extension_manager; - const int kTransportSequenceNumberExtensionId = 1; - extension_manager.Register(kRtpExtensionTransportSequenceNumber, - kTransportSequenceNumberExtensionId); - - // Transport sequence numbers start at 1, for historical reasons. - uint16_t transport_sequence_number = 1; - - auto packet = absl::make_unique(&extension_manager); - EXPECT_TRUE(packet->ReserveExtension()); - packet->SetSsrc(kSsrc1); - EXPECT_CALL( - rtp_1, - TrySendPacket( - Property(&RtpPacketToSend::GetExtension, - transport_sequence_number), - _)) - .WillOnce(Return(true)); - packet_router.SendPacket(std::move(packet), PacedPacketInfo()); - - ++transport_sequence_number; - packet = absl::make_unique(&extension_manager); - EXPECT_TRUE(packet->ReserveExtension()); - packet->SetSsrc(kSsrc2); - - // There will be a failed attempt to send on kSsrc1 before trying - // the correct RTP module. - EXPECT_CALL(rtp_1, TrySendPacket).WillOnce(Return(false)); - EXPECT_CALL( - rtp_2, - TrySendPacket( - Property(&RtpPacketToSend::GetExtension, - transport_sequence_number), - _)) - .WillOnce(Return(true)); - packet_router.SendPacket(std::move(packet), PacedPacketInfo()); - - packet_router.RemoveSendRtpModule(&rtp_1); - packet_router.RemoveSendRtpModule(&rtp_2); -} - } // namespace webrtc diff --git a/modules/rtp_rtcp/include/rtp_packet_sender.h b/modules/rtp_rtcp/include/rtp_packet_sender.h index 493ec1b98b..9b7b23ef23 100644 --- a/modules/rtp_rtcp/include/rtp_packet_sender.h +++ b/modules/rtp_rtcp/include/rtp_packet_sender.h @@ -18,28 +18,10 @@ namespace webrtc { -// TODO(bugs.webrtc.org/10633): Remove Priority and InsertPacket when old pacer -// code path is gone. class RtpPacketSender { public: virtual ~RtpPacketSender() = default; - // These are part of the legacy PacedSender interface and will be removed. - enum Priority { - kHighPriority = 0, // Pass through; will be sent immediately. - kNormalPriority = 2, // Put in back of the line. - kLowPriority = 3, // Put in back of the low priority line. - }; - - // Adds the packet information to the queue and call TimeToSendPacket when - // it's time to send. - virtual void InsertPacket(Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission) = 0; - // Insert packet into queue, for eventual transmission. Based on the type of // the packet, it will be prioritized and scheduled relative to other packets // and the current target send rate. diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 5ace64b717..1d9f2ad6c3 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -282,22 +282,12 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { int payload_type, bool force_sender_report) = 0; - virtual RtpPacketSendResult TimeToSendPacket( - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) = 0; - // Try to send the provided packet. Returns true iff packet matches any of // the SSRCs for this module (media/rtx/fec etc) and was forwarded to the // transport. virtual bool TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) = 0; - virtual size_t TimeToSendPadding(size_t bytes, - const PacedPacketInfo& pacing_info) = 0; - virtual std::vector> GeneratePadding( size_t target_size_bytes) = 0; diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 1a9a0c48b5..347d4cee0f 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -85,17 +85,9 @@ class MockRtpRtcp : public RtpRtcp { MOCK_CONST_METHOD1(EstimatedReceiveBandwidth, int(uint32_t* available_bandwidth)); MOCK_METHOD4(OnSendingRtpFrame, bool(uint32_t, int64_t, int, bool)); - MOCK_METHOD5(TimeToSendPacket, - RtpPacketSendResult(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info)); MOCK_METHOD2(TrySendPacket, bool(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info)); - MOCK_METHOD2(TimeToSendPadding, - size_t(size_t bytes, const PacedPacketInfo& pacing_info)); MOCK_METHOD1( GeneratePadding, std::vector>(size_t target_size_bytes)); diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index d63d8032df..e6542a9ce0 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -314,7 +314,7 @@ std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket( StoredPacket* best_packet = *best_packet_it; if (best_packet->pending_transmission_) { // Because PacedSender releases it's lock when it calls - // TimeToSendPadding() there is the potential for a race where a new + // GeneratePadding() there is the potential for a race where a new // packet ends up here instead of the regular transmit path. In such a // case, just return empty and it will be picked up on the next // Process() call. diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 06573bfd2b..d8ffd22fe7 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -354,16 +354,6 @@ bool ModuleRtpRtcpImpl::OnSendingRtpFrame(uint32_t timestamp, return true; } -RtpPacketSendResult ModuleRtpRtcpImpl::TimeToSendPacket( - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) { - return rtp_sender_->TimeToSendPacket(ssrc, sequence_number, capture_time_ms, - retransmission, pacing_info); -} - bool ModuleRtpRtcpImpl::TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) { return rtp_sender_->TrySendPacket(packet, pacing_info); @@ -377,12 +367,6 @@ bool ModuleRtpRtcpImpl::SupportsRtxPayloadPadding() const { return rtp_sender_->SupportsRtxPayloadPadding(); } -size_t ModuleRtpRtcpImpl::TimeToSendPadding( - size_t bytes, - const PacedPacketInfo& pacing_info) { - return rtp_sender_->TimeToSendPadding(bytes, pacing_info); -} - std::vector> ModuleRtpRtcpImpl::GeneratePadding(size_t target_size_bytes) { return rtp_sender_->GeneratePadding(target_size_bytes); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index b34b145d9f..f5d184e223 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -135,20 +135,9 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { int payload_type, bool force_sender_report) override; - RtpPacketSendResult TimeToSendPacket( - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) override; - bool TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) override; - // Returns the number of padding bytes actually sent, which can be more or - // less than |bytes|. - size_t TimeToSendPadding(size_t bytes, - const PacedPacketInfo& pacing_info) override; std::vector> GeneratePadding( size_t target_size_bytes) override; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 08858e2f6a..b53d4655b8 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -471,17 +471,6 @@ void RTPSender::OnReceivedNack( } } -// Called from pacer when we can send the packet. -RtpPacketSendResult RTPSender::TimeToSendPacket( - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) { - RTC_NOTREACHED(); - return RtpPacketSendResult::kSuccess; -} - // Called from pacer when we can send the packet. bool RTPSender::TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info) { @@ -715,19 +704,6 @@ void RTPSender::UpdateRtpStats(const RtpPacketToSend& packet, rtp_stats_callback_->DataCountersUpdated(*counters, packet.Ssrc()); } -size_t RTPSender::TimeToSendPadding(size_t bytes, - const PacedPacketInfo& pacing_info) { - // TODO(bugs.webrtc.org/10633): Remove when downstream test usage is gone. - size_t padding_bytes_sent = 0; - for (auto& packet : GeneratePadding(bytes)) { - const size_t packet_size = packet->payload_size() + packet->padding_size(); - if (TrySendPacket(packet.get(), pacing_info)) { - padding_bytes_sent += packet_size; - } - } - return padding_bytes_sent; -} - std::vector> RTPSender::GeneratePadding( size_t target_size_bytes) { // This method does not actually send packets, it just generates diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index f831e1b6d7..3a2d86e92a 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -111,16 +111,10 @@ class RTPSender { // Returns an RtpPacketSendResult indicating success, network unavailable, // or packet not found. - RtpPacketSendResult TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info); bool TrySendPacket(RtpPacketToSend* packet, const PacedPacketInfo& pacing_info); bool SupportsPadding() const; bool SupportsRtxPayloadPadding() const; - size_t TimeToSendPadding(size_t bytes, const PacedPacketInfo& pacing_info); std::vector> GeneratePadding( size_t target_size_bytes); diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 484711d179..67cad44d72 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -165,14 +165,6 @@ class MockRtpPacketPacer : public RtpPacketSender { MOCK_METHOD1(EnqueuePacket, void(std::unique_ptr)); - MOCK_METHOD6(InsertPacket, - void(Priority priority, - uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - size_t bytes, - bool retransmission)); - MOCK_METHOD2(CreateProbeCluster, void(int bitrate_bps, int cluster_id)); MOCK_METHOD0(Pause, void()); @@ -296,6 +288,15 @@ class RtpSenderTest : public ::testing::TestWithParam { return SendPacket(kCaptureTimeMs, sizeof(kPayloadData)); } + size_t GenerateAndSendPadding(size_t target_size_bytes) { + size_t generated_bytes = 0; + for (auto& packet : rtp_sender_->GeneratePadding(target_size_bytes)) { + generated_bytes += packet->payload_size() + packet->padding_size(); + rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); + } + return generated_bytes; + } + // The following are helpers for configuring the RTPSender. They must be // called before sending any packets. @@ -399,16 +400,16 @@ TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberMayAllowPaddingOnVideo) { auto packet = rtp_sender_->AllocatePacket(); ASSERT_TRUE(packet); - ASSERT_FALSE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); + ASSERT_TRUE(rtp_sender_->GeneratePadding(kPaddingSize).empty()); packet->SetMarker(false); ASSERT_TRUE(rtp_sender_->AssignSequenceNumber(packet.get())); // Packet without marker bit doesn't allow padding on video stream. - EXPECT_FALSE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); + ASSERT_TRUE(rtp_sender_->GeneratePadding(kPaddingSize).empty()); packet->SetMarker(true); ASSERT_TRUE(rtp_sender_->AssignSequenceNumber(packet.get())); // Packet with marker bit allows send padding. - EXPECT_TRUE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); + ASSERT_FALSE(rtp_sender_->GeneratePadding(kPaddingSize).empty()); } TEST_P(RtpSenderTest, AssignSequenceNumberAllowsPaddingOnAudio) { @@ -434,15 +435,13 @@ TEST_P(RtpSenderTest, AssignSequenceNumberAllowsPaddingOnAudio) { const size_t kPaddingSize = 59; EXPECT_CALL(transport, SendRtp(_, kPaddingSize + kRtpHeaderSize, _)) .WillOnce(Return(true)); - EXPECT_EQ(kPaddingSize, - rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); + EXPECT_EQ(kPaddingSize, GenerateAndSendPadding(kPaddingSize)); // Requested padding size is too small, will send a larger one. const size_t kMinPaddingSize = 50; EXPECT_CALL(transport, SendRtp(_, kMinPaddingSize + kRtpHeaderSize, _)) .WillOnce(Return(true)); - EXPECT_EQ(kMinPaddingSize, rtp_sender_->TimeToSendPadding(kMinPaddingSize - 5, - PacedPacketInfo())); + EXPECT_EQ(kMinPaddingSize, GenerateAndSendPadding(kMinPaddingSize - 5)); } TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberSetPaddingTimestamps) { @@ -453,11 +452,11 @@ TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberSetPaddingTimestamps) { packet->SetTimestamp(kTimestamp); ASSERT_TRUE(rtp_sender_->AssignSequenceNumber(packet.get())); - ASSERT_TRUE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); + auto padding_packets = rtp_sender_->GeneratePadding(kPaddingSize); - ASSERT_EQ(1u, transport_.sent_packets_.size()); + ASSERT_EQ(1u, padding_packets.size()); // Verify padding packet timestamp. - EXPECT_EQ(kTimestamp, transport_.last_sent_packet().Timestamp()); + EXPECT_EQ(kTimestamp, padding_packets[0]->Timestamp()); } TEST_P(RtpSenderTestWithoutPacer, @@ -1003,8 +1002,7 @@ TEST_P(RtpSenderTest, SendPadding) { const size_t kPaddingBytes = 100; const size_t kMaxPaddingLength = 224; // Value taken from rtp_sender.cc. // Padding will be forced to full packets. - EXPECT_EQ(kMaxPaddingLength, - rtp_sender_->TimeToSendPadding(kPaddingBytes, PacedPacketInfo())); + EXPECT_EQ(kMaxPaddingLength, GenerateAndSendPadding(kPaddingBytes)); // Process send bucket. Padding should now be sent. EXPECT_EQ(++total_packets_sent, transport_.packets_sent()); @@ -1878,7 +1876,7 @@ TEST_P(RtpSenderTestWithoutPacer, StreamDataCountersCallbacks) { callback.Matches(ssrc, expected); // Send padding. - rtp_sender_->TimeToSendPadding(kMaxPaddingSize, PacedPacketInfo()); + GenerateAndSendPadding(kMaxPaddingSize); expected.transmitted.payload_bytes = 12; expected.transmitted.header_bytes = 36; expected.transmitted.padding_bytes = kMaxPaddingSize; @@ -1913,8 +1911,8 @@ TEST_P(RtpSenderTestWithoutPacer, BytesReportedCorrectly) { SendGenericPacket(); // Will send 2 full-size padding packets. - rtp_sender_->TimeToSendPadding(1, PacedPacketInfo()); - rtp_sender_->TimeToSendPadding(1, PacedPacketInfo()); + GenerateAndSendPadding(1); + GenerateAndSendPadding(1); StreamDataCounters rtp_stats; StreamDataCounters rtx_stats;