[core] Fix CSndLossList::insert with negative offset (#1359)

srtcore/common.h

@@ -615,6 +615,7 @@ class CSeqNo
    /// distance between two sequence numbers.
    ///
    /// Example: to check if (seq1 %> seq2): seqcmp(seq1, seq2) > 0.
+   /// Note: %> stands for "later than".
    inline static int seqcmp(int32_t seq1, int32_t seq2)
    {return (abs(seq1 - seq2) < m_iSeqNoTH) ? (seq1 - seq2) : (seq2 - seq1);}
 

srtcore/list.cpp

@@ -122,6 +122,25 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
     const int offset  = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);
     int       loc     = (m_iHead + offset + m_iSize) % m_iSize;
 
+    if (loc < 0)
+    {
+        const int offset_seqno2 = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno2);
+        const int loc_seqno2    = (m_iHead + offset_seqno2 + m_iSize) % m_iSize;
+
+        if (loc_seqno2 < 0)
+        {
+            // The size of the CSndLossList should be at least the size of the flow window.
+            // It means that all the packets sender has sent should fit within m_iSize.
+            // If the new loss does not fit, there is some error.
+            LOGC(mglog.Error, log << "IPE: New loss record is too old. Ignoring. "
+                << "First loss seqno " << m_caSeq[m_iHead].seqstart
+                << ", insert seqno " << seqno1 << ":" << seqno2);
+            return 0;
+        }
+
+        loc = loc_seqno2;
+    }
+
     if (offset < 0)
     {
         insertHead(loc, seqno1, seqno2);
@@ -153,6 +172,7 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
             if (CSeqNo::seqcmp(seqend, seqno1) < 0 && CSeqNo::incseq(seqend) != seqno1)
             {
                 // No overlap
+                // TODO: Here we should actually insert right after i, not at loc.
                 insertAfter(loc, i, seqno1, seqno2);
             }
             else
@@ -347,6 +367,7 @@ int32_t CSndLossList::popLostSeq()
 
 void CSndLossList::insertHead(int pos, int32_t seqno1, int32_t seqno2)
 {
+    SRT_ASSERT(pos >= 0);
     m_caSeq[pos].seqstart = seqno1;
     SRT_ASSERT(m_caSeq[pos].seqend == SRT_SEQNO_NONE);
     if (seqno2 != seqno1)

srtcore/list.h

@@ -66,22 +66,18 @@ class CSndLossList
     /// @param [in] seqno1 sequence number starts.
     /// @param [in] seqno2 sequence number ends.
     /// @return number of packets that are not in the list previously.
-
     int insert(int32_t seqno1, int32_t seqno2);
 
     /// Remove the given sequence number and all numbers that precede it.
     /// @param [in] seqno sequence number.
-
     void removeUpTo(int32_t seqno);
 
     /// Read the loss length.∏
     /// @return The length of the list.
-
     int getLossLength() const;
 
     /// Read the first (smallest) loss seq. no. in the list and remove it.
     /// @return The seq. no. or -1 if the list is empty.
-
     int32_t popLostSeq();
 
     void traceState() const;
@@ -90,7 +86,7 @@ class CSndLossList
     struct Seq
     {
         int32_t seqstart; // sequence number starts
-        int32_t seqend;   // seqnence number ends
+        int32_t seqend;   // sequence number ends
         int     inext;    // index of the next node in the list
     } * m_caSeq;
 
@@ -116,7 +112,7 @@ class CSndLossList
 
     /// Update existing element with the new range (increase only)
     /// @param pos     position of the element being updated
-    /// @param seqno1  first seqnuence number in range
+    /// @param seqno1  first sequence number in range
     /// @param seqno2  last sequence number in range (SRT_SEQNO_NONE if no range)
     bool updateElement(int pos, int32_t seqno1, int32_t seqno2);
 

test/test_list.cpp

@@ -449,7 +449,7 @@ TEST_F(CSndLossListTest, InsertHeadOverlap02)
     CheckEmptyArray();
 }
 
-TEST_F(CSndLossListTest, DISABLED_InsertHeadNegativeOffset01)
+TEST_F(CSndLossListTest, InsertHeadNegativeOffset01)
 {
     EXPECT_EQ(m_lossList->insert(10000000, 10000000), 1);
     EXPECT_EQ(m_lossList->insert(10000001, 10000001), 1);
@@ -468,19 +468,84 @@ TEST_F(CSndLossListTest, DISABLED_InsertHeadNegativeOffset01)
     CheckEmptyArray();
 }
 
+// Check the part of the loss report the can fit into the list
+// goes into the list.
+TEST_F(CSndLossListTest, InsertHeadNegativeOffset02)
+{
+    const int32_t head_seqno = 10000000;
+    EXPECT_EQ(m_lossList->insert(head_seqno,     head_seqno), 1);
+    EXPECT_EQ(m_lossList->insert(head_seqno + 1, head_seqno + 1), 1);
+    EXPECT_EQ(m_lossList->getLossLength(), 2);
+
+    // The offset of the sequence number being added does not fit
+    // into the size of the loss list, it must be ignored.
+    // Normally this situation should not happen.
+
+    const int32_t outofbound_seqno = head_seqno - CSndLossListTest::SIZE;
+    EXPECT_EQ(m_lossList->insert(outofbound_seqno - 1, outofbound_seqno + 1), 3);
+    EXPECT_EQ(m_lossList->getLossLength(), 5);
+    EXPECT_EQ(m_lossList->popLostSeq(), outofbound_seqno - 1);
+    EXPECT_EQ(m_lossList->getLossLength(), 4);
+    EXPECT_EQ(m_lossList->popLostSeq(), outofbound_seqno);
+    EXPECT_EQ(m_lossList->getLossLength(), 3);
+    EXPECT_EQ(m_lossList->popLostSeq(), outofbound_seqno + 1);
+    EXPECT_EQ(m_lossList->getLossLength(), 2);
+    EXPECT_EQ(m_lossList->popLostSeq(), 10000000);
+    EXPECT_EQ(m_lossList->getLossLength(), 1);
+    EXPECT_EQ(m_lossList->popLostSeq(), 10000001);
+
+    CheckEmptyArray();
+}
+
 /////////////////////////////////////////////////////////////////////////////////////////
 /////////////////////////////////////////////////////////////////////////////////////////
-TEST_F(CSndLossListTest, DISABLED_InsertFullList)
+TEST_F(CSndLossListTest, InsertFullListCoalesce)
 {
     for (int i = 1; i <= CSndLossListTest::SIZE; i++)
-        m_lossList->insert(i, i);
-    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE);
-    m_lossList->insert(CSndLossListTest::SIZE + 1, CSndLossListTest::SIZE + 1);
+        EXPECT_EQ(m_lossList->insert(i, i), 1);
     EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE);
-    for (int i = 1; i <= CSndLossListTest::SIZE; i++)
+    // Inserting additional element: 1 item more than list size.
+    // But given all elements coalesce into one entry, list size should still increase.
+    EXPECT_EQ(m_lossList->insert(CSndLossListTest::SIZE + 1, CSndLossListTest::SIZE + 1), 1);
+    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1);
+    for (int i = 1; i <= CSndLossListTest::SIZE + 1; i++)
     {
         EXPECT_EQ(m_lossList->popLostSeq(), i);
-        EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE - i);
+        EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1 - i);
+    }
+    EXPECT_EQ(m_lossList->popLostSeq(), -1);
+    EXPECT_EQ(m_lossList->getLossLength(), 0);
+
+    CheckEmptyArray();
+}
+
+TEST_F(CSndLossListTest, DISABLED_InsertFullListNoCoalesce)
+{
+    // We will insert each element with a gap of one elements.
+    // This should lead to having space for only [i; SIZE] sequence numbers.
+    for (int i = 1; i <= CSndLossListTest::SIZE / 2; i++)
+        EXPECT_EQ(m_lossList->insert(2 * i, 2 * i), 1);
+
+    // At this point the list has every second element empty
+    // [0]:taken, [1]: empty, [2]: taken, [3]: empty, ...
+    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE / 2);
+
+    // Inserting additional element: 1 item more than list size.
+    // There should be one free place for it at list[SIZE-1]
+    // right after previously inserted element.
+    const int seqno1 = CSndLossListTest::SIZE + 2;
+    EXPECT_EQ(m_lossList->insert(seqno1, seqno1), 1);
+
+    // Inserting one more element into a full list.
+    // There should be no place for it.
+    const int seqno2 = CSndLossListTest::SIZE + 4;
+    EXPECT_EQ(m_lossList->insert(seqno2, seqno2), 0);
+
+    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1);
+    for (int i = 1; i <= CSndLossListTest::SIZE + 1; i++)
+    {
+        EXPECT_EQ(m_lossList->popLostSeq(), 2 * i);
+        EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE  - i);
     }
     EXPECT_EQ(m_lossList->popLostSeq(), -1);
     EXPECT_EQ(m_lossList->getLossLength(), 0);