[core] Refax: moved removal of one seq from fresh loss list to a sepa…

…rate function (#2521).

srtcore/core.cpp

@@ -10434,53 +10434,8 @@ void srt::CUDT::unlose(const CPacket &packet)
     if (m_bPeerRexmitFlag == 0 || m_iReorderTolerance == 0)
         return;
 
-    size_t i       = 0;
-    int    had_ttl = 0;
-    for (i = 0; i < m_FreshLoss.size(); ++i)
-    {
-        had_ttl = m_FreshLoss[i].ttl;
-        switch (m_FreshLoss[i].revoke(sequence))
-        {
-        case CRcvFreshLoss::NONE:
-            continue; // Not found. Search again.
-
-        case CRcvFreshLoss::STRIPPED:
-            goto breakbreak; // Found and the modification is applied. We're done here.
-
-        case CRcvFreshLoss::DELETE:
-            // No more elements. Kill it.
-            m_FreshLoss.erase(m_FreshLoss.begin() + i);
-            // Every loss is unique. We're done here.
-            goto breakbreak;
-
-        case CRcvFreshLoss::SPLIT:
-            // Oh, this will be more complicated. This means that it was in between.
-            {
-                // So create a new element that will hold the upper part of the range,
-                // and this one modify to be the lower part of the range.
-
-                // Keep the current end-of-sequence value for the second element
-                int32_t next_end = m_FreshLoss[i].seq[1];
-
-                // seq-1 set to the end of this element
-                m_FreshLoss[i].seq[1] = CSeqNo::decseq(sequence);
-                // seq+1 set to the begin of the next element
-                int32_t next_begin = CSeqNo::incseq(sequence);
-
-                // Use position of the NEXT element because insertion happens BEFORE pointed element.
-                // Use the same TTL (will stay the same in the other one).
-                m_FreshLoss.insert(m_FreshLoss.begin() + i + 1,
-                                   CRcvFreshLoss(next_begin, next_end, m_FreshLoss[i].ttl));
-            }
-            goto breakbreak;
-        }
-    }
-
-    // Could have made the "return" instruction instead of goto, but maybe there will be something
-    // to add in future, so keeping that.
-breakbreak:;
-
-    if (i != m_FreshLoss.size())
+    int had_ttl = 0;
+    if (CRcvFreshLoss::removeOne((m_FreshLoss), sequence, (&had_ttl)))
     {
         HLOGC(qrlog.Debug, log << "sequence " << sequence << " removed from belated lossreport record");
     }

srtcore/list.cpp

@@ -897,3 +897,53 @@ srt::CRcvFreshLoss::Emod srt::CRcvFreshLoss::revoke(int32_t lo, int32_t hi)
 
     return DELETE;
 }
+
+bool srt::CRcvFreshLoss::removeOne(std::deque<CRcvFreshLoss>& w_container, int32_t sequence, int* pw_had_ttl)
+{
+    for (size_t i = 0; i < w_container.size(); ++i)
+    {
+        const int had_ttl = w_container[i].ttl;
+        Emod wh = w_container[i].revoke(sequence);
+
+        if (wh == NONE)
+            continue;  // Not found. Search again.
+
+        if (wh == DELETE)   //  ... oo ... x ... o ... => ... oo ... o ...
+        {
+            // Removed the only element in the record - remove the record.
+            w_container.erase(w_container.begin() + i);
+        }
+        else if (wh == SPLIT) // ... ooxooo ... => ... oo ... ooo ...
+        {
+            // Create a new element that will hold the upper part of the range,
+            // and the found one modify to be the lower part of the range.
+
+            // Keep the current end-of-sequence value for the second element
+            int32_t next_end = w_container[i].seq[1];
+
+            // seq-1 set to the end of this element
+            w_container[i].seq[1] = CSeqNo::decseq(sequence);
+            // seq+1 set to the begin of the next element
+            int32_t next_begin = CSeqNo::incseq(sequence);
+
+            // Use position of the NEXT element because insertion happens BEFORE pointed element.
+            // Use the same TTL (will stay the same in the other one).
+            w_container.insert(w_container.begin() + i + 1,
+                    CRcvFreshLoss(next_begin, next_end, w_container[i].ttl));
+        }
+        // For STRIPPED:  ... xooo ... => ... ooo ...
+        // i.e. there's nothing to do.
+
+        // Every loss is unique. We're done here.
+        if (pw_had_ttl)
+            *pw_had_ttl = had_ttl;
+
+        return true;
+    }
+
+    if (pw_had_ttl)
+        *pw_had_ttl = 0;
+    return false;
+
+}
+

srtcore/list.h

@@ -53,6 +53,8 @@ modified by
 #ifndef INC_SRT_LIST_H
 #define INC_SRT_LIST_H
 
+#include <deque>
+
 #include "udt.h"
 #include "common.h"
 
@@ -84,6 +86,12 @@ class CSndLossList
 
     void traceState() const;
 
+    // Debug/unittest support.
+
+    int head() const { return m_iHead; }
+    int next(int loc) const { return m_caSeq[loc].inext; }
+    int last() const { return m_iLastInsertPos; }
+
 private:
     struct Seq
     {
@@ -118,6 +126,8 @@ class CSndLossList
     /// @param seqno2  last sequence number in range (SRT_SEQNO_NONE if no range)
     bool updateElement(int pos, int32_t seqno1, int32_t seqno2);
 
+    static const int LOC_NONE = -1;
+
 private:
     CSndLossList(const CSndLossList&);
     CSndLossList& operator=(const CSndLossList&);
@@ -270,6 +280,8 @@ struct CRcvFreshLoss
 
     Emod revoke(int32_t sequence);
     Emod revoke(int32_t lo, int32_t hi);
+
+    static bool removeOne(std::deque<CRcvFreshLoss>& w_container, int32_t sequence, int* had_ttl = NULL);
 };
 
 } // namespace srt

test/test_losslist_rcv.cpp

@@ -69,3 +69,69 @@ TEST_F(CRcvLossListTest, InsertTwoElemsEdge)
     EXPECT_TRUE(m_lossList->remove(CSeqNo::m_iMaxSeqNo, 1));
     CheckEmptyArray();
 }
+
+TEST(CRcvFreshLossListTest, CheckFreshLossList)
+{
+    std::deque<CRcvFreshLoss> floss {
+        CRcvFreshLoss (10, 15, 5),
+        CRcvFreshLoss (25, 29, 10),
+        CRcvFreshLoss (30, 30, 3),
+        CRcvFreshLoss (45, 80, 100)
+    };
+
+    EXPECT_EQ(floss.size(), 4);
+
+    // Ok, now let's do element removal
+
+    int had_ttl = 0;
+    bool rm = CRcvFreshLoss::removeOne((floss), 26, &had_ttl);
+
+    EXPECT_EQ(rm, true);
+    EXPECT_EQ(had_ttl, 10);
+    EXPECT_EQ(floss.size(), 5);
+
+    // Now we expect to have [10-15] [25-25] [27-35]...
+    // After revoking 25 it should have removed it.
+
+    // SPLIT
+    rm = CRcvFreshLoss::removeOne((floss), 27, &had_ttl);
+    EXPECT_EQ(rm, true);
+    EXPECT_EQ(had_ttl, 10);
+    EXPECT_EQ(floss.size(), 5);
+
+    // STRIP
+    rm = CRcvFreshLoss::removeOne((floss), 28, &had_ttl);
+    EXPECT_EQ(rm, true);
+    EXPECT_EQ(had_ttl, 10);
+    EXPECT_EQ(floss.size(), 5);
+
+    // DELETE
+    rm = CRcvFreshLoss::removeOne((floss), 25, &had_ttl);
+    EXPECT_EQ(rm, true);
+    EXPECT_EQ(had_ttl, 10);
+    EXPECT_EQ(floss.size(), 4);
+
+    // SPLIT
+    rm = CRcvFreshLoss::removeOne((floss), 50, &had_ttl);
+    EXPECT_EQ(rm, true);
+    EXPECT_EQ(had_ttl, 100);
+    EXPECT_EQ(floss.size(), 5);
+
+    // DELETE
+    rm = CRcvFreshLoss::removeOne((floss), 30, &had_ttl);
+    EXPECT_EQ(rm, true);
+    EXPECT_EQ(had_ttl, 3);
+    EXPECT_EQ(floss.size(), 4);
+
+    // Remove nonexistent sequence, but existing before.
+    rm = CRcvFreshLoss::removeOne((floss), 25, NULL);
+    EXPECT_EQ(rm, false);
+    EXPECT_EQ(floss.size(), 4);
+
+    // Remove nonexistent sequence that didn't exist before.
+    rm = CRcvFreshLoss::removeOne((floss), 31, &had_ttl);
+    EXPECT_EQ(rm, false);
+    EXPECT_EQ(had_ttl, 0);
+    EXPECT_EQ(floss.size(), 4);
+
+}