From d28796209b75a2c28a1afcf4475340cdd4a6baf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Fri, 10 May 2019 08:29:01 -0700 Subject: [PATCH] Distinguish between missing packet and send failure. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL introduces three-value enum, in order to be able to distinguish between send success, send failure, and invalid states such as missing packet or invalid ssrc. The behavior is unchanged in this CL, a follow-up will change the pacer to not consume media budget on invalid states. Bug: webrtc:8052,webrtc:8975 Change-Id: I1c9e2226f995356daa538d3d3cf44945f35e0133 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/135165 Commit-Queue: Erik Språng Reviewed-by: Sebastian Jansson Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#27923} --- modules/pacing/paced_sender.cc | 11 +- modules/pacing/paced_sender.h | 11 +- modules/pacing/paced_sender_unittest.cc | 122 ++++++++++-------- modules/pacing/packet_router.cc | 13 +- modules/pacing/packet_router.h | 11 +- modules/pacing/packet_router_unittest.cc | 58 +++++---- .../test/bbr_paced_sender.cc | 7 +- .../test/packet_sender.cc | 17 +-- .../test/packet_sender.h | 11 +- modules/rtp_rtcp/include/rtp_rtcp.h | 11 +- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 7 + modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 10 +- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 11 +- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 11 +- modules/rtp_rtcp/source/rtp_sender.cc | 28 ++-- modules/rtp_rtcp/source/rtp_sender.h | 12 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 35 ++--- 17 files changed, 215 insertions(+), 171 deletions(-) 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());