diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 71302f1108..27dfc5a78b 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -371,11 +371,14 @@ void PacedSender::Process() { break; critsect_.Leave(); - bool success = packet_sender_->TimeToSendPacket( + RtpPacketSendResult success = packet_sender_->TimeToSendPacket( packet->ssrc, packet->sequence_number, packet->capture_time_ms, packet->retransmission, pacing_info); critsect_.Enter(); - if (success) { + if (success == RtpPacketSendResult::kSuccess || + success == RtpPacketSendResult::kPacketNotFound) { + // Packet sent or invalid packet, remove it from queue. + // TODO(webrtc:8052): Don't consume media budget on kInvalid. bytes_sent += packet->bytes; // Send succeeded, remove it from the queue. OnPacketSent(packet); @@ -443,10 +446,6 @@ void PacedSender::OnPacketSent(const RoundRobinPacketQueue::Packet* packet) { bool audio_packet = packet->priority == kHighPriority; if (!audio_packet || account_for_audio_) { // Update media bytes sent. - // TODO(eladalon): TimeToSendPacket() can also return |true| in some - // situations where nothing actually ended up being sent to the network, - // and we probably don't want to update the budget in such cases. - // https://bugs.chromium.org/p/webrtc/issues/detail?id=8052 UpdateBudgetWithBytesSent(packet->bytes); last_send_time_us_ = clock_->TimeInMicroseconds(); } diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 8f9f4138ef..58c1200f58 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -44,11 +44,12 @@ class PacedSender : public Pacer { // module again. // Called when it's time to send a queued packet. // Returns false if packet cannot be sent. - virtual bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& cluster_info) = 0; + virtual RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& cluster_info) = 0; // Called when it's a good time to send a padding data. // Returns the number of bytes sent. virtual size_t TimeToSendPadding(size_t bytes, diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 234a29f56d..2e430693ff 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -43,11 +43,11 @@ static const int kTargetBitrateBps = 800000; class MockPacedSenderCallback : public PacedSender::PacketSender { public: MOCK_METHOD5(TimeToSendPacket, - bool(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info)); + 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)); }; @@ -56,12 +56,13 @@ class PacedSenderPadding : public PacedSender::PacketSender { public: PacedSenderPadding() : padding_sent_(0) {} - bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) override { - return true; + RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) override { + return RtpPacketSendResult::kSuccess; } size_t TimeToSendPadding(size_t bytes, @@ -82,13 +83,14 @@ class PacedSenderProbing : public PacedSender::PacketSender { public: PacedSenderProbing() : packets_sent_(0), padding_sent_(0) {} - bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) override { + RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) override { packets_sent_++; - return true; + return RtpPacketSendResult::kSuccess; } size_t TimeToSendPadding(size_t bytes, @@ -134,7 +136,7 @@ class PacedSenderTest : public ::testing::TestWithParam { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number, capture_time_ms, retransmission, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); } SimulatedClock clock_; MockPacedSenderCallback callback_; @@ -175,7 +177,8 @@ TEST_F(PacedSenderFieldTrialTest, DefaultNoPaddingInSilence) { pacer.SetPacingRates(kTargetBitrateBps, 0); // Video packet to reset last send time and provide padding data. InsertPacket(&pacer, &video); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); clock_.AdvanceTimeMilliseconds(5); pacer.Process(); EXPECT_CALL(callback_, TimeToSendPadding).Times(0); @@ -190,7 +193,8 @@ TEST_F(PacedSenderFieldTrialTest, PaddingInSilenceWithTrial) { pacer.SetPacingRates(kTargetBitrateBps, 0); // Video packet to reset last send time and provide padding data. InsertPacket(&pacer, &video); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); clock_.AdvanceTimeMilliseconds(5); pacer.Process(); EXPECT_CALL(callback_, TimeToSendPadding).WillOnce(Return(1000)); @@ -207,7 +211,8 @@ TEST_F(PacedSenderFieldTrialTest, DefaultCongestionWindowAffectsAudio) { pacer.UpdateOutstandingData(0); // Video packet fills congestion window. InsertPacket(&pacer, &video); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); // Audio packet blocked due to congestion. InsertPacket(&pacer, &audio); @@ -217,7 +222,8 @@ TEST_F(PacedSenderFieldTrialTest, DefaultCongestionWindowAffectsAudio) { // Audio packet unblocked when congestion window clear. ::testing::Mock::VerifyAndClearExpectations(&callback_); pacer.UpdateOutstandingData(0); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); } @@ -230,11 +236,13 @@ TEST_F(PacedSenderFieldTrialTest, CongestionWindowDoesNotAffectAudioInTrial) { pacer.UpdateOutstandingData(0); // Video packet fills congestion window. InsertPacket(&pacer, &video); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); // Audio not blocked due to congestion. InsertPacket(&pacer, &audio); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); } @@ -244,7 +252,8 @@ TEST_F(PacedSenderFieldTrialTest, DefaultBudgetAffectsAudio) { 0); // Video fills budget for following process periods. InsertPacket(&pacer, &video); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); // Audio packet blocked due to budget limit. EXPECT_CALL(callback_, TimeToSendPacket).Times(0); @@ -253,7 +262,8 @@ TEST_F(PacedSenderFieldTrialTest, DefaultBudgetAffectsAudio) { ProcessNext(&pacer); ::testing::Mock::VerifyAndClearExpectations(&callback_); // Audio packet unblocked when the budget has recovered. - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); ProcessNext(&pacer); } @@ -266,10 +276,12 @@ TEST_F(PacedSenderFieldTrialTest, BudgetDoesNotAffectAudioInTrial) { 0); // Video fills budget for following process periods. InsertPacket(&pacer, &video); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); ProcessNext(&pacer); // Audio packet not blocked due to budget limit. - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); InsertPacket(&pacer, &audio); ProcessNext(&pacer); } @@ -322,7 +334,7 @@ TEST_F(PacedSenderTest, QueuePacket) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number++, queued_packet_timestamp, false, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); send_bucket_->Process(); sequence_number++; EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); @@ -371,7 +383,7 @@ TEST_F(PacedSenderTest, PaceQueuedPackets) { clock_.AdvanceTimeMilliseconds(5); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, _, false, _)) .Times(packets_to_send_per_interval) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); send_bucket_->Process(); } @@ -571,7 +583,7 @@ TEST_F(PacedSenderTest, Priority) { EXPECT_CALL(callback_, TimeToSendPadding(_, _)).Times(0); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, capture_time_ms, false, _)) .Times(packets_to_send_per_interval + 1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); @@ -583,7 +595,7 @@ TEST_F(PacedSenderTest, Priority) { TimeToSendPacket(ssrc_low_priority, _, capture_time_ms_low_priority, false, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); @@ -622,7 +634,7 @@ TEST_F(PacedSenderTest, RetransmissionPriority) { EXPECT_CALL(callback_, TimeToSendPacket( ssrc, _, capture_time_ms_retransmission, true, _)) .Times(packets_to_send_per_interval) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); @@ -635,7 +647,7 @@ TEST_F(PacedSenderTest, RetransmissionPriority) { EXPECT_CALL(callback_, TimeToSendPacket(_, _, _, true, _)).Times(0); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, capture_time_ms, false, _)) .Times(packets_to_send_per_interval) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); @@ -676,7 +688,7 @@ TEST_F(PacedSenderTest, HighPrioDoesntAffectBudget) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number++, capture_time_ms, false, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); send_bucket_->Process(); @@ -736,7 +748,8 @@ TEST_F(PacedSenderTest, DoesNotAllowOveruseAfterCongestion) { send_bucket_->UpdateOutstandingData(0); // Not yet budget limited or congested, packet is sent. send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); clock_.AdvanceTimeMilliseconds(5); send_bucket_->Process(); // Packet blocked due to congestion. @@ -752,7 +765,8 @@ TEST_F(PacedSenderTest, DoesNotAllowOveruseAfterCongestion) { send_bucket_->UpdateOutstandingData(0); // Congestion removed and budget has recovered, packet is sent. send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); - EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + EXPECT_CALL(callback_, TimeToSendPacket) + .WillOnce(Return(RtpPacketSendResult::kSuccess)); clock_.AdvanceTimeMilliseconds(5); send_bucket_->Process(); send_bucket_->UpdateOutstandingData(0); @@ -799,7 +813,7 @@ TEST_F(PacedSenderTest, ResumesSendingWhenCongestionEnds) { int ack_count = kCongestionCount / 2; EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, _, false, _)) .Times(ack_count) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); send_bucket_->UpdateOutstandingData(kCongestionWindow - kPacketSize * ack_count); @@ -814,7 +828,7 @@ TEST_F(PacedSenderTest, ResumesSendingWhenCongestionEnds) { // marked as acked. EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, _, false, _)) .Times(unacked_packets) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); for (int duration = 0; duration < kCongestionTimeMs; duration += 5) { send_bucket_->UpdateOutstandingData(0); clock_.AdvanceTimeMilliseconds(5); @@ -895,34 +909,34 @@ TEST_F(PacedSenderTest, Pause) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc_high_priority, _, capture_time_ms, _, _)) .Times(packets_to_send_per_interval) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); EXPECT_CALL(callback_, TimeToSendPacket(ssrc_high_priority, _, second_capture_time_ms, _, _)) .Times(packets_to_send_per_interval) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); for (size_t i = 0; i < packets_to_send_per_interval; ++i) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, capture_time_ms, _, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); } for (size_t i = 0; i < packets_to_send_per_interval; ++i) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, second_capture_time_ms, _, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); } for (size_t i = 0; i < packets_to_send_per_interval; ++i) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc_low_priority, _, capture_time_ms, _, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); } for (size_t i = 0; i < packets_to_send_per_interval; ++i) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc_low_priority, _, second_capture_time_ms, _, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); } } send_bucket_->Resume(); @@ -961,7 +975,7 @@ TEST_F(PacedSenderTest, ResendPacket) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number, capture_time_ms, false, _)) .Times(1) - .WillOnce(Return(false)); + .WillOnce(Return(RtpPacketSendResult::kTransportUnavailable)); clock_.AdvanceTimeMilliseconds(10000); send_bucket_->Process(); @@ -973,11 +987,11 @@ TEST_F(PacedSenderTest, ResendPacket) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number, capture_time_ms, false, _)) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(RtpPacketSendResult::kSuccess)); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number + 1, capture_time_ms + 1, false, _)) .Times(1) - .WillOnce(Return(false)); + .WillOnce(Return(RtpPacketSendResult::kTransportUnavailable)); clock_.AdvanceTimeMilliseconds(10000); send_bucket_->Process(); @@ -989,7 +1003,7 @@ TEST_F(PacedSenderTest, ResendPacket) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number + 1, capture_time_ms + 1, false, _)) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(RtpPacketSendResult::kSuccess)); clock_.AdvanceTimeMilliseconds(10000); send_bucket_->Process(); EXPECT_EQ(0, send_bucket_->QueueInMs()); @@ -1183,7 +1197,7 @@ TEST_F(PacedSenderTest, AverageQueueTime) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number, first_capture_time, false, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); send_bucket_->Process(); EXPECT_EQ(10, send_bucket_->AverageQueueTimeMs()); @@ -1192,7 +1206,7 @@ TEST_F(PacedSenderTest, AverageQueueTime) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number + 1, first_capture_time + 10, false, _)) .Times(1) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); for (int i = 0; i < 3; ++i) { clock_.AdvanceTimeMilliseconds(30); // Max delta. send_bucket_->Process(); @@ -1221,7 +1235,7 @@ TEST_F(PacedSenderTest, ProbeClusterId) { TimeToSendPacket(_, _, _, _, Field(&PacedPacketInfo::probe_cluster_id, 0))) .Times(5) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); for (int i = 0; i < 5; ++i) { clock_.AdvanceTimeMilliseconds(20); send_bucket_->Process(); @@ -1232,7 +1246,7 @@ TEST_F(PacedSenderTest, ProbeClusterId) { TimeToSendPacket(_, _, _, _, Field(&PacedPacketInfo::probe_cluster_id, 1))) .Times(5) - .WillRepeatedly(Return(true)); + .WillRepeatedly(Return(RtpPacketSendResult::kSuccess)); for (int i = 0; i < 5; ++i) { clock_.AdvanceTimeMilliseconds(20); send_bucket_->Process(); @@ -1262,7 +1276,7 @@ TEST_F(PacedSenderTest, AvoidBusyLoopOnSendFailure) { kPacketSize, false); EXPECT_CALL(callback_, TimeToSendPacket(_, _, _, _, _)) - .WillOnce(Return(true)); + .WillOnce(Return(RtpPacketSendResult::kSuccess)); send_bucket_->Process(); EXPECT_EQ(10, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(9); diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index d674e22d57..a7c2b939c1 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -99,11 +99,12 @@ void PacketRouter::RemoveReceiveRtpModule( rtcp_feedback_senders_.erase(it); } -bool PacketRouter::TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_timestamp, - bool retransmission, - const PacedPacketInfo& pacing_info) { +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_); for (auto* rtp_module : rtp_send_modules_) { if (!rtp_module->SendingMedia()) { @@ -121,7 +122,7 @@ bool PacketRouter::TimeToSendPacket(uint32_t ssrc, pacing_info); } } - return true; + return RtpPacketSendResult::kPacketNotFound; } size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send, diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 3362298b20..23a4c4aad9 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -52,11 +52,12 @@ class PacketRouter : public PacedSender::PacketSender, void RemoveReceiveRtpModule(RtcpFeedbackSenderInterface* rtcp_sender); // Implements PacedSender::Callback. - bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_timestamp, - bool retransmission, - const PacedPacketInfo& packet_info) override; + RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_timestamp, + bool retransmission, + const PacedPacketInfo& packet_info) override; size_t TimeToSendPadding(size_t bytes, const PacedPacketInfo& packet_info) override; diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index b3d91e6bcb..df7619afad 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -54,11 +54,9 @@ TEST(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPacket) { constexpr bool retransmission = false; const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes); - // TODO(eladalon): TimeToSendPacket() returning true when nothing was - // sent, because no modules were registered, is sub-optimal. - // https://bugs.chromium.org/p/webrtc/issues/detail?id=8052 - EXPECT_TRUE(packet_router.TimeToSendPacket(ssrc, sequence_number, timestamp, - retransmission, paced_info)); + EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, + packet_router.TimeToSendPacket(ssrc, sequence_number, timestamp, + retransmission, paced_info)); } TEST(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPadding) { @@ -116,11 +114,12 @@ TEST(PacketRouterTest, TimeToSendPacket) { kSsrc1, sequence_number, timestamp, retransmission, Field(&PacedPacketInfo::probe_cluster_id, 1))) .Times(1) - .WillOnce(Return(true)); + .WillOnce(Return(RtpPacketSendResult::kSuccess)); EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _, _)).Times(0); - EXPECT_TRUE(packet_router.TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); + 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; @@ -135,19 +134,21 @@ TEST(PacketRouterTest, TimeToSendPacket) { kSsrc2, sequence_number, timestamp, retransmission, Field(&PacedPacketInfo::probe_cluster_id, 2))) .Times(1) - .WillOnce(Return(true)); - EXPECT_TRUE(packet_router.TimeToSendPacket( - kSsrc2, sequence_number, timestamp, retransmission, - PacedPacketInfo(2, kProbeMinProbes, kProbeMinBytes))); + .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. EXPECT_CALL(rtp_1, SendingMedia()).Times(1).WillOnce(Return(false)); EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _, _, _)).Times(0); EXPECT_CALL(rtp_2, SendingMedia()).Times(1).WillOnce(Return(false)); EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _, _)).Times(0); - EXPECT_TRUE(packet_router.TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); + 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. EXPECT_CALL(rtp_1, SendingMedia()).Times(1).WillOnce(Return(true)); @@ -156,9 +157,10 @@ TEST(PacketRouterTest, TimeToSendPacket) { EXPECT_CALL(rtp_2, SSRC()).Times(1).WillOnce(Return(kSsrc2)); EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _, _, _)).Times(0); EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _, _)).Times(0); - EXPECT_TRUE(packet_router.TimeToSendPacket( - kSsrc1 + kSsrc2, sequence_number, timestamp, retransmission, - PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); + EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, + packet_router.TimeToSendPacket( + kSsrc1 + kSsrc2, sequence_number, timestamp, retransmission, + PacedPacketInfo(1, kProbeMinProbes, kProbeMinBytes))); packet_router.RemoveSendRtpModule(&rtp_1); @@ -167,10 +169,11 @@ TEST(PacketRouterTest, TimeToSendPacket) { EXPECT_CALL(rtp_2, SendingMedia()).Times(1).WillOnce(Return(true)); EXPECT_CALL(rtp_2, SSRC()).Times(1).WillOnce(Return(kSsrc2)); EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _, _)).Times(0); - EXPECT_TRUE(packet_router.TimeToSendPacket( - kSsrc1, sequence_number, timestamp, retransmission, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); + EXPECT_EQ(RtpPacketSendResult::kPacketNotFound, + packet_router.TimeToSendPacket( + kSsrc1, sequence_number, timestamp, retransmission, + PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, + kProbeMinBytes))); packet_router.RemoveSendRtpModule(&rtp_2); } @@ -357,10 +360,11 @@ TEST(PacketRouterTest, SenderOnlyFunctionsRespectSendingMedia) { // Verify that TimeToSendPacket does not end up in a receiver. EXPECT_CALL(rtp, TimeToSendPacket(_, _, _, _, _)).Times(0); - EXPECT_TRUE(packet_router.TimeToSendPacket( - kSsrc, 1, 1, false, - PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes, - kProbeMinBytes))); + 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( diff --git a/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc b/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc index 5ba384be16..620fd1bb0c 100644 --- a/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc +++ b/modules/remote_bitrate_estimator/test/bbr_paced_sender.cc @@ -130,9 +130,10 @@ void BbrPacedSender::Process() { bool BbrPacedSender::TryToSendPacket(Packet* packet) { PacedPacketInfo pacing_info; - return packet_sender_->TimeToSendPacket(packet->ssrc, packet->sequence_number, - packet->capture_time_ms, - packet->retransmission, pacing_info); + return packet_sender_->TimeToSendPacket( + packet->ssrc, packet->sequence_number, packet->capture_time_ms, + packet->retransmission, + pacing_info) != RtpPacketSendResult::kTransportUnavailable; } } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/test/packet_sender.cc b/modules/remote_bitrate_estimator/test/packet_sender.cc index 247b2e5fd6..f7cb716cd6 100644 --- a/modules/remote_bitrate_estimator/test/packet_sender.cc +++ b/modules/remote_bitrate_estimator/test/packet_sender.cc @@ -293,11 +293,12 @@ void PacedVideoSender::QueuePackets(Packets* batch, batch->merge(to_transfer, DereferencingComparator); } -bool PacedVideoSender::TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) { +RtpPacketSendResult PacedVideoSender::TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) { for (Packets::iterator it = pacer_queue_.begin(); it != pacer_queue_.end(); ++it) { MediaPacket* media_packet = static_cast(*it); @@ -306,17 +307,17 @@ bool PacedVideoSender::TimeToSendPacket(uint32_t ssrc, // Make sure a packet is never paced out earlier than when it was put into // the pacer. - assert(pace_out_time_ms >= media_packet->send_time_ms()); + RTC_CHECK_GE(pace_out_time_ms, media_packet->send_time_ms()); media_packet->SetAbsSendTimeMs(pace_out_time_ms); media_packet->set_send_time_us(1000 * pace_out_time_ms); media_packet->set_sender_timestamp_us(1000 * pace_out_time_ms); queue_.push_back(media_packet); pacer_queue_size_in_bytes_ -= media_packet->payload_size(); pacer_queue_.erase(it); - return true; + return RtpPacketSendResult::kSuccess; } } - return false; + return RtpPacketSendResult::kTransportUnavailable; } size_t PacedVideoSender::TimeToSendPadding(size_t bytes, diff --git a/modules/remote_bitrate_estimator/test/packet_sender.h b/modules/remote_bitrate_estimator/test/packet_sender.h index 1d3b355feb..085ceab381 100644 --- a/modules/remote_bitrate_estimator/test/packet_sender.h +++ b/modules/remote_bitrate_estimator/test/packet_sender.h @@ -109,11 +109,12 @@ class PacedVideoSender : public VideoSender, public PacedSender::PacketSender { void RunFor(int64_t time_ms, Packets* in_out) override; // Implements PacedSender::Callback. - bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) override; + RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) override; size_t TimeToSendPadding(size_t bytes, const PacedPacketInfo& pacing_info) override; diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 9fb26cbbea..f1d3b2c941 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -260,11 +260,12 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { int payload_type, bool force_sender_report) = 0; - virtual bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) = 0; + virtual RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) = 0; virtual size_t TimeToSendPadding(size_t bytes, const PacedPacketInfo& pacing_info) = 0; diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index d984575109..d44a366bc0 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -547,5 +547,12 @@ class SendPacketObserver { uint32_t ssrc) = 0; }; +// Status returned from TimeToSendPacket() family of callbacks. +enum class RtpPacketSendResult { + kSuccess, // Packet sent OK. + kTransportUnavailable, // Network unavailable, try again later. + kPacketNotFound // SSRC/sequence number does not map to an available packet. +}; + } // namespace webrtc #endif // MODULES_RTP_RTCP_INCLUDE_RTP_RTCP_DEFINES_H_ diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 733ec28c47..95d3cb2122 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -83,11 +83,11 @@ class MockRtpRtcp : public RtpRtcp { int(uint32_t* available_bandwidth)); MOCK_METHOD4(OnSendingRtpFrame, bool(uint32_t, int64_t, int, bool)); MOCK_METHOD5(TimeToSendPacket, - bool(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info)); + 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(RegisterRtcpObservers, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 97cc8798b2..cd7f845dd4 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -407,11 +407,12 @@ bool ModuleRtpRtcpImpl::OnSendingRtpFrame(uint32_t timestamp, return true; } -bool ModuleRtpRtcpImpl::TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) { +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); } diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index de0080013d..a4afb40ce7 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -133,11 +133,12 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { int payload_type, bool force_sender_report) override; - bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) override; + RtpPacketSendResult TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) override; // Returns the number of padding bytes actually sent, which can be more or // less than |bytes|. diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index b60114bc6b..acc2f15b36 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -538,13 +538,15 @@ void RTPSender::OnReceivedNack( } // Called from pacer when we can send the packet. -bool RTPSender::TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info) { - if (!SendingMedia()) - return true; +RtpPacketSendResult RTPSender::TimeToSendPacket( + uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info) { + if (!SendingMedia()) { + return RtpPacketSendResult::kPacketNotFound; + } std::unique_ptr packet; if (ssrc == SSRC()) { @@ -554,14 +556,16 @@ bool RTPSender::TimeToSendPacket(uint32_t ssrc, } if (!packet) { - // Packet cannot be found or was resend too recently. - return true; + // Packet cannot be found or was resent too recently. + return RtpPacketSendResult::kPacketNotFound; } return PrepareAndSendPacket( - std::move(packet), - retransmission && (RtxStatus() & kRtxRetransmitted) > 0, retransmission, - pacing_info); + std::move(packet), + retransmission && (RtxStatus() & kRtxRetransmitted) > 0, + retransmission, pacing_info) + ? RtpPacketSendResult::kSuccess + : RtpPacketSendResult::kTransportUnavailable; } bool RTPSender::PrepareAndSendPacket(std::unique_ptr packet, diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 11bb6cf9f0..cb3cdea279 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -103,11 +103,13 @@ class RTPSender : public AcknowledgedPacketsObserver { bool IsRtpHeaderExtensionRegistered(RTPExtensionType type) const; int32_t DeregisterRtpHeaderExtension(RTPExtensionType type); - bool TimeToSendPacket(uint32_t ssrc, - uint16_t sequence_number, - int64_t capture_time_ms, - bool retransmission, - const PacedPacketInfo& pacing_info); + // Returns an RtpPacketSendResult indicating succes, failure or invalid + // status such as on incorrect sequence number. + RtpPacketSendResult TimeToSendPacket(uint32_t ssrc, + uint16_t sequence_number, + int64_t capture_time_ms, + bool retransmission, + const PacedPacketInfo& pacing_info); size_t TimeToSendPadding(size_t bytes, const PacedPacketInfo& pacing_info); // NACK. diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 47f2856337..fc2fe82a36 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1180,12 +1180,14 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { EXPECT_CALL(mock_rtc_event_log_, LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) .Times(2); - EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); - EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kFlexfecSsrc, flexfec_seq_num, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); + EXPECT_EQ(RtpPacketSendResult::kSuccess, + rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum, + fake_clock_.TimeInMilliseconds(), + false, PacedPacketInfo())); + EXPECT_EQ(RtpPacketSendResult::kSuccess, + rtp_sender_->TimeToSendPacket(kFlexfecSsrc, flexfec_seq_num, + fake_clock_.TimeInMilliseconds(), + false, PacedPacketInfo())); ASSERT_EQ(2, transport_.packets_sent()); const RtpPacketReceived& media_packet = transport_.sent_packets_[0]; EXPECT_EQ(kMediaPayloadType, media_packet.PayloadType()); @@ -1258,9 +1260,10 @@ TEST_P(RtpSenderTest, NoFlexfecForTimingFrames) { EXPECT_CALL(mock_rtc_event_log_, LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) .Times(1); - EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); + EXPECT_EQ(RtpPacketSendResult::kSuccess, + rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum, + fake_clock_.TimeInMilliseconds(), + false, PacedPacketInfo())); ASSERT_EQ(1, transport_.packets_sent()); const RtpPacketReceived& media_packet = transport_.sent_packets_[0]; EXPECT_EQ(kMediaPayloadType, media_packet.PayloadType()); @@ -1284,12 +1287,14 @@ TEST_P(RtpSenderTest, NoFlexfecForTimingFrames) { EXPECT_CALL(mock_rtc_event_log_, LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) .Times(2); - EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum + 1, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); - EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kFlexfecSsrc, flexfec_seq_num, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); + EXPECT_EQ(RtpPacketSendResult::kSuccess, + rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum + 1, + fake_clock_.TimeInMilliseconds(), + false, PacedPacketInfo())); + EXPECT_EQ(RtpPacketSendResult::kSuccess, + rtp_sender_->TimeToSendPacket(kFlexfecSsrc, flexfec_seq_num, + fake_clock_.TimeInMilliseconds(), + false, PacedPacketInfo())); ASSERT_EQ(3, transport_.packets_sent()); const RtpPacketReceived& media_packet2 = transport_.sent_packets_[1]; EXPECT_EQ(kMediaPayloadType, media_packet2.PayloadType());