Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions srtcore/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);}

Expand Down
21 changes: 21 additions & 0 deletions srtcore/list.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 2 additions & 6 deletions srtcore/list.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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);

Expand Down
79 changes: 72 additions & 7 deletions test/test_list.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down