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.

Sign up for GitHub

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

Small improvements of CSndLossList::insert #1148

Merged
merged 3 commits into from
Feb 26, 2020
Merged

Small improvements of CSndLossList::insert #1148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cdbe0e
[tests] Added some CSndLossList tests
maxsharabayko Feb 23, 2020
d2c14ab
[core] Minor refactoring of CSndLossList::insert
maxsharabayko Feb 23, 2020
f62fdb3
[core] Improve insert serial element
maxsharabayko Feb 23, 2020
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
85 changes: 55 additions & 30 deletions srtcore/list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,20 @@ CSndLossList::~CSndLossList()
releaseMutex(m_ListLock);
}

void CSndLossList::traceState() const
{
int pos = m_iHead;
while (pos != -1)
{
::cout << pos << ":[" << m_caSeq[pos].seqstart;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use LOG* facility to display it. Even if this is only left for future debugging.

Of course, you'd need std::ostringstream for intermediate collection, as a single log instruction cannot print a fragment of a line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can LOGF not end the line with \n like Verb() allows to do?

Copy link
Collaborator

@ethouris ethouris Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. One log instruction always ends with an end of line. Partial-line logging would have to be added support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then would be simpler to leave this type of output for the sake of code simplicity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verb() has a special VerbNoEOL tag to leave without EOL. But Verb() doesn't have a log header, so there's a little problem with this. For LOG* we'd need something like a temporary log cumulation variable, which will be then reused when doing a final log with EOL. But I don't think we could this way do any better than by using std::ostringstream.

if (m_caSeq[pos].seqend != -1)
::cout << ", " << m_caSeq[pos].seqend;
::cout << "], ";
pos = m_caSeq[pos].inext;
}
::cout << "\n";
}

int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
{
CGuard listguard(m_ListLock);
Expand All @@ -103,10 +117,10 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
return m_iLength;
}

// otherwise find the position where the data can be inserted
int origlen = m_iLength;
int offset = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);
int loc = (m_iHead + offset + m_iSize) % m_iSize;
// Find the insert position in the non-empty list
const int origlen = m_iLength;
const int offset = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);
int loc = (m_iHead + offset + m_iSize) % m_iSize;

if (offset < 0)
{
Expand All @@ -122,44 +136,40 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
}
else
{
// searching the prior node
int i;
if ((-1 != m_iLastInsertPos) && (CSeqNo::seqcmp(m_caSeq[m_iLastInsertPos].seqstart, seqno1) < 0))
// Find the prior node.
// It should be the highest sequence number less than seqno1.
// 1. Start the search either from m_iHead, or from m_iLastInsertPos
int i = m_iHead;
if ((m_iLastInsertPos != -1) && (CSeqNo::seqcmp(m_caSeq[m_iLastInsertPos].seqstart, seqno1) < 0))
i = m_iLastInsertPos;
else
i = m_iHead;

while ((-1 != m_caSeq[i].inext) && (CSeqNo::seqcmp(m_caSeq[m_caSeq[i].inext].seqstart, seqno1) < 0))
// 2. Find the highest sequence number less than seqno1.
while (m_caSeq[i].inext != -1 && CSeqNo::seqcmp(m_caSeq[m_caSeq[i].inext].seqstart, seqno1) < 0)
i = m_caSeq[i].inext;

if ((-1 == m_caSeq[i].seqend) || (CSeqNo::seqcmp(m_caSeq[i].seqend, seqno1) < 0))
{
m_iLastInsertPos = loc;

// no overlap, create new node
m_caSeq[loc].seqstart = seqno1;
if (seqno2 != seqno1)
m_caSeq[loc].seqend = seqno2;

m_caSeq[loc].inext = m_caSeq[i].inext;
m_caSeq[i].inext = loc;
// 3. Check if seqno1 overlaps with (seqbegin, seqend)
const int seqend = m_caSeq[i].seqend == -1
? m_caSeq[i].seqstart
: m_caSeq[i].seqend;

m_iLength += CSeqNo::seqlen(seqno1, seqno2);
if (CSeqNo::seqcmp(seqend, seqno1) < 0 && CSeqNo::incseq(seqend) != seqno1)
{
// No overlap
insertAfter(loc, i, seqno1, seqno2);
}
else
{
// TODO: Replace with updateElement(i, seqno1, seqno2).
// Some changes to updateElement(..) are required.
m_iLastInsertPos = i;
if (CSeqNo::seqcmp(seqend, seqno2) >= 0)
return 0;

// overlap, coalesce with prior node, insert(3, 7) to [2, 5], ... becomes [2, 7]
if (CSeqNo::seqcmp(m_caSeq[i].seqend, seqno2) < 0)
{
m_iLength += CSeqNo::seqlen(m_caSeq[i].seqend, seqno2) - 1;
m_caSeq[i].seqend = seqno2;
m_iLength += CSeqNo::seqlen(seqend, seqno2) - 1;
m_caSeq[i].seqend = seqno2;

loc = i;
}
else
return 0;
loc = i;
}
}
}
Expand Down Expand Up @@ -340,6 +350,7 @@ int32_t CSndLossList::popLostSeq()
void CSndLossList::insertHead(int pos, int32_t seqno1, int32_t seqno2)
{
m_caSeq[pos].seqstart = seqno1;
SRT_ASSERT(m_caSeq[pos].seqend == -1);
if (seqno2 != seqno1)
m_caSeq[pos].seqend = seqno2;

Expand All @@ -351,6 +362,20 @@ void CSndLossList::insertHead(int pos, int32_t seqno1, int32_t seqno2)
m_iLength += CSeqNo::seqlen(seqno1, seqno2);
}

void CSndLossList::insertAfter(int pos, int pos_after, int32_t seqno1, int32_t seqno2)
{
m_caSeq[pos].seqstart = seqno1;
SRT_ASSERT(m_caSeq[pos].seqend == -1);
if (seqno2 != seqno1)
m_caSeq[pos].seqend = seqno2;

m_caSeq[pos].inext = m_caSeq[pos_after].inext;
m_caSeq[pos_after].inext = pos;
m_iLastInsertPos = pos;

m_iLength += CSeqNo::seqlen(seqno1, seqno2);
}

void CSndLossList::coalesce(int loc)
{
// coalesce with next node. E.g., [3, 7], ..., [6, 9] becomes [3, 9]
Expand Down
6 changes: 6 additions & 0 deletions srtcore/list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ class CSndLossList

int32_t popLostSeq();

void traceState() const;

private:
struct Seq
{
Expand All @@ -106,6 +108,10 @@ class CSndLossList
/// No lock.
void insertHead(int pos, int32_t seqno1, int32_t seqno2);

/// Inserts an element after previous element.
/// No lock.
void insertAfter(int pos, int pos_after, int32_t seqno1, int32_t seqno2);

/// Check if it is possible to coalesce element at loc with further elements.
/// @param loc - last changed location
void coalesce(int loc);
Expand Down
31 changes: 31 additions & 0 deletions test/test_list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,19 @@ TEST_F(CSndLossListTest, InsertPopTwoElems)
CheckEmptyArray();
}

/// Insert 1 and 2 and pop() one by one
TEST_F(CSndLossListTest, InsertPopTwoSerialElems)
{
EXPECT_EQ(m_lossList->insert(1, 1), 1);
EXPECT_EQ(m_lossList->insert(2, 2), 1);

EXPECT_EQ(m_lossList->getLossLength(), 2);
EXPECT_EQ(m_lossList->popLostSeq(), 1);
EXPECT_EQ(m_lossList->getLossLength(), 1);
EXPECT_EQ(m_lossList->popLostSeq(), 2);
CheckEmptyArray();
}

/// Insert (1,2) and 4, then pop one by one
TEST_F(CSndLossListTest, InsertPopRangeAndSingle)
{
Expand Down Expand Up @@ -118,6 +131,24 @@ TEST_F(CSndLossListTest, InsertPopFourElems)
CheckEmptyArray();
}

/// Insert (1,2) and 4, then pop one by one
TEST_F(CSndLossListTest, InsertCoalesce)
{
EXPECT_EQ(m_lossList->insert(1, 2), 2);
EXPECT_EQ(m_lossList->insert(4, 4), 1);
EXPECT_EQ(m_lossList->insert(3, 3), 1);

EXPECT_EQ(m_lossList->getLossLength(), 4);
EXPECT_EQ(m_lossList->popLostSeq(), 1);
EXPECT_EQ(m_lossList->getLossLength(), 3);
EXPECT_EQ(m_lossList->popLostSeq(), 2);
EXPECT_EQ(m_lossList->getLossLength(), 2);
EXPECT_EQ(m_lossList->popLostSeq(), 3);
EXPECT_EQ(m_lossList->getLossLength(), 1);
EXPECT_EQ(m_lossList->popLostSeq(), 4);
CheckEmptyArray();
}

///////////////////////////////////////////////////////////////////////////////
///
/// The group of tests checks remove() from different positions in the list,
Expand Down