From affa39cb39c77408109fef691533021533d969e1 Mon Sep 17 00:00:00 2001 From: sprang Date: Wed, 21 Oct 2015 13:46:33 -0700 Subject: [PATCH] Remove time constraint on first retransmit of a packet. We don't allow more than one retransmission within one RTT, but the RTT estimate might be off. Reasonably, the remote end will not send a NACK until the packet after has been received - so always resend on first request. Review URL: https://codereview.webrtc.org/1414563003 Cr-Commit-Position: refs/heads/master@{#10362} --- .../rtp_rtcp/source/rtp_packet_history.cc | 24 +++++---- .../rtp_rtcp/source/rtp_packet_history.h | 6 +-- .../source/rtp_packet_history_unittest.cc | 51 ++++++++++++++++--- 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc index e1d6d4852c..0b1de6d588 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -16,6 +16,7 @@ #include #include +#include "webrtc/base/checks.h" #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" #include "webrtc/system_wrappers/interface/logging.h" @@ -118,6 +119,7 @@ int32_t RTPPacketHistory::PutRTPPacket(const uint8_t* packet, (capture_time_ms > 0) ? capture_time_ms : clock_->TimeInMilliseconds(); stored_packets_[prev_index_].send_time = 0; // Packet not sent. stored_packets_[prev_index_].storage_type = type; + stored_packets_[prev_index_].has_been_retransmitted = false; ++prev_index_; if (prev_index_ >= stored_packets_.size()) { @@ -173,10 +175,9 @@ bool RTPPacketHistory::GetPacketAndSetSendTime(uint16_t sequence_number, size_t* packet_length, int64_t* stored_time_ms) { CriticalSectionScoped cs(critsect_.get()); - assert(*packet_length >= IP_PACKET_SIZE); - if (!store_) { + RTC_CHECK_GE(*packet_length, static_cast(IP_PACKET_SIZE)); + if (!store_) return false; - } int32_t index = 0; bool found = FindSeqNum(sequence_number, &index); @@ -193,17 +194,22 @@ bool RTPPacketHistory::GetPacketAndSetSendTime(uint16_t sequence_number, return false; } - // Verify elapsed time since last retrieve. + // Verify elapsed time since last retrieve, but only for retransmissions and + // always send packet upon first retransmission request. int64_t now = clock_->TimeInMilliseconds(); - if (min_elapsed_time_ms > 0 && + if (min_elapsed_time_ms > 0 && retransmit && + stored_packets_[index].has_been_retransmitted && ((now - stored_packets_[index].send_time) < min_elapsed_time_ms)) { return false; } - if (retransmit && stored_packets_[index].storage_type == kDontRetransmit) { - // No bytes copied since this packet shouldn't be retransmitted or is - // of zero size. - return false; + if (retransmit) { + if (stored_packets_[index].storage_type == kDontRetransmit) { + // No bytes copied since this packet shouldn't be retransmitted or is + // of zero size. + return false; + } + stored_packets_[index].has_been_retransmitted = true; } stored_packets_[index].send_time = clock_->TimeInMilliseconds(); GetPacket(index, packet, packet_length, stored_time_ms); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h index d128494383..e97d11eeaa 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h @@ -46,13 +46,12 @@ class RTPPacketHistory { // The packet is copied to the buffer pointed to by ptr_rtp_packet. // The rtp_packet_length should show the available buffer size. // Returns true if packet is found. - // rtp_packet_length: returns the copied packet length on success. + // packet_length: returns the copied packet length on success. // min_elapsed_time_ms: the minimum time that must have elapsed since the last // time the packet was resent (parameter is ignored if set to zero). - // If the packet is found but the minimum time has not elaped, no bytes are + // If the packet is found but the minimum time has not elapsed, no bytes are // copied. // stored_time_ms: returns the time when the packet was stored. - // type: returns the storage type set in PutRTPPacket. bool GetPacketAndSetSendTime(uint16_t sequence_number, int64_t min_elapsed_time_ms, bool retransmit, @@ -94,6 +93,7 @@ class RTPPacketHistory { int64_t time_ms = 0; int64_t send_time = 0; StorageType storage_type = kDontRetransmit; + bool has_been_retransmitted = false; uint8_t data[IP_PACKET_SIZE]; size_t length = 0; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index a30753adf9..63c95935ca 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -161,6 +161,8 @@ TEST_F(RtpPacketHistoryTest, DontRetransmit) { } TEST_F(RtpPacketHistoryTest, MinResendTime) { + static const int64_t kMinRetransmitIntervalMs = 100; + hist_->SetStorePacketsStatus(true, 10); size_t len = 0; int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); @@ -168,22 +170,57 @@ TEST_F(RtpPacketHistoryTest, MinResendTime) { EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms, kAllowRetransmission)); + // First transmission: TimeToSendPacket() call from pacer. int64_t time; len = kMaxPacketLength; - EXPECT_TRUE(hist_->GetPacketAndSetSendTime(kSeqNum, 100, false, packet_, &len, - &time)); - fake_clock_.AdvanceTimeMilliseconds(100); + EXPECT_TRUE( + hist_->GetPacketAndSetSendTime(kSeqNum, 0, false, packet_, &len, &time)); + + fake_clock_.AdvanceTimeMilliseconds(kMinRetransmitIntervalMs); // Time has elapsed. len = kMaxPacketLength; - EXPECT_TRUE(hist_->GetPacketAndSetSendTime(kSeqNum, 100, false, packet_, &len, - &time)); + EXPECT_TRUE(hist_->GetPacketAndSetSendTime(kSeqNum, kMinRetransmitIntervalMs, + true, packet_, &len, &time)); EXPECT_GT(len, 0u); EXPECT_EQ(capture_time_ms, time); + fake_clock_.AdvanceTimeMilliseconds(kMinRetransmitIntervalMs - 1); // Time has not elapsed. Packet should be found, but no bytes copied. len = kMaxPacketLength; - EXPECT_FALSE(hist_->GetPacketAndSetSendTime(kSeqNum, 101, false, packet_, - &len, &time)); + EXPECT_FALSE(hist_->GetPacketAndSetSendTime(kSeqNum, kMinRetransmitIntervalMs, + true, packet_, &len, &time)); +} + +TEST_F(RtpPacketHistoryTest, EarlyFirstResend) { + static const int64_t kMinRetransmitIntervalMs = 100; + + hist_->SetStorePacketsStatus(true, 10); + size_t len = 0; + int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); + CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len); + EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms, + kAllowRetransmission)); + + // First transmission: TimeToSendPacket() call from pacer. + int64_t time; + len = kMaxPacketLength; + EXPECT_TRUE( + hist_->GetPacketAndSetSendTime(kSeqNum, 0, false, packet_, &len, &time)); + + fake_clock_.AdvanceTimeMilliseconds(kMinRetransmitIntervalMs - 1); + // Time has not elapsed, but this is the first retransmission request so + // allow anyway. + len = kMaxPacketLength; + EXPECT_TRUE(hist_->GetPacketAndSetSendTime(kSeqNum, kMinRetransmitIntervalMs, + true, packet_, &len, &time)); + EXPECT_GT(len, 0u); + EXPECT_EQ(capture_time_ms, time); + + fake_clock_.AdvanceTimeMilliseconds(kMinRetransmitIntervalMs - 1); + // Time has not elapsed. Packet should be found, but no bytes copied. + len = kMaxPacketLength; + EXPECT_FALSE(hist_->GetPacketAndSetSendTime(kSeqNum, kMinRetransmitIntervalMs, + true, packet_, &len, &time)); } TEST_F(RtpPacketHistoryTest, DynamicExpansion) {