[core] Fixed getFlighSpan function and rexmit bug

srtcore/buffer.cpp

@@ -575,8 +575,6 @@ int CSndBuffer::readData(const int offset, CPacket& w_packet, steady_clock::time
     // the packet originally (the other overload of this function) must set these
     // flags.
     w_packet.m_iMsgNo = p->m_iMsgNoBitset;
-
-    // TODO: FR #930. Use source time if it is provided.
     w_srctime = getSourceTime(*p);
 
     // This function is called when packet retransmission is triggered.

srtcore/common.h

@@ -620,6 +620,7 @@ class CSeqNo
    {return (abs(seq1 - seq2) < m_iSeqNoTH) ? (seq1 - seq2) : (seq2 - seq1);}
 
    /// This function measures a length of the range from seq1 to seq2,
+   /// including endpoints (seqlen(a, a) = 1; seqlen(a, a + 1) = 2),
    /// WITH A PRECONDITION that certainly @a seq1 is earlier than @a seq2.
    /// This can also include an enormously large distance between them,
    /// that is, exceeding the m_iSeqNoTH value (can be also used to test

srtcore/core.cpp

@@ -6416,10 +6416,8 @@ void CUDT::checkNeedDrop(bool& w_bCongestion)
             m_stats.sndBytesDropTotal += dbytes;
             leaveCS(m_StatsLock);
 
-#if ENABLE_HEAVY_LOGGING
-            int32_t realack = m_iSndLastDataAck;
-#endif
-            int32_t fakeack = CSeqNo::incseq(m_iSndLastDataAck, dpkts);
+            IF_HEAVY_LOGGING(const int32_t realack = m_iSndLastDataAck);
+            const int32_t fakeack = CSeqNo::incseq(m_iSndLastDataAck, dpkts);
 
             m_iSndLastAck     = fakeack;
             m_iSndLastDataAck = fakeack;
@@ -7365,15 +7363,10 @@ void CUDT::bstats(CBytePerfMon *perf, bool clear, bool instantaneous)
         m_stats.rcvBytesDropTotal + (m_stats.rcvDropTotal * pktHdrSize) + m_stats.m_rcvBytesUndecryptTotal;
     perf->pktRcvUndecryptTotal  = m_stats.m_rcvUndecryptTotal;
     perf->byteRcvUndecryptTotal = m_stats.m_rcvBytesUndecryptTotal;
-    //<
 
     double interval = count_microseconds(currtime - m_stats.tsLastSampleTime);
-
-    //>mod
     perf->mbpsSendRate = double(perf->byteSent) * 8.0 / interval;
     perf->mbpsRecvRate = double(perf->byteRecv) * 8.0 / interval;
-    //<
-
     perf->usPktSndPeriod      = count_microseconds(m_tdSendInterval);
     perf->pktFlowWindow       = m_iFlowWindowSize;
     perf->pktCongestionWindow = (int)m_dCongestionWindow;
@@ -9002,10 +8995,10 @@ std::pair<int, steady_clock::time_point> CUDT::packData(CPacket& w_packet)
     {
         // If no loss, and no packetfilter control packet, pack a new packet.
 
-        // check congestion/flow window limit
+        // Check the congestion/flow window limit
         const int cwnd    = std::min(int(m_iFlowWindowSize), int(m_dCongestionWindow));
         const int flightspan = getFlightSpan();
-        if (cwnd >= flightspan)
+        if (cwnd > flightspan)
         {
             // XXX Here it's needed to set kflg to msgno_bitset in the block stored in the
             // send buffer. This should be somehow avoided, the crypto flags should be set
@@ -10815,30 +10808,18 @@ void CUDT::checkRexmitTimer(const steady_clock::time_point& currtime)
     const bool is_laterexmit = m_CongCtl->rexmitMethod() == SrtCongestion::SRM_LATEREXMIT;
     const bool is_fastrexmit = m_CongCtl->rexmitMethod() == SrtCongestion::SRM_FASTREXMIT;
 
-    // If the receiver will send periodic NAK reports, then FASTREXMIT is inactive.
-    // MIND that probably some method of "blind rexmit" MUST BE DONE, when TLPKTDROP is off.
+    // If the receiver will send periodic NAK reports, then FASTREXMIT (live) is inactive.
+    // TODO: Probably some method of "blind rexmit" MUST BE DONE, when TLPKTDROP is off.
     if (is_fastrexmit && m_bPeerNakReport)
         return;
 
-    // We need to retransmit only when the data in the sender's buffer was already sent.
-    // Otherwise it might still be sent regulary.
-    bool retransmit = false;
-    const int32_t unsent_seqno = CSeqNo::incseq(m_iSndCurrSeqNo);
-    // IF:
-    // - LATEREXMIT
-    // - flight window == 0
-    // - the sender loss list is empty (the receiver didn't send any LOSSREPORT, or LOSSREPORT was lost on track)
-    if ((is_laterexmit && unsent_seqno != m_iSndLastAck && m_pSndLossList->getLossLength() == 0)
-    // OR:
-            // - FASTREXMIT
-            // - flight window > 0
-         || (is_fastrexmit && getFlightSpan() != 0))
-    {
-        retransmit = true;
-    }
-
-
-    if (retransmit)
+    // Schedule for retransmission IF:
+    // - there are packets in flight (getFlightSpan() > 0);
+    // - in case of LATEREXMIT (File Mode): the sender loss list is empty
+    //   (the receiver didn't send any LOSSREPORT, or LOSSREPORT was lost on track).
+    // - in case of FASTREXMIT (Live Mode): there is the latency constraint, therefore
+    //   schedule unacknowledged packets for retransmission regardless of the loss list emptiness.
+    if (getFlightSpan() > 0 && (!is_laterexmit || m_pSndLossList->getLossLength() == 0))
     {
         // Sender: Insert all the packets sent after last received acknowledgement into the sender loss list.
         ScopedLock acklock(m_RecvAckLock); // Protect packet retransmission

srtcore/core.h

@@ -394,17 +394,40 @@ class CUDT
     duration minNAKInterval() const { return m_tdMinNakInterval; }
     sockaddr_any peerAddr() const { return m_PeerAddr; }
 
-    int32_t getFlightSpan() const
+    /// Returns the number of packets in flight (sent, but not yet acknowledged).
+    /// @param lastack is the sequence number of the first unacknowledged packet.
+    /// @param curseq is the sequence number of the latest original packet sent
+    ///
+    /// @note When there are no packets in flight, lastack = incseq(curseq).
+    ///
+    /// @returns The number of packets in flight belonging to the interval [0; ...)
+    static int32_t getFlightSpan(int32_t lastack, int32_t curseq)
     {
-        // This is a number of unacknowledged packets at this moment
-        // Note that normally m_iSndLastAck should be PAST m_iSndCurrSeqNo,
+        // Packets sent:
+        // | 1 | 2 | 3 | 4 | 5 |
+        //   ^               ^
+        //   |               |
+        // lastack           |
+        //                curseq
+        //
+        // In Flight: [lastack; curseq]
+        //
+        // Normally 'lastack' should be PAST the 'curseq',
         // however in a case when the sending stopped and all packets were
-        // ACKed, the m_iSndLastAck is one sequence ahead of m_iSndCurrSeqNo.
-        // Therefore we increase m_iSndCurrSeqNo by 1 forward and then
+        // ACKed, the 'lastack' is one sequence ahead of 'curseq'.
+        // Therefore we increase 'curseq' by 1 forward and then
         // get the distance towards the last ACK. This way this value may
-        // be only positive or 0.
+        // be only positive as seqlen() includes endpoints.
+        // Finally, we subtract 1 to exclude the increment added earlier.
 
-        return CSeqNo::seqlen(m_iSndLastAck, CSeqNo::incseq(m_iSndCurrSeqNo));
+        return CSeqNo::seqlen(lastack, CSeqNo::incseq(curseq)) - 1;
+    }
+
+    /// Returns the number of packets in flight (sent, but not yet acknowledged).
+    /// @returns The number of packets in flight belonging to the interval [0; ...)
+    int32_t getFlightSpan() const
+    {
+        return getFlightSpan(m_iSndLastAck, m_iSndCurrSeqNo);
     }
 
     int minSndSize(int len = 0) const

srtcore/list.cpp

@@ -111,6 +111,7 @@ void CSndLossList::traceState() const
 
 int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
 {
+    SRT_ASSERT(CSeqNo::seqlen(seqno1, seqno2) > 0);
     ScopedLock listguard(m_ListLock);
 
     if (m_iLength == 0)

test/test_seqno.cpp

@@ -1,5 +1,6 @@
 #include "gtest/gtest.h"
 #include "common.h"
+#include "core.h"
 
 
 const int32_t CSeqNo::m_iSeqNoTH;
@@ -41,6 +42,26 @@ TEST(CSeqNo, seqoff)
     EXPECT_EQ(CSeqNo::seqoff(0x7FFFFFFF, 1),    2);
 }
 
+TEST(CSeqNo, seqlen)
+{
+    EXPECT_EQ(CSeqNo::seqlen(125, 125), 1);
+    EXPECT_EQ(CSeqNo::seqlen(125, 126), 2);
+}
+
+TEST(CUDT, getFlightSpan)
+{
+    const int test_values[3][3] = {
+        // lastack  curseq  span
+        {      125,    124,   0 }, // all sent packets are acknowledged
+        {      125,    125,   1 },
+        {      125,    130,   6 }
+    };
+
+    for (auto val : test_values)
+    {
+        EXPECT_EQ(CUDT::getFlightSpan(val[0], val[1]), val[2]) << "Span(" << val[0] << ", " << val[1] << ")";
+    }
+}
 
 TEST(CSeqNo, incseq)
 {