Refactor and improve RtpSender packet history test.

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 <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34111}

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<uint16_t> retransmitted_sequence_number() {
+  absl::optional<uint16_t> 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.

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<RtpSenderEgress> sender = CreateRtpSenderEgress();
+  packet_history_.SetStorePacketsStatus(
+      RtpPacketHistory::StorageMode::kStoreAndCull, 10);
+
+  std::unique_ptr<RtpPacketToSend> 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<RtpSenderEgress> sender = CreateRtpSenderEgress();
+  packet_history_.SetStorePacketsStatus(
+      RtpPacketHistory::StorageMode::kStoreAndCull, 10);
+
+  std::unique_ptr<RtpPacketToSend> 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<RtpSenderEgress> 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<RtpPacketToSend> 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<RtpPacketToSend> 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<RtpPacketToSend> 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<RtpSenderEgress> sender = CreateRtpSenderEgress();
+  packet_history_.SetStorePacketsStatus(
+      RtpPacketHistory::StorageMode::kStoreAndCull, 10);
+
+  // Send a packet, putting it in the history.
+  std::unique_ptr<RtpPacketToSend> 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<RtpPacketToSend> 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),

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<RtpPacketToSend> 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.
+  // Simulate packet exiting pacer, mark as not longer pending.
-  packet = BuildRtpPacket(kPayload, true, 0, clock_->TimeInMilliseconds());
+  rtp_sender_context_->packet_history_.MarkPacketAsSent(media_sequence_number);
-  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());
 
   // 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.
+TEST_P(RtpSenderTest, MarksRetransmittedPackets) {
-  EXPECT_EQ(rtp_sender()->ReSendPacket(seq_no), 0);
+  rtp_sender_context_->packet_history_.SetStorePacketsStatus(
+      RtpPacketHistory::StorageMode::kStoreAndCull, 10);
+
+  // Build a media packet and put in the packet history.
+  std::unique_ptr<RtpPacketToSend> 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) {