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

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

Merged
merged 13 commits into from
Nov 29, 2022
+218 −131
Merged

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d49f52c
Removed LastSkipAck field. Updates rely on receiver buffer
Nov 17, 2022
3972832
Merge branch 'master' into dev-remove-lastskipack
Nov 18, 2022
7251986
Fixed checking against offset towards buffer start
Nov 18, 2022
95e2b23
Fixed: no lock inside handle*. Use past-the-end for removeUpTo.
ethouris Nov 21, 2022
de6df41
Merge branch 'master' into dev-remove-lastskipack
Nov 22, 2022
0b65158
Restored the old dropping proc for the sake of DROPREQ. Changed remov…
Nov 22, 2022
97bc8aa
Blocked unnecessary debug logs
ethouris Nov 22, 2022
24f4d45
Cosmetix and removal of wrong and unnecessary changes
ethouris Nov 22, 2022
2995f89
Minor post-review fixes
Nov 23, 2022
6c3d7cb
Further post-review fixes
Nov 24, 2022
d0a0c30
Removed extra condition from strFullnessState
Nov 25, 2022
ee49a81
Minor fix: use max to allow keeping variable constant
Nov 28, 2022
d978930
Removed wrong comment
Nov 29, 2022
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
9 changes: 3 additions & 6 deletions srtcore/buffer_rcv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,15 +1048,12 @@ void CRcvBuffer::updateTsbPdTimeBase(uint32_t usPktTimestamp)
m_tsbpd.updateTsbPdTimeBase(usPktTimestamp);
}

string CRcvBuffer::strFullnessState(bool enable_debug_log, int iFirstUnackSeqNo, const time_point& tsNow) const
string CRcvBuffer::strFullnessState(int iFirstUnackSeqNo, const time_point& tsNow) const
{
stringstream ss;

if (enable_debug_log)
{
ss << "iFirstUnackSeqNo=" << iFirstUnackSeqNo << " m_iStartSeqNo=" << m_iStartSeqNo
<< " m_iStartPos=" << m_iStartPos << " m_iMaxPosInc=" << m_iMaxPosInc << ". ";
}
ss << "iFirstUnackSeqNo=" << iFirstUnackSeqNo << " m_iStartSeqNo=" << m_iStartSeqNo
<< " m_iStartPos=" << m_iStartPos << " m_iMaxPosInc=" << m_iMaxPosInc << ". ";

ss << "Space avail " << getAvailSize(iFirstUnackSeqNo) << "/" << m_szSize << " pkts. ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the no room check has nothing to do with getAvailSize(ack), this "Space avail..." log should also be changed or removed, e.g. https://github.com/Haivision/srt/pull/2535/files#diff-45454334aff81b7e3327c5453befe97eb41e7ec0c3bdfc695e25949ba9f463adL1061

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually don't understand what is happening here. I raised some concerns already about the way how the "avail space" is calculated. I didn't trace that before, but that all depends on what you're going to use this value for. The sequence of acknowledgement is only a marker of the part of the buffer not yet retrieved by the application. But rest of the buffer contains two ranges: working range (packets that are there, maybe with losses, but haven't been yet acknowledged) and available range (no packet with such a sequence has come even for the first time). A sender that is about to decide as to whether it can send the packet and count on that the receiver has a place to store it, should be given only this last "available" range. But also the sender should regard this free space value only when going to send the new packets, not when retransmitting. Retransmission should use the "working range" which should always be available. But now I'm not exactly sure how to best display it.


Expand Down
2 changes: 1 addition & 1 deletion srtcore/buffer_rcv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ class CRcvBuffer

/// Form a string of the current buffer fullness state.
/// number of packets acknowledged, TSBPD readiness, etc.
std::string strFullnessState(bool enable_debug_log, int iFirstUnackSeqNo, const time_point& tsNow) const;
std::string strFullnessState(int iFirstUnackSeqNo, const time_point& tsNow) const;

private:
CTsbpdTime m_tsbpd;
Expand Down
284 changes: 173 additions & 111 deletions srtcore/core.cpp
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 1 addition & 11 deletions srtcore/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,7 @@ class CUDT
/// the receiver fresh loss list.
void unlose(const CPacket& oldpacket);
void dropFromLossLists(int32_t from, int32_t to);
bool getFirstNoncontSequence(int32_t& w_seq, std::string& w_log_reason);

void checkSndTimers();

Expand Down Expand Up @@ -722,8 +723,6 @@ class CUDT
/// @return The number of packets dropped.
int rcvDropTooLateUpTo(int seqno);

void updateForgotten(int seqlen, int32_t lastack, int32_t skiptoseqno);

static loss_seqs_t defaultPacketArrival(void* vself, CPacket& pkt);
static loss_seqs_t groupPacketArrival(void* vself, CPacket& pkt);

Expand Down Expand Up @@ -915,7 +914,6 @@ class CUDT
#ifdef ENABLE_LOGGING
int32_t m_iDebugPrevLastAck;
#endif
int32_t m_iRcvLastSkipAck; // Last dropped sequence ACK
int32_t m_iRcvLastAckAck; // (RCV) Latest packet seqno in a sent ACK acknowledged by ACKACK. RcvQTh (sendCtrlAck {r}, processCtrlAckAck {r}, processCtrlAck {r}, connection {w}).
int32_t m_iAckSeqNo; // Last ACK sequence number
sync::atomic<int32_t> m_iRcvCurrSeqNo; // (RCV) Largest received sequence number. RcvQTh, TSBPDTh.
Expand Down Expand Up @@ -1095,11 +1093,6 @@ class CUDT
static void addLossRecord(std::vector<int32_t>& lossrecord, int32_t lo, int32_t hi);
int32_t bake(const sockaddr_any& addr, int32_t previous_cookie = 0, int correction = 0);

/// @brief Acknowledge reading position up to the @p seq.
/// Updates m_iRcvLastAck and m_iRcvLastSkipAck to @p seq.
/// @param seq first unacknowledged packet sequence number.
void ackDataUpTo(int32_t seq);

#if ENABLE_BONDING
/// @brief Drop packets in the recv buffer behind group_recv_base.
/// Updates m_iRcvLastSkipAck if it's behind group_recv_base.
Expand All @@ -1108,9 +1101,6 @@ class CUDT

void processKeepalive(const CPacket& ctrlpkt, const time_point& tsArrival);

/// Locks m_RcvBufferLock and retrieves the available size of the receiver buffer.
SRT_ATTR_EXCLUDES(m_RcvBufferLock)
size_t getAvailRcvBufferSizeLock() const;

/// Retrieves the available size of the receiver buffer.
/// Expects that m_RcvBufferLock is locked.
Expand Down
2 changes: 1 addition & 1 deletion srtcore/group.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4000,7 +4000,7 @@ void CUDTGroup::updateLatestRcv(CUDTSocket* s)

HLOGC(grlog.Debug,
log << "updateLatestRcv: BACKUP group, updating from active link @" << s->m_SocketID << " with %"
<< s->core().m_iRcvLastSkipAck);
<< s->core().m_iRcvLastAck);

CUDT* source = &s->core();
vector<CUDT*> targets;
Expand Down
34 changes: 33 additions & 1 deletion srtcore/list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@ namespace srt_logging
{
extern Logger qrlog;
extern Logger qslog;
extern Logger tslog;
}

using srt_logging::qrlog;
using srt_logging::qslog;
using srt_logging::tslog;

using namespace srt::sync;

Expand Down Expand Up @@ -728,6 +730,34 @@ bool srt::CRcvLossList::remove(int32_t seqno1, int32_t seqno2)
return true;
}

int32_t srt::CRcvLossList::removeUpTo(int32_t seqno_last)
{
int32_t first = getFirstLostSeq();
if (first == SRT_SEQNO_NONE)
{
//HLOGC(tslog.Debug, log << "rcv-loss: DROP to %" << seqno_last << " - empty list");
return first; // empty, so nothing to remove
}

if (CSeqNo::seqcmp(seqno_last, first) < 0)
{
//HLOGC(tslog.Debug, log << "rcv-loss: DROP to %" << seqno_last << " - first %" << first << " is newer, exitting");
return first; // seqno_last older than first - nothing to remove
}

HLOGC(tslog.Debug, log << "rcv-loss: DROP to %" << seqno_last << " ...");

// NOTE: seqno_last is past-the-end here. Removed are only seqs
// that are earlier than this.
for (int32_t i = first; CSeqNo::seqcmp(i, seqno_last) <= 0; i = CSeqNo::incseq(i))
{
//HLOGC(tslog.Debug, log << "... removing %" << i);
remove(i);
}

return first;
}

bool srt::CRcvLossList::find(int32_t seqno1, int32_t seqno2) const
{
if (0 == m_iLength)
Expand Down Expand Up @@ -836,8 +866,10 @@ srt::CRcvFreshLoss::Emod srt::CRcvFreshLoss::revoke(int32_t lo, int32_t hi)
// ITEM: <lo, hi> <--- delete
// If the sequence range is older than the range to be revoked,
// delete it anyway.
if (CSeqNo::seqcmp(lo, seq[1]) > 0)
if (lo != SRT_SEQNO_NONE && CSeqNo::seqcmp(lo, seq[1]) > 0)
return DELETE;
// IF <lo> is NONE, then rely simply on that item.hi <% arg.hi,
// which is a condition at the end.

// LOHI: <lo, hi>
// ITEM: <lo, hi> <-- NOTFOUND
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 @@ -150,6 +150,12 @@ class CRcvLossList

bool remove(int32_t seqno1, int32_t seqno2);


/// Remove all numbers that precede the given sequence number.
/// @param [in] seqno sequence number.
/// @return the first removed sequence number
int32_t removeUpTo(int32_t seqno);

/// Find if there is any lost packets whose sequence number falling seqno1 and seqno2.
/// @param [in] seqno1 start sequence number.
/// @param [in] seqno2 end sequence number.
Expand Down