From 36005afeb470bc1a54e6fd6af5ce7ca4341941df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 24 May 2021 18:50:04 +0200 Subject: [PATCH] Refactor and improve RtpSender packet history test. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL refactors RtpSenderTest.SendPacketHandlesRetransmissionHistory, moves some testing to rtp_ender_egress_unittest and adds test coverage for a few cases. Bug: webrtc:11340 Change-Id: Ic225d2af43c3926f69fe3ea45f41b18c29b8b4fd Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219796 Commit-Queue: Erik Språng Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#34111} --- modules/rtp_rtcp/source/rtp_packet_to_send.h | 4 +- .../source/rtp_sender_egress_unittest.cc | 88 +++++++++++++++++++ .../rtp_rtcp/source/rtp_sender_unittest.cc | 50 +++++++---- 3 files changed, 122 insertions(+), 20 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_packet_to_send.h b/modules/rtp_rtcp/source/rtp_packet_to_send.h index 2411deac49..12341ef6cf 100644 --- a/modules/rtp_rtcp/source/rtp_packet_to_send.h +++ b/modules/rtp_rtcp/source/rtp_packet_to_send.h @@ -59,14 +59,14 @@ class RtpPacketToSend : public RtpPacket { void set_retransmitted_sequence_number(uint16_t sequence_number) { retransmitted_sequence_number_ = sequence_number; } - absl::optional retransmitted_sequence_number() { + absl::optional retransmitted_sequence_number() const { return retransmitted_sequence_number_; } void set_allow_retransmission(bool allow_retransmission) { allow_retransmission_ = allow_retransmission; } - bool allow_retransmission() { return allow_retransmission_; } + bool allow_retransmission() const { return allow_retransmission_; } // An application can attach arbitrary data to an RTP packet using // `additional_data`. The additional data does not affect WebRTC processing. diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index ab8d85e757..fa6bd0064b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -34,6 +34,7 @@ namespace { using ::testing::_; using ::testing::Field; using ::testing::NiceMock; +using ::testing::Optional; using ::testing::StrictMock; constexpr Timestamp kStartTime = Timestamp::Millis(123456789); @@ -478,6 +479,93 @@ TEST_P(RtpSenderEgressTest, BitrateCallbacks) { } } +TEST_P(RtpSenderEgressTest, DoesNotPutNotRetransmittablePacketsInHistory) { + std::unique_ptr sender = CreateRtpSenderEgress(); + packet_history_.SetStorePacketsStatus( + RtpPacketHistory::StorageMode::kStoreAndCull, 10); + + std::unique_ptr packet = BuildRtpPacket(); + packet->set_allow_retransmission(false); + sender->SendPacket(packet.get(), PacedPacketInfo()); + EXPECT_FALSE( + packet_history_.GetPacketState(packet->SequenceNumber()).has_value()); +} + +TEST_P(RtpSenderEgressTest, PutsRetransmittablePacketsInHistory) { + std::unique_ptr sender = CreateRtpSenderEgress(); + packet_history_.SetStorePacketsStatus( + RtpPacketHistory::StorageMode::kStoreAndCull, 10); + + std::unique_ptr packet = BuildRtpPacket(); + packet->set_allow_retransmission(true); + sender->SendPacket(packet.get(), PacedPacketInfo()); + EXPECT_THAT( + packet_history_.GetPacketState(packet->SequenceNumber()), + Optional( + Field(&RtpPacketHistory::PacketState::pending_transmission, false))); +} + +TEST_P(RtpSenderEgressTest, DoesNotPutNonMediaInHistory) { + std::unique_ptr sender = CreateRtpSenderEgress(); + packet_history_.SetStorePacketsStatus( + RtpPacketHistory::StorageMode::kStoreAndCull, 10); + + // Non-media packets, even when marked as retransmittable, are not put into + // the packet history. + std::unique_ptr retransmission = BuildRtpPacket(); + retransmission->set_allow_retransmission(true); + retransmission->set_packet_type(RtpPacketMediaType::kRetransmission); + sender->SendPacket(retransmission.get(), PacedPacketInfo()); + EXPECT_FALSE(packet_history_.GetPacketState(retransmission->SequenceNumber()) + .has_value()); + + std::unique_ptr fec = BuildRtpPacket(); + fec->set_allow_retransmission(true); + fec->set_packet_type(RtpPacketMediaType::kForwardErrorCorrection); + sender->SendPacket(fec.get(), PacedPacketInfo()); + EXPECT_FALSE( + packet_history_.GetPacketState(fec->SequenceNumber()).has_value()); + + std::unique_ptr padding = BuildRtpPacket(); + padding->set_allow_retransmission(true); + padding->set_packet_type(RtpPacketMediaType::kPadding); + sender->SendPacket(padding.get(), PacedPacketInfo()); + EXPECT_FALSE( + packet_history_.GetPacketState(padding->SequenceNumber()).has_value()); +} + +TEST_P(RtpSenderEgressTest, UpdatesSendStatusOfRetransmittedPackets) { + std::unique_ptr sender = CreateRtpSenderEgress(); + packet_history_.SetStorePacketsStatus( + RtpPacketHistory::StorageMode::kStoreAndCull, 10); + + // Send a packet, putting it in the history. + std::unique_ptr media_packet = BuildRtpPacket(); + media_packet->set_allow_retransmission(true); + sender->SendPacket(media_packet.get(), PacedPacketInfo()); + EXPECT_THAT( + packet_history_.GetPacketState(media_packet->SequenceNumber()), + Optional( + Field(&RtpPacketHistory::PacketState::pending_transmission, false))); + + // Simulate a retransmission, marking the packet as pending. + std::unique_ptr retransmission = + packet_history_.GetPacketAndMarkAsPending(media_packet->SequenceNumber()); + retransmission->set_retransmitted_sequence_number( + media_packet->SequenceNumber()); + retransmission->set_packet_type(RtpPacketMediaType::kRetransmission); + EXPECT_THAT(packet_history_.GetPacketState(media_packet->SequenceNumber()), + Optional(Field( + &RtpPacketHistory::PacketState::pending_transmission, true))); + + // Simulate packet leaving pacer, the packet should be marked as non-pending. + sender->SendPacket(retransmission.get(), PacedPacketInfo()); + EXPECT_THAT( + packet_history_.GetPacketState(media_packet->SequenceNumber()), + Optional( + Field(&RtpPacketHistory::PacketState::pending_transmission, false))); +} + INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderEgressTest, ::testing::Values(TestConfig(false), diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 67d36d6748..f4c1ca1fe6 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1366,38 +1366,52 @@ TEST_P(RtpSenderTest, SendPacketHandlesRetransmissionHistory) { // Ignore calls to EnqueuePackets() for this test. EXPECT_CALL(mock_paced_sender_, EnqueuePackets).WillRepeatedly(Return()); - // Build a media packet and send it. + // Build a media packet and put in the packet history. std::unique_ptr packet = BuildRtpPacket(kPayload, true, 0, clock_->TimeInMilliseconds()); const uint16_t media_sequence_number = packet->SequenceNumber(); - packet->set_packet_type(RtpPacketMediaType::kVideo); packet->set_allow_retransmission(true); - rtp_sender_context_->InjectPacket(std::move(packet), PacedPacketInfo()); + rtp_sender_context_->packet_history_.PutRtpPacket( + std::move(packet), clock_->TimeInMilliseconds()); - // Simulate retransmission request. + // Simulate successful retransmission request. time_controller_.AdvanceTime(TimeDelta::Millis(30)); - EXPECT_GT(rtp_sender()->ReSendPacket(media_sequence_number), 0); + EXPECT_THAT(rtp_sender()->ReSendPacket(media_sequence_number), Gt(0)); // Packet already pending, retransmission not allowed. time_controller_.AdvanceTime(TimeDelta::Millis(30)); - EXPECT_EQ(rtp_sender()->ReSendPacket(media_sequence_number), 0); + EXPECT_THAT(rtp_sender()->ReSendPacket(media_sequence_number), Eq(0)); - // Packet exiting pacer, mark as not longer pending. - packet = BuildRtpPacket(kPayload, true, 0, clock_->TimeInMilliseconds()); - EXPECT_NE(packet->SequenceNumber(), media_sequence_number); - packet->set_packet_type(RtpPacketMediaType::kRetransmission); - packet->SetSsrc(kRtxSsrc); - packet->set_retransmitted_sequence_number(media_sequence_number); - packet->set_allow_retransmission(false); - uint16_t seq_no = packet->SequenceNumber(); - rtp_sender_context_->InjectPacket(std::move(packet), PacedPacketInfo()); + // Simulate packet exiting pacer, mark as not longer pending. + rtp_sender_context_->packet_history_.MarkPacketAsSent(media_sequence_number); // Retransmissions allowed again. time_controller_.AdvanceTime(TimeDelta::Millis(30)); - EXPECT_GT(rtp_sender()->ReSendPacket(media_sequence_number), 0); + EXPECT_THAT(rtp_sender()->ReSendPacket(media_sequence_number), Gt(0)); +} - // Retransmission of RTX packet should not be allowed. - EXPECT_EQ(rtp_sender()->ReSendPacket(seq_no), 0); +TEST_P(RtpSenderTest, MarksRetransmittedPackets) { + rtp_sender_context_->packet_history_.SetStorePacketsStatus( + RtpPacketHistory::StorageMode::kStoreAndCull, 10); + + // Build a media packet and put in the packet history. + std::unique_ptr packet = + BuildRtpPacket(kPayload, true, 0, clock_->TimeInMilliseconds()); + const uint16_t media_sequence_number = packet->SequenceNumber(); + packet->set_allow_retransmission(true); + rtp_sender_context_->packet_history_.PutRtpPacket( + std::move(packet), clock_->TimeInMilliseconds()); + + // Expect a retransmission packet marked with which packet it is a + // retransmit of. + EXPECT_CALL( + mock_paced_sender_, + EnqueuePackets(ElementsAre(AllOf( + Pointee(Property(&RtpPacketToSend::packet_type, + RtpPacketMediaType::kRetransmission)), + Pointee(Property(&RtpPacketToSend::retransmitted_sequence_number, + Eq(media_sequence_number))))))); + EXPECT_THAT(rtp_sender()->ReSendPacket(media_sequence_number), Gt(0)); } TEST_P(RtpSenderTest, SendPacketUpdatesExtensions) {