diff --git a/srtcore/common.h b/srtcore/common.h index 7ab2c13b26..043718575b 100644 --- a/srtcore/common.h +++ b/srtcore/common.h @@ -614,7 +614,7 @@ class CSeqNo /// VALUE for any other purpose. It is not meant to be the /// distance between two sequence numbers. /// - /// Example: to check if (seq1 %> seq2): seqcmp(seq1, seq2) > 0. + /// Example: to check if (seq1 > seq2): seqcmp(seq1, seq2) > 0. inline static int seqcmp(int32_t seq1, int32_t seq2) {return (abs(seq1 - seq2) < m_iSeqNoTH) ? (seq1 - seq2) : (seq2 - seq1);} diff --git a/srtcore/list.cpp b/srtcore/list.cpp index cec001bfc9..0009cfb906 100644 --- a/srtcore/list.cpp +++ b/srtcore/list.cpp @@ -122,6 +122,17 @@ 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) + { + // 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; + } + if (offset < 0) { insertHead(loc, seqno1, seqno2); @@ -153,6 +164,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 +359,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) diff --git a/srtcore/list.h b/srtcore/list.h index d6fa36d151..19b300f301 100644 --- a/srtcore/list.h +++ b/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); diff --git a/test/test_list.cpp b/test/test_list.cpp index 0aab096d14..e843955bdd 100644 --- a/test/test_list.cpp +++ b/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); @@ -470,17 +470,53 @@ TEST_F(CSndLossListTest, DISABLED_InsertHeadNegativeOffset01) ///////////////////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////////////////// -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);