Refax: removed m_iRcvLastSkipAck and its dependencies #2546

ethouris · 2022-11-22T08:54:49Z

Removed the m_iRcvLastSkipAck field. This field was different to m_iRcvLastAck in case when a drop was done and was used as a base sequence pinned in the old receiver buffer to the "end pointer" that was pointing to the end of the initial contiguous region. With the defined sequence number of the first cell in the new receiver buffer this field was only duplicating the information as well as the convention where the acknowledgement signs off packets for retrieval in live mode as well has been dismissed (this is still valid in file mode, but relies on only signaling the CV and setting read ready; both these things are being done by the TSBPD thread in live mode). The value of this field can be determined from m_iRcvLastAck and m_pRcvBuffer->getStartSeqNo(), whichever is later, however the way how this value was used in comparisons had to be changed as well, as the check if a sequence is within the buffer can be now done more straighforward way.

srtcore/core.cpp

…eUpTo convention for including given seqno. Removed unused updateForgotten.

gou4shi1 · 2022-11-22T17:19:11Z

Great refactor!
I just curious about will this PR be merged before #2535？Or this PR replaced #2535？
I noticed that the new packet insertion check is similar to #2535, so I'm OK to close #2535.

gou4shi1 · 2022-11-22T17:31:05Z

srtcore/buffer_rcv.cpp

@@ -1050,13 +1050,13 @@ void CRcvBuffer::updateTsbPdTimeBase(uint32_t usPktTimestamp)

 string CRcvBuffer::strFullnessState(bool enable_debug_log, int iFirstUnackSeqNo, const time_point& tsNow) const
 {
+    if (!enable_debug_log)


The reason of this enable_debug_log check: #2463 (comment)
If return empty string, why not just check enable_debug_log in the caller site? i.e. qrlog.Debug.CheckEnabled() ? strFullnessState() : ""

Because the idea is to avoid the lengthy evaluation when the resulting string will be ignored anyway. The instruction for logging will not result in a printed log if the log isn't enabled, but all the arguments will be evaluated anyway.

the lengthy evaluation

Does it mean qrlog.Debug.CheckEnabled() ? strFullnessState() : "" ?

Probably would work as well. @maxsharabayko what do you think?

Does it mean qrlog.Debug.CheckEnabled() ? strFullnessState() : "" ?

Good suggestion. And get rid of the enable_debug_log argument.

Maybe even this way? At least this improves reliability over the explicit version:

<< LOGEVAL_IF(qrlog.Warn, m_pRcvBuffer->strFullnessState(m_iRcvLastAck, steady_clock::now()))

gou4shi1

Some minor comments about logs.

gou4shi1 · 2022-11-23T03:45:17Z

srtcore/buffer_rcv.cpp

-           << " 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. ";


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

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.

gou4shi1 · 2022-11-23T03:53:13Z

srtcore/core.cpp

-        // Remove [from,to-inclusive]
-        dropFromLossLists(m_iRcvLastSkipAck, CSeqNo::decseq(seqno));
-        m_iRcvLastSkipAck = seqno;
+        LOGC(tslog.Error, log << CONID() << "IPE: drop request up to %" << seqno << " while last received is %"


It's not an IPE in group mode, it will happen frequently if it's called from dropToGroupRecvBase() in broadcast mode, IIRC.

Right. Forgot that this function is also being used from the group synchronization code, and in this case this sequence may exceed the last received sequence. Still, AFAIR this last received sequence has to be synchronized as well. Fortunately this will remain only temporarily, until the common receiver buffer is ready.

this last received sequence has to be synchronized as well

I tried, this may break senders' check:

if (CSeqNo::seqcmp(ackdata_seqno, CSeqNo::incseq(m_iSndCurrSeqNo)) > 0) { leaveCS(m_RecvAckLock); // this should not happen: attack or bug

That's why I'd simply leave it as is for now. The common receiver code will approach this part differently.

srtcore/core.cpp

@@ -9749,14 +9763,12 @@ int srt::CUDT::handleSocketPacketReception(const vector<CUnit*>& incoming, bool&
            m_stats.rcvr.recvdBelated.count(rpkt.getLength());
            leaveCS(m_StatsLock);
            HLOGC(qrlog.Debug,
-                    log << CONID() << "RECEIVED: seq=" << rpkt.m_iSeqNo << " offset=" << offset << " (BELATED/"
+                    log << CONID() << "RECEIVED: %" << rpkt.m_iSeqNo << " bufidx=" << bufidx << " (BELATED/"


gou4shi1 · 2022-11-23T05:47:51Z

srtcore/core.cpp

-                        << "+" << CSeqNo::incseq(m_iRcvLastSkipAck, int(m_pRcvBuffer->capacity()) - 1)
-                        << "), " << (offset-avail_bufsize+1)
+                        " %" << rpkt.m_iSeqNo
+                        << " buffer=(%" << bufseq


<< "buffer=(%" << bufseq << ":%" << m_iRcvCurrSeqNo << "+%" << CSeqNo::incseq(bufseq, int(m_pRcvBuffer->capacity()) - 1) << ")"

This code block was repeated many times, maybe add a dedicated function?

Might be, but I'd postpone it until the common receiver as things may change there a lot. In case of a common receiver there will be a different buffer where data come from.

srtcore/core.cpp

maxsharabayko · 2022-11-23T11:57:55Z

srtcore/core.cpp

+// [[using locked(m_RcvBufferLock)]]
+bool srt::CUDT::getFirstNoncontSequence(int32_t& w_seq, string& w_log_reason)


Given the function return value is always true and thus can be ignored, might make sense to return the w_seq instead of passing it as a reference.

This is consistent with the future implementation of it where it returns false in case when it neither has a receiver buffer nor is a group member. This situation should never happen and therefore for this it's a sanity check. I can change it, but this will cause merge problems in the future.

srtcore/core.cpp

maxsharabayko · 2022-11-23T14:50:18Z

srtcore/core.cpp

-                << ":" << m_iRcvCurrSeqNo                   // -1 = size to last index
-                << "+" << CSeqNo::incseq(m_iRcvLastSkipAck, m_pRcvBuffer->capacity()-1)
+            // XXX Fix this when the end of contiguous region detection is added.
+            int ackidx = CSeqNo::seqoff(m_pRcvBuffer->getStartSeqNo(), m_iRcvLastAck);


Suggested change

int ackidx = CSeqNo::seqoff(m_pRcvBuffer->getStartSeqNo(), m_iRcvLastAck);

const int ackidx = CSeqNo::seqoff(m_pRcvBuffer->getStartSeqNo(), m_iRcvLastAck);

The instruction two lines below this will not compile.

Can be converted into a single line using the std::max(0, CSeqNo...).

srtcore/core.cpp

Mikołaj Małecki and others added 4 commits November 17, 2022 16:16

Removed LastSkipAck field. Updates rely on receiver buffer

d49f52c

Merge branch 'master' into dev-remove-lastskipack

3972832

Fixed checking against offset towards buffer start

7251986

Fixed: no lock inside handle*. Use past-the-end for removeUpTo.

95e2b23

ethouris added Priority: High Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Nov 22, 2022

ethouris requested a review from maxsharabayko November 22, 2022 08:54

ethouris changed the title ~~Refax: removed m_RcvLastSkipAck and its dependencies~~ Refax: removed m_iRcvLastSkipAck and its dependencies Nov 22, 2022

maxsharabayko requested changes Nov 22, 2022

View reviewed changes

srtcore/core.cpp Outdated Show resolved Hide resolved

Mikołaj Małecki and others added 4 commits November 22, 2022 14:26

Merge branch 'master' into dev-remove-lastskipack

de6df41

Restored the old dropping proc for the sake of DROPREQ. Changed remov…

0b65158

…eUpTo convention for including given seqno. Removed unused updateForgotten.

Blocked unnecessary debug logs

97bc8aa

Cosmetix and removal of wrong and unnecessary changes

24f4d45

This comment was marked as off-topic.

Sign in to view

gou4shi1 reviewed Nov 22, 2022

View reviewed changes

gou4shi1 reviewed Nov 23, 2022

View reviewed changes

Minor post-review fixes

2995f89

maxsharabayko reviewed Nov 23, 2022

View reviewed changes

srtcore/core.cpp Outdated Show resolved Hide resolved

Mikołaj Małecki added 3 commits November 24, 2022 11:50

Further post-review fixes

6c3d7cb

Removed extra condition from strFullnessState

d0a0c30

Minor fix: use max to allow keeping variable constant

ee49a81

maxsharabayko approved these changes Nov 28, 2022

View reviewed changes

gou4shi1 reviewed Nov 29, 2022

View reviewed changes

srtcore/core.cpp Outdated Show resolved Hide resolved

Removed wrong comment

d978930

gou4shi1 approved these changes Nov 29, 2022

View reviewed changes

maxsharabayko approved these changes Nov 29, 2022

View reviewed changes

maxsharabayko mentioned this pull request Nov 29, 2022

[BUG] Possible crash while sending crypto keys. #2553

Closed

maxsharabayko merged commit 637d439 into Haivision:master Nov 29, 2022

gou4shi1 mentioned this pull request Nov 29, 2022

[core] Utilize all insert results of the new recv buffer. #2535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

ethouris commented Nov 22, 2022

This comment was marked as off-topic.

gou4shi1 commented Nov 22, 2022 •

edited

Loading

gou4shi1 Nov 22, 2022 •

edited

Loading

ethouris Nov 23, 2022

gou4shi1 Nov 23, 2022

ethouris Nov 23, 2022

maxsharabayko Nov 23, 2022

ethouris Nov 24, 2022 •

edited

Loading

gou4shi1 left a comment

gou4shi1 Nov 23, 2022

ethouris Nov 23, 2022

gou4shi1 Nov 23, 2022 •

edited

Loading

ethouris Nov 23, 2022

gou4shi1 Nov 23, 2022 •

edited

Loading

ethouris Nov 23, 2022

This comment was marked as resolved.

gou4shi1 Nov 23, 2022

ethouris Nov 23, 2022

maxsharabayko Nov 23, 2022

ethouris Nov 23, 2022

maxsharabayko Nov 23, 2022

ethouris Nov 23, 2022

maxsharabayko Nov 28, 2022

		// [[using locked(m_RcvBufferLock)]]
		bool srt::CUDT::getFirstNoncontSequence(int32_t& w_seq, string& w_log_reason)

	int ackidx = CSeqNo::seqoff(m_pRcvBuffer->getStartSeqNo(), m_iRcvLastAck);
	const int ackidx = CSeqNo::seqoff(m_pRcvBuffer->getStartSeqNo(), m_iRcvLastAck);

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

Conversation

ethouris commented Nov 22, 2022

This comment was marked as off-topic.

gou4shi1 commented Nov 22, 2022 • edited Loading

gou4shi1 Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethouris Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

gou4shi1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gou4shi1 Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gou4shi1 Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gou4shi1 commented Nov 22, 2022 •

edited

Loading

gou4shi1 Nov 22, 2022 •

edited

Loading

ethouris Nov 24, 2022 •

edited

Loading

gou4shi1 Nov 23, 2022 •

edited

Loading

gou4shi1 Nov 23, 2022 •

edited

Loading