From 34a7134e87184d5887c05bdceb17ac4c4eefc9e6 Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Fri, 19 Jun 2020 11:25:01 +0200 Subject: [PATCH 1/2] [core] Fix CSndLossList::insert with negative offset --- srtcore/common.h | 1 + srtcore/list.cpp | 13 ++++++++++++ srtcore/list.h | 8 ++------ test/test_list.cpp | 50 +++++++++++++++++++++++++++++++++++++++------- 4 files changed, 59 insertions(+), 13 deletions(-) diff --git a/srtcore/common.h b/srtcore/common.h index 7ab2c13b2..db0e26229 100644 --- a/srtcore/common.h +++ b/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);} diff --git a/srtcore/list.cpp b/srtcore/list.cpp index dc884f6e0..6e94709f7 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 d6fa36d15..19b300f30 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 0aab096d1..e843955bd 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); From 1e44b9d5b1fe25368a0dbf6d71e47529f2286205 Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Wed, 12 Aug 2020 11:10:33 +0200 Subject: [PATCH 2/2] Added check for upper sequence --- srtcore/list.cpp | 22 +++++++++++++++------- test/test_list.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/srtcore/list.cpp b/srtcore/list.cpp index 6e94709f7..d229fb1a5 100644 --- a/srtcore/list.cpp +++ b/srtcore/list.cpp @@ -124,13 +124,21 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2) 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; + 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) diff --git a/test/test_list.cpp b/test/test_list.cpp index e843955bd..72164a5e5 100644 --- a/test/test_list.cpp +++ b/test/test_list.cpp @@ -468,6 +468,35 @@ TEST_F(CSndLossListTest, 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, InsertFullListCoalesce)