[core] Fixed packet retransmission.

Do not break sending if a packet in the loss list was ackowledged.
Haivision · May 16, 2019 · 25d1dce · 25d1dce
1 parent 3d159f0
commit 25d1dce
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 56 deletions.
diff --git a/srtcore/core.cpp b/srtcore/core.cpp
@@ -7325,6 +7325,80 @@ void CUDT::updateAfterSrtHandshake(int srt_cmd, int hsv)
     }
 }
 
+
+int CUDT::packLostData(CPacket& packet, uint64_t& origintime)
+{
+    // protect m_iSndLastDataAck from updating by ACK processing
+    CGuard ackguard(m_AckLock);
+
+    while ((packet.m_iSeqNo = m_pSndLossList->getLostSeq()) >= 0)
+    {
+        const int offset = CSeqNo::seqoff(m_iSndLastDataAck, packet.m_iSeqNo);
+        if (offset < 0)
+        {
+            LOGC(dlog.Error, log << "IPE: packLostData: LOST packet negative offset: seqoff(m_iSeqNo "
+                << packet.m_iSeqNo << ", m_iSndLastDataAck " << m_iSndLastDataAck
+                << ")=" << offset << ". Continue");
+            continue;
+        }
+
+        int msglen;
+
+        const int payload = m_pSndBuffer->readData(&(packet.m_pcData), offset, packet.m_iMsgNo, origintime, msglen);
+        SRT_ASSERT(payload != 0);
+        if (payload == -1)
+        {
+            int32_t seqpair[2];
+            seqpair[0] = packet.m_iSeqNo;
+            seqpair[1] = CSeqNo::incseq(seqpair[0], msglen);
+            sendCtrl(UMSG_DROPREQ, &packet.m_iMsgNo, seqpair, 8);
+
+            // only one msg drop request is necessary
+            m_pSndLossList->remove(seqpair[1]);
+
+            // skip all dropped packets
+            if (CSeqNo::seqcmp(m_iSndCurrSeqNo, CSeqNo::incseq(seqpair[1])) < 0)
+                m_iSndCurrSeqNo = CSeqNo::incseq(seqpair[1]);
+
+            continue;
+        }
+        // NOTE: This is just a sanity check. Returning 0 is impossible to happen
+        // in case of retransmission. If the offset was a positive value, then the
+        // block must exist in the old blocks because it wasn't yet cut off by ACK
+        // and has been already recorded as sent (otherwise the peer wouldn't send
+        // back the loss report). May something happen here in case when the send
+        // loss record has been updated by the FASTREXMIT.
+        else if (payload == 0)
+            continue;
+
+        // At this point we no longer need the ACK lock,
+        // because we are going to return from the function.
+        // Therefore unlocking in order not to block other threads.
+        ackguard.forceUnlock();
+
+        CGuard::enterCS(m_StatsLock);
+        ++m_stats.traceRetrans;
+        ++m_stats.retransTotal;
+        m_stats.traceBytesRetrans += payload;
+        m_stats.bytesRetransTotal += payload;
+        CGuard::leaveCS(m_StatsLock);
+
+        // Despite the contextual interpretation of packet.m_iMsgNo around
+        // CSndBuffer::readData version 2 (version 1 doesn't return -1), in this particular
+        // case we can be sure that this is exactly the value of PH_MSGNO as a bitset.
+        // So, set here the rexmit flag if the peer understands it.
+        if (m_bPeerRexmitFlag)
+        {
+            packet.m_iMsgNo |= PACKET_SND_REXMIT;
+        }
+
+        return payload;
+    }
+
+    return 0;
+}
+
+
 int CUDT::packData(CPacket& packet, uint64_t& ts_tk)
 {
    int payload = 0;
@@ -7365,62 +7439,10 @@ int CUDT::packData(CPacket& packet, uint64_t& ts_tk)
 
    string reason;
 
-   // Loss retransmission always has higher priority.
-   packet.m_iSeqNo = m_pSndLossList->getLostSeq();
-   if (packet.m_iSeqNo >= 0)
+   payload = packLostData(packet, origintime);
+   if (payload > 0)
    {
-      // protect m_iSndLastDataAck from updating by ACK processing
-      CGuard ackguard(m_AckLock);
-
-      int offset = CSeqNo::seqoff(m_iSndLastDataAck, packet.m_iSeqNo);
-      if (offset < 0)
-         return 0;
-
-      int msglen;
-
-      payload = m_pSndBuffer->readData(&(packet.m_pcData), offset, packet.m_iMsgNo, origintime, msglen);
-
-      if (-1 == payload)
-      {
-         int32_t seqpair[2];
-         seqpair[0] = packet.m_iSeqNo;
-         seqpair[1] = CSeqNo::incseq(seqpair[0], msglen);
-         sendCtrl(UMSG_DROPREQ, &packet.m_iMsgNo, seqpair, 8);
-
-         // only one msg drop request is necessary
-         m_pSndLossList->remove(seqpair[1]);
-
-         // skip all dropped packets
-         if (CSeqNo::seqcmp(m_iSndCurrSeqNo, CSeqNo::incseq(seqpair[1])) < 0)
-             m_iSndCurrSeqNo = CSeqNo::incseq(seqpair[1]);
-
-         return 0;
-      }
-      // NOTE: This is just a sanity check. Returning 0 is impossible to happen
-      // in case of retransmission. If the offset was a positive value, then the
-      // block must exist in the old blocks because it wasn't yet cut off by ACK
-      // and has been already recorded as sent (otherwise the peer wouldn't send
-      // back the loss report). May something happen here in case when the send
-      // loss record has been updated by the FASTREXMIT.
-      else if (payload == 0)
-         return 0;
-
-      CGuard::enterCS(m_StatsLock);
-      ++ m_stats.traceRetrans;
-      ++ m_stats.retransTotal;
-      m_stats.traceBytesRetrans += payload;
-      m_stats.bytesRetransTotal += payload;
-      CGuard::leaveCS(m_StatsLock);
-
-      // Kinda confusion here. Despite the contextual interpretation of packet.m_iMsgNo around
-      // CSndBuffer::readData version 2 (version 1 doesn't return -1), in this particular
-      // case we can be sure that this is exactly the value of PH_MSGNO as a bitset.
-      // So, set here the rexmit flag if the peer understands it.
-      if ( m_bPeerRexmitFlag )
-      {
-          packet.m_iMsgNo |= PACKET_SND_REXMIT;
-      }
-      reason = "reXmit";
+       reason = "reXmit";
    }
    else
    {

diff --git a/srtcore/core.h b/srtcore/core.h
@@ -454,7 +454,7 @@ class CUDT
     /// @param perf [in, out] pointer to a CPerfMon structure to record the performance data.
     /// @param clear [in] flag to decide if the local performance trace should be cleared. 
     /// @param instantaneous [in] flag to request instantaneous data 
-    /// instead of moving averages. 
+    /// instead of moving averages.
     void bstats(CBytePerfMon* perf, bool clear = true, bool instantaneous = false);
 
     /// Mark sequence contained in the given packet as not lost. This
@@ -702,6 +702,15 @@ class CUDT
 private: // Generation and processing of packets
     void sendCtrl(UDTMessageType pkttype, void* lparam = NULL, void* rparam = NULL, int size = 0);
     void processCtrl(CPacket& ctrlpkt);
+
+    /// Pack a packet from a list of lost packets.
+    ///
+    /// @param packet [in, out] a packet structure to fill
+    /// @param origintime [in, out] origin timestamp of the packet
+    ///
+    /// @return payload size on success, <=0 on failure
+    int packLostData(CPacket& packet, uint64_t& origintime);
+
     int packData(CPacket& packet, uint64_t& ts);
     int processData(CUnit* unit);
     void processClose();