diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 0bfb043448..eda3ecdee5 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -29,20 +29,13 @@ constexpr int64_t RtpPacketHistory::kMinPacketDurationMs; constexpr int RtpPacketHistory::kMinPacketDurationRtt; constexpr int RtpPacketHistory::kPacketCullingDelayFactor; -RtpPacketHistory::PacketState::PacketState() = default; -RtpPacketHistory::PacketState::PacketState(const PacketState&) = default; -RtpPacketHistory::PacketState::~PacketState() = default; - RtpPacketHistory::StoredPacket::StoredPacket( std::unique_ptr packet, - absl::optional send_time_ms, + int64_t send_time_ms, uint64_t insert_order) : send_time_ms_(send_time_ms), packet_(std::move(packet)), - // No send time indicates packet is not sent immediately, but instead will - // be put in the pacer queue and later retrieved via - // GetPacketAndSetSendTime(). - pending_transmission_(!send_time_ms.has_value()), + pending_transmission_(false), insert_order_(insert_order), times_retransmitted_(0) {} @@ -120,7 +113,7 @@ void RtpPacketHistory::SetRtt(int64_t rtt_ms) { } void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, - absl::optional send_time_ms) { + int64_t send_time_ms) { RTC_DCHECK(packet); MutexLock lock(&lock_); int64_t now_ms = clock_->TimeInMilliseconds(); @@ -145,11 +138,11 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, // Packet to be inserted ahead of first packet, expand front. for (; packet_index < 0; ++packet_index) { - packet_history_.emplace_front(nullptr, absl::nullopt, 0); + packet_history_.emplace_front(); } // Packet to be inserted behind last packet, expand back. while (static_cast(packet_history_.size()) <= packet_index) { - packet_history_.emplace_back(nullptr, absl::nullopt, 0); + packet_history_.emplace_back(); } RTC_DCHECK_GE(packet_index, 0); @@ -168,36 +161,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, } } -std::unique_ptr RtpPacketHistory::GetPacketAndSetSendTime( - uint16_t sequence_number) { - MutexLock lock(&lock_); - if (mode_ == StorageMode::kDisabled) { - return nullptr; - } - - StoredPacket* packet = GetStoredPacket(sequence_number); - if (packet == nullptr) { - return nullptr; - } - - int64_t now_ms = clock_->TimeInMilliseconds(); - if (!VerifyRtt(*packet, now_ms)) { - return nullptr; - } - - if (packet->send_time_ms_) { - packet->IncrementTimesRetransmitted( - enable_padding_prio_ ? &padding_priority_ : nullptr); - } - - // Update send-time and mark as no long in pacer queue. - packet->send_time_ms_ = now_ms; - packet->pending_transmission_ = false; - - // Return copy of packet instance since it may need to be retransmitted. - return std::make_unique(*packet->packet_); -} - std::unique_ptr RtpPacketHistory::GetPacketAndMarkAsPending( uint16_t sequence_number) { return GetPacketAndMarkAsPending( @@ -251,8 +214,6 @@ void RtpPacketHistory::MarkPacketAsSent(uint16_t sequence_number) { return; } - RTC_DCHECK(packet->send_time_ms_); - // Update send-time, mark as no longer in pacer queue, and increment // transmission count. packet->send_time_ms_ = clock_->TimeInMilliseconds(); @@ -261,41 +222,37 @@ void RtpPacketHistory::MarkPacketAsSent(uint16_t sequence_number) { : nullptr); } -absl::optional RtpPacketHistory::GetPacketState( - uint16_t sequence_number) const { +bool RtpPacketHistory::GetPacketState(uint16_t sequence_number) const { MutexLock lock(&lock_); if (mode_ == StorageMode::kDisabled) { - return absl::nullopt; + return false; } int packet_index = GetPacketIndex(sequence_number); if (packet_index < 0 || static_cast(packet_index) >= packet_history_.size()) { - return absl::nullopt; + return false; } const StoredPacket& packet = packet_history_[packet_index]; if (packet.packet_ == nullptr) { - return absl::nullopt; + return false; } if (!VerifyRtt(packet, clock_->TimeInMilliseconds())) { - return absl::nullopt; + return false; } - return StoredPacketToPacketState(packet); + return true; } bool RtpPacketHistory::VerifyRtt(const RtpPacketHistory::StoredPacket& packet, int64_t now_ms) const { - if (packet.send_time_ms_) { - // Send-time already set, this check must be for a retransmission. - if (packet.times_retransmitted() > 0 && - now_ms < *packet.send_time_ms_ + rtt_ms_) { - // This packet has already been retransmitted once, and the time since - // that even is lower than on RTT. Ignore request as this packet is - // likely already in the network pipe. - return false; - } + if (packet.times_retransmitted() > 0 && + now_ms < packet.send_time_ms_ + rtt_ms_) { + // This packet has already been retransmitted once, and the time since + // that even is lower than on RTT. Ignore request as this packet is + // likely already in the network pipe. + return false; } return true; @@ -368,21 +325,6 @@ void RtpPacketHistory::CullAcknowledgedPackets( } } -bool RtpPacketHistory::SetPendingTransmission(uint16_t sequence_number) { - MutexLock lock(&lock_); - if (mode_ == StorageMode::kDisabled) { - return false; - } - - StoredPacket* packet = GetStoredPacket(sequence_number); - if (packet == nullptr) { - return false; - } - - packet->pending_transmission_ = true; - return true; -} - void RtpPacketHistory::Clear() { MutexLock lock(&lock_); Reset(); @@ -410,13 +352,13 @@ void RtpPacketHistory::CullOldPackets(int64_t now_ms) { return; } - if (*stored_packet.send_time_ms_ + packet_duration_ms > now_ms) { + if (stored_packet.send_time_ms_ + packet_duration_ms > now_ms) { // Don't cull packets too early to avoid failed retransmission requests. return; } if (packet_history_.size() >= number_to_store_ || - *stored_packet.send_time_ms_ + + stored_packet.send_time_ms_ + (packet_duration_ms * kPacketCullingDelayFactor) <= now_ms) { // Too many packets in history, or this packet has timed out. Remove it @@ -487,17 +429,4 @@ RtpPacketHistory::StoredPacket* RtpPacketHistory::GetStoredPacket( return &packet_history_[index]; } -RtpPacketHistory::PacketState RtpPacketHistory::StoredPacketToPacketState( - const RtpPacketHistory::StoredPacket& stored_packet) { - RtpPacketHistory::PacketState state; - state.rtp_sequence_number = stored_packet.packet_->SequenceNumber(); - state.send_time_ms = stored_packet.send_time_ms_; - state.capture_time_ms = stored_packet.packet_->capture_time().ms(); - state.ssrc = stored_packet.packet_->Ssrc(); - state.packet_size = stored_packet.packet_->size(); - state.times_retransmitted = stored_packet.times_retransmitted(); - state.pending_transmission = stored_packet.pending_transmission_; - return state; -} - } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index f87ad4d550..18cef0aa13 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -35,22 +35,6 @@ class RtpPacketHistory { // packets as they time out or as signaled as received. }; - // Snapshot indicating the state of a packet in the history. - struct PacketState { - PacketState(); - PacketState(const PacketState&); - ~PacketState(); - - uint16_t rtp_sequence_number = 0; - absl::optional send_time_ms; - int64_t capture_time_ms = 0; - uint32_t ssrc = 0; - size_t packet_size = 0; - // Number of times RE-transmitted, ie not including the first transmission. - size_t times_retransmitted = 0; - bool pending_transmission = false; - }; - // Maximum number of packets we ever allow in the history. static constexpr size_t kMaxCapacity = 9600; // Maximum number of entries in prioritized queue of padding packets. @@ -78,15 +62,8 @@ class RtpPacketHistory { // a packet in the history before we are reasonably sure it has been received. void SetRtt(int64_t rtt_ms); - // If `send_time` is set, packet was sent without using pacer, so state will - // be set accordingly. void PutRtpPacket(std::unique_ptr packet, - absl::optional send_time_ms); - - // Gets stored RTP packet corresponding to the input |sequence number|. - // Returns nullptr if packet is not found or was (re)sent too recently. - std::unique_ptr GetPacketAndSetSendTime( - uint16_t sequence_number); + int64_t send_time_ms); // Gets stored RTP packet corresponding to the input |sequence number|. // Returns nullptr if packet is not found or was (re)sent too recently. @@ -109,9 +86,9 @@ class RtpPacketHistory { // counter. Marks the packet as no longer being in the pacer queue. void MarkPacketAsSent(uint16_t sequence_number); - // Similar to GetPacketAndSetSendTime(), but only returns a snapshot of the - // current state for packet, and never updates internal state. - absl::optional GetPacketState(uint16_t sequence_number) const; + // Returns true if history contains packet with `sequence_number` and it can + // be retransmitted. + bool GetPacketState(uint16_t sequence_number) const; // Get the packet (if any) from the history, that is deemed most likely to // the remote side. This is calculated from heuristics such as packet age @@ -130,11 +107,6 @@ class RtpPacketHistory { // Cull packets that have been acknowledged as received by the remote end. void CullAcknowledgedPackets(rtc::ArrayView sequence_numbers); - // Mark packet as queued for transmission. This will prevent premature - // removal or duplicate retransmissions in the pacer queue. - // Returns true if status was set, false if packet was not found. - bool SetPendingTransmission(uint16_t sequence_number); - // Remove all pending packets from the history, but keep storage mode and // capacity. void Clear(); @@ -146,8 +118,9 @@ class RtpPacketHistory { class StoredPacket { public: + StoredPacket() = default; StoredPacket(std::unique_ptr packet, - absl::optional send_time_ms, + int64_t send_time_ms, uint64_t insert_order); StoredPacket(StoredPacket&&); StoredPacket& operator=(StoredPacket&&); @@ -158,7 +131,7 @@ class RtpPacketHistory { void IncrementTimesRetransmitted(PacketPrioritySet* priority_set); // The time of last transmission, including retransmissions. - absl::optional send_time_ms_; + int64_t send_time_ms_; // The actual packet. std::unique_ptr packet_; @@ -178,8 +151,7 @@ class RtpPacketHistory { bool operator()(StoredPacket* lhs, StoredPacket* rhs) const; }; - // Helper method used by GetPacketAndSetSendTime() and GetPacketState() to - // check if packet has too recently been sent. + // Helper method to check if packet has too recently been sent. bool VerifyRtt(const StoredPacket& packet, int64_t now_ms) const RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_); void Reset() RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_); @@ -192,8 +164,6 @@ class RtpPacketHistory { RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_); StoredPacket* GetStoredPacket(uint16_t sequence_number) RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_); - static PacketState StoredPacketToPacketState( - const StoredPacket& stored_packet); Clock* const clock_; const bool enable_padding_prio_; diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index ef9ce593b5..ffe167c5f6 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -63,8 +63,8 @@ TEST_P(RtpPacketHistoryTest, SetStoreStatus) { TEST_P(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - // Store a packet, but with send-time. It should then not be removed. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), + fake_clock_.TimeInMilliseconds()); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Changing store status, even to the current one, will clear the history. @@ -74,17 +74,19 @@ TEST_P(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { TEST_P(RtpPacketHistoryTest, StartSeqResetAfterReset) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - // Store a packet, but with send-time. It should then not be removed. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), absl::nullopt); - EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), + fake_clock_.TimeInMilliseconds()); + // Mark packet as pending so it won't be removed. + EXPECT_TRUE(hist_.GetPacketAndMarkAsPending(kStartSeqNum)); // Changing store status, to clear the history. hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); // Add a new packet. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), absl::nullopt); - EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); + hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), + fake_clock_.TimeInMilliseconds()); + EXPECT_TRUE(hist_.GetPacketAndMarkAsPending(To16u(kStartSeqNum + 1))); // Advance time past where packet expires. fake_clock_.AdvanceTimeMilliseconds( @@ -92,7 +94,8 @@ TEST_P(RtpPacketHistoryTest, StartSeqResetAfterReset) { RtpPacketHistory::kMinPacketDurationMs); // Add one more packet and verify no state left from packet before reset. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 2)), absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 2)), + fake_clock_.TimeInMilliseconds()); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); @@ -101,7 +104,7 @@ TEST_P(RtpPacketHistoryTest, StartSeqResetAfterReset) { TEST_P(RtpPacketHistoryTest, NoStoreStatus) { EXPECT_EQ(StorageMode::kDisabled, hist_.GetStorageMode()); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - hist_.PutRtpPacket(std::move(packet), absl::nullopt); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); // Packet should not be stored. EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } @@ -116,7 +119,7 @@ TEST_P(RtpPacketHistoryTest, PutRtpPacket) { std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); - hist_.PutRtpPacket(std::move(packet), absl::nullopt); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); } @@ -126,88 +129,16 @@ TEST_P(RtpPacketHistoryTest, GetRtpPacket) { std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); packet->set_capture_time(capture_time); rtc::CopyOnWriteBuffer buffer = packet->Buffer(); - hist_.PutRtpPacket(std::move(packet), absl::nullopt); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); std::unique_ptr packet_out = - hist_.GetPacketAndSetSendTime(kStartSeqNum); - EXPECT_TRUE(packet_out); + hist_.GetPacketAndMarkAsPending(kStartSeqNum); + ASSERT_TRUE(packet_out); EXPECT_EQ(buffer, packet_out->Buffer()); EXPECT_EQ(capture_time, packet_out->capture_time()); } -TEST_P(RtpPacketHistoryTest, PacketStateIsCorrect) { - const uint32_t kSsrc = 92384762; - const int64_t kRttMs = 100; - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - hist_.SetRtt(kRttMs); - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - packet->SetSsrc(kSsrc); - packet->SetPayloadSize(1234); - const size_t packet_size = packet->size(); - - hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); - - absl::optional state = - hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(state); - EXPECT_EQ(state->rtp_sequence_number, kStartSeqNum); - EXPECT_EQ(state->send_time_ms, fake_clock_.TimeInMilliseconds()); - EXPECT_EQ(state->capture_time_ms, fake_clock_.TimeInMilliseconds()); - EXPECT_EQ(state->ssrc, kSsrc); - EXPECT_EQ(state->packet_size, packet_size); - EXPECT_EQ(state->times_retransmitted, 0u); - - fake_clock_.AdvanceTimeMilliseconds(1); - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); - fake_clock_.AdvanceTimeMilliseconds(kRttMs + 1); - - state = hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(state); - EXPECT_EQ(state->times_retransmitted, 1u); -} - -TEST_P(RtpPacketHistoryTest, MinResendTimeWithPacer) { - static const int64_t kMinRetransmitIntervalMs = 100; - - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - hist_.SetRtt(kMinRetransmitIntervalMs); - int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - size_t len = packet->size(); - hist_.PutRtpPacket(std::move(packet), absl::nullopt); - - // First transmission: TimeToSendPacket() call from pacer. - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); - - // First retransmission - allow early retransmission. - fake_clock_.AdvanceTimeMilliseconds(1); - - // With pacer there's two calls to history: - // 1) When the NACK request arrived, use GetPacketState() to see if the - // packet is there and verify RTT constraints. Then we use the ssrc - // and sequence number to enqueue the retransmission in the pacer - // 2) When the pacer determines that it is time to send the packet, it calls - // GetPacketAndSetSendTime(). - absl::optional packet_state = - hist_.GetPacketState(kStartSeqNum); - EXPECT_TRUE(packet_state); - EXPECT_EQ(len, packet_state->packet_size); - EXPECT_EQ(capture_time_ms, packet_state->capture_time_ms); - - // Retransmission was allowed, next send it from pacer. - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); - - // Second retransmission - advance time to just before retransmission OK. - fake_clock_.AdvanceTimeMilliseconds(kMinRetransmitIntervalMs - 1); - EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); - - // Advance time to just after retransmission OK. - fake_clock_.AdvanceTimeMilliseconds(1); - EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); -} - -TEST_P(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { +TEST_P(RtpPacketHistoryTest, MinResendTime) { static const int64_t kMinRetransmitIntervalMs = 100; hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); @@ -219,18 +150,19 @@ TEST_P(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { // First retransmission - allow early retransmission. fake_clock_.AdvanceTimeMilliseconds(1); - packet = hist_.GetPacketAndSetSendTime(kStartSeqNum); - EXPECT_TRUE(packet); + packet = hist_.GetPacketAndMarkAsPending(kStartSeqNum); + ASSERT_TRUE(packet); EXPECT_EQ(len, packet->size()); EXPECT_EQ(packet->capture_time(), capture_time); + hist_.MarkPacketAsSent(kStartSeqNum); // Second retransmission - advance time to just before retransmission OK. fake_clock_.AdvanceTimeMilliseconds(kMinRetransmitIntervalMs - 1); - EXPECT_FALSE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); + EXPECT_FALSE(hist_.GetPacketAndMarkAsPending(kStartSeqNum)); // Advance time to just after retransmission OK. fake_clock_.AdvanceTimeMilliseconds(1); - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); + EXPECT_TRUE(hist_.GetPacketAndMarkAsPending(kStartSeqNum)); } TEST_P(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { @@ -275,8 +207,9 @@ TEST_P(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { for (size_t i = 0; i < kMaxNumPackets; ++i) { std::unique_ptr packet = CreateRtpPacket(To16u(kStartSeqNum + i)); - // Don't mark packets as sent, preventing them from being removed. - hist_.PutRtpPacket(std::move(packet), absl::nullopt); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); + // Mark packets as pending, preventing it from being removed. + hist_.GetPacketAndMarkAsPending(To16u(kStartSeqNum + i)); } // First packet should still be there. @@ -329,39 +262,6 @@ TEST_P(RtpPacketHistoryTest, RemovesLowestPrioPaddingWhenAtMaxCapacity) { EXPECT_EQ(packet->SequenceNumber(), To16u(kStartSeqNum + kMaxNumPackets)); } -TEST_P(RtpPacketHistoryTest, DontRemoveUnsentPackets) { - const size_t kMaxNumPackets = 10; - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); - - // Add packets until the buffer is full. - for (size_t i = 0; i < kMaxNumPackets; ++i) { - // Mark packets as unsent. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + i)), absl::nullopt); - } - fake_clock_.AdvanceTimeMilliseconds(RtpPacketHistory::kMinPacketDurationMs); - - // First packet should still be there. - EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); - - // History is full, but old packets not sent, so allow expansion. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + kMaxNumPackets)), - fake_clock_.TimeInMilliseconds()); - EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); - - // Set all packet as sent and advance time past min packet duration time, - // otherwise packets till still be prevented from being removed. - for (size_t i = 0; i <= kMaxNumPackets; ++i) { - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(To16u(kStartSeqNum + i))); - } - fake_clock_.AdvanceTimeMilliseconds(RtpPacketHistory::kMinPacketDurationMs); - // Add a new packet, this means the two oldest ones will be culled. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + kMaxNumPackets + 1)), - fake_clock_.TimeInMilliseconds()); - EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); - EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); - EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); -} - TEST_P(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { // Set size to remove old packets as soon as possible. hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); @@ -476,29 +376,26 @@ TEST_P(RtpPacketHistoryTest, CullWithAcks) { std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); packet->SetPayloadSize(50); hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); - hist_.GetPacketAndSetSendTime(kStartSeqNum); fake_clock_.AdvanceTimeMilliseconds(33); packet = CreateRtpPacket(To16u(kStartSeqNum + 1)); packet->SetPayloadSize(50); hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); - hist_.GetPacketAndSetSendTime(To16u(kStartSeqNum + 1)); fake_clock_.AdvanceTimeMilliseconds(33); packet = CreateRtpPacket(To16u(kStartSeqNum + 2)); packet->SetPayloadSize(50); hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); - hist_.GetPacketAndSetSendTime(To16u(kStartSeqNum + 2)); - EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum).has_value()); - EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1)).has_value()); - EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2)).has_value()); + EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); + EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); + EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); // Remove middle one using ack, check that only that one is gone. std::vector acked_sequence_numbers = {To16u(kStartSeqNum + 1)}; hist_.CullAcknowledgedPackets(acked_sequence_numbers); - EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum).has_value()); - EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1)).has_value()); - EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2)).has_value()); + EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); + EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); + EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); // Advance time to where second packet would have expired, verify first packet // is removed. @@ -506,58 +403,16 @@ TEST_P(RtpPacketHistoryTest, CullWithAcks) { fake_clock_.AdvanceTimeMilliseconds(second_packet_expiry_time - fake_clock_.TimeInMilliseconds()); hist_.SetRtt(1); // Trigger culling of old packets. - EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum).has_value()); - EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1)).has_value()); - EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2)).has_value()); + EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); + EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); + EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); // Advance to where last packet expires, verify all gone. fake_clock_.AdvanceTimeMilliseconds(33); hist_.SetRtt(1); // Trigger culling of old packets. - EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum).has_value()); - EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1)).has_value()); - EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 2)).has_value()); -} - -TEST_P(RtpPacketHistoryTest, SetsPendingTransmissionState) { - const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; - hist_.SetRtt(kRttMs); - - // Set size to remove old packets as soon as possible. - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); - - // Add a packet, without send time, indicating it's in pacer queue. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), - /* send_time_ms = */ absl::nullopt); - - // Packet is pending transmission. - absl::optional packet_state = - hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(packet_state.has_value()); - EXPECT_TRUE(packet_state->pending_transmission); - - // Packet sent, state should be back to non-pending. - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); - packet_state = hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(packet_state.has_value()); - EXPECT_FALSE(packet_state->pending_transmission); - - // Time for a retransmission. - fake_clock_.AdvanceTimeMilliseconds(kRttMs); - EXPECT_TRUE(hist_.SetPendingTransmission(kStartSeqNum)); - packet_state = hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(packet_state.has_value()); - EXPECT_TRUE(packet_state->pending_transmission); - - // Packet sent. - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); - // Too early for retransmission. - ASSERT_FALSE(hist_.GetPacketState(kStartSeqNum).has_value()); - - // Retransmission allowed again, it's not in a pending state. - fake_clock_.AdvanceTimeMilliseconds(kRttMs); - packet_state = hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(packet_state.has_value()); - EXPECT_FALSE(packet_state->pending_transmission); + EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); + EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); + EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); } TEST_P(RtpPacketHistoryTest, GetPacketAndSetSent) { @@ -647,21 +502,17 @@ TEST_P(RtpPacketHistoryTest, DontRemovePendingTransmissions) { // Advance clock to just before packet timeout. fake_clock_.AdvanceTimeMilliseconds(kPacketTimeoutMs - 1); // Mark as enqueued in pacer. - EXPECT_TRUE(hist_.SetPendingTransmission(kStartSeqNum)); + EXPECT_TRUE(hist_.GetPacketAndMarkAsPending(kStartSeqNum)); // Advance clock to where packet would have timed out. It should still // be there and pending. fake_clock_.AdvanceTimeMilliseconds(1); - absl::optional packet_state = - hist_.GetPacketState(kStartSeqNum); - ASSERT_TRUE(packet_state.has_value()); - EXPECT_TRUE(packet_state->pending_transmission); + EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Packet sent. Now it can be removed. - EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); + hist_.MarkPacketAsSent(kStartSeqNum); hist_.SetRtt(kRttMs); // Force culling of old packets. - packet_state = hist_.GetPacketState(kStartSeqNum); - ASSERT_FALSE(packet_state.has_value()); + EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } TEST_P(RtpPacketHistoryTest, PrioritizedPayloadPadding) { @@ -716,11 +567,11 @@ TEST_P(RtpPacketHistoryTest, NoPendingPacketAsPadding) { EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum); // If packet is pending retransmission, don't try to use it as padding. - hist_.SetPendingTransmission(kStartSeqNum); + hist_.GetPacketAndMarkAsPending(kStartSeqNum); EXPECT_EQ(nullptr, hist_.GetPayloadPaddingPacket()); // Market it as no longer pending, should be usable as padding again. - hist_.GetPacketAndSetSendTime(kStartSeqNum); + hist_.MarkPacketAsSent(kStartSeqNum); EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum); } @@ -766,29 +617,22 @@ TEST_P(RtpPacketHistoryTest, OutOfOrderInsertRemoval) { // Insert packets, out of order, including both forwards and backwards // sequence number wraps. const int seq_offsets[] = {0, 1, -1, 2, -2, 3, -3}; - const int64_t start_time_ms = fake_clock_.TimeInMilliseconds(); for (int offset : seq_offsets) { uint16_t seq_no = To16u(kStartSeqNum + offset); std::unique_ptr packet = CreateRtpPacket(seq_no); packet->SetPayloadSize(50); hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); - hist_.GetPacketAndSetSendTime(seq_no); fake_clock_.AdvanceTimeMilliseconds(33); } // Check packet are there and remove them in the same out-of-order fashion. - int64_t expected_time_offset_ms = 0; for (int offset : seq_offsets) { uint16_t seq_no = To16u(kStartSeqNum + offset); - absl::optional packet_state = - hist_.GetPacketState(seq_no); - ASSERT_TRUE(packet_state.has_value()); - EXPECT_EQ(packet_state->send_time_ms, - start_time_ms + expected_time_offset_ms); + EXPECT_TRUE(hist_.GetPacketState(seq_no)); std::vector acked_sequence_numbers = {seq_no}; hist_.CullAcknowledgedPackets(acked_sequence_numbers); - expected_time_offset_ms += 33; + EXPECT_FALSE(hist_.GetPacketState(seq_no)); } } diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index a9bc57f7ca..f644c7d64a 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -286,16 +286,7 @@ void RTPSender::SetRtxPayloadType(int payload_type, } int32_t RTPSender::ReSendPacket(uint16_t packet_id) { - // Try to find packet in RTP packet history. Also verify RTT here, so that we - // don't retransmit too often. - absl::optional stored_packet = - packet_history_->GetPacketState(packet_id); - if (!stored_packet || stored_packet->pending_transmission) { - // Packet not found or already queued for retransmission, ignore. - return 0; - } - - const int32_t packet_size = static_cast(stored_packet->packet_size); + int32_t packet_size = 0; const bool rtx = (RtxStatus() & kRtxRetransmitted) > 0; std::unique_ptr packet = @@ -304,6 +295,7 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id) { // Check if we're overusing retransmission bitrate. // TODO(sprang): Add histograms for nack success or failure // reasons. + packet_size = stored_packet.size(); std::unique_ptr retransmit_packet; if (retransmission_rate_limiter_ && !retransmission_rate_limiter_->TryUseRate(packet_size)) { @@ -321,7 +313,14 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id) { } return retransmit_packet; }); + if (packet_size == 0) { + // Packet not found or already queued for retransmission, ignore. + RTC_DCHECK(!packet); + return 0; + } if (!packet) { + // Packet was found, but lambda helper above chose not to create + // `retransmit_packet` out of it. return -1; } packet->set_packet_type(RtpPacketMediaType::kRetransmission); diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index 3d345b1727..ee7123b112 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -496,8 +496,7 @@ TEST_P(RtpSenderEgressTest, DoesNotPutNotRetransmittablePacketsInHistory) { std::unique_ptr packet = BuildRtpPacket(); packet->set_allow_retransmission(false); sender->SendPacket(packet.get(), PacedPacketInfo()); - EXPECT_FALSE( - packet_history_.GetPacketState(packet->SequenceNumber()).has_value()); + EXPECT_FALSE(packet_history_.GetPacketState(packet->SequenceNumber())); } TEST_P(RtpSenderEgressTest, PutsRetransmittablePacketsInHistory) { @@ -508,10 +507,7 @@ TEST_P(RtpSenderEgressTest, PutsRetransmittablePacketsInHistory) { 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))); + EXPECT_TRUE(packet_history_.GetPacketState(packet->SequenceNumber())); } TEST_P(RtpSenderEgressTest, DoesNotPutNonMediaInHistory) { @@ -527,22 +523,20 @@ TEST_P(RtpSenderEgressTest, DoesNotPutNonMediaInHistory) { retransmission->set_retransmitted_sequence_number( retransmission->SequenceNumber()); sender->SendPacket(retransmission.get(), PacedPacketInfo()); - EXPECT_FALSE(packet_history_.GetPacketState(retransmission->SequenceNumber()) - .has_value()); + EXPECT_FALSE( + packet_history_.GetPacketState(retransmission->SequenceNumber())); 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()); + EXPECT_FALSE(packet_history_.GetPacketState(fec->SequenceNumber())); 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()); + EXPECT_FALSE(packet_history_.GetPacketState(padding->SequenceNumber())); } TEST_P(RtpSenderEgressTest, UpdatesSendStatusOfRetransmittedPackets) { @@ -554,10 +548,7 @@ TEST_P(RtpSenderEgressTest, UpdatesSendStatusOfRetransmittedPackets) { 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))); + EXPECT_TRUE(packet_history_.GetPacketState(media_packet->SequenceNumber())); // Simulate a retransmission, marking the packet as pending. std::unique_ptr retransmission = @@ -565,16 +556,11 @@ TEST_P(RtpSenderEgressTest, UpdatesSendStatusOfRetransmittedPackets) { 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))); + EXPECT_TRUE(packet_history_.GetPacketState(media_packet->SequenceNumber())); // 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))); + EXPECT_TRUE(packet_history_.GetPacketState(media_packet->SequenceNumber())); } TEST_P(RtpSenderEgressTest, StreamDataCountersCallbacks) {