[core] Fix : Sequence number in the ACK packet isn’t correct (#2873)

[kura979]

Reset the m_iLargestSeq back to SRT_SEQNO_NONE once the loss list is empty. This way if the next packet loss seqno has a distance more than m_iSeqNoTH from the m_iLargestSeq, it is not ignored and added to the loss list.

Fixes #2873.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8b73dbc) 66.99% compared to head (f916578) 66.59%.

❗ Current head f916578 differs from pull request most recent head 62f9e7e. Consider uploading reports for the commit 62f9e7e to get more accurate results

Files	Patch %	Lines
srtcore/list.cpp	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2877      +/-   ##
==========================================
- Coverage   66.99%   66.59%   -0.40%     
==========================================
  Files         103      103              
  Lines       20460    20464       +4     
==========================================
- Hits        13707    13628      -79     
- Misses       6753     6836      +83     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[maxsharabayko]

The Yoda notation is not used in the project. To be consistent, the typical order should be used.

[gou4shi1]

I think this PR will re-introduce this bug: #2192 (comment)

[gou4shi1]

m_iLargestSeq should record history seq, and should not be reset after empty, to fix concurrent issues.

We should find another way to fix your issue "the next packet loss seqno has a distance more than m_iSeqNoTH"

[ethouris]

In #2527 I have provided various improvements for the receiver buffer. In this solution there's also changed the way of taking the sequence number for ACK, which is no longer taken from the loss list, and it's taken exclusively from the receiver buffer (the ACK actually acknowledges the initial contiguous region in the receiver buffer, which is defined directly in this version). Would you be able to take a look at that?

[maxsharabayko]

m_iLargestSeq should record history seq, and should not be reset after empty, to fix concurrent issues.

We should find another way to fix your issue "the next packet loss seqno has a distance more than m_iSeqNoTH"

Just thinking out loud, what if the m_iLargestSeq would be ignored upon inserting to the RCV loss list if the loss list is empty?
As a workaround to resolve both issues until #2527 is merged.

Besides, PR #2527 does not fix the RCV loss list, that does not add the loss seqno in itself.

[ethouris]

Yes, #2527 doesn't fix the loss list, it simply takes out one of its roles in deciding about ACK. Loss list in here is exclusively used for recording the loss for the need of NAKREPORT and in case of the file CC, also checking the loss level for speed control.

[maxsharabayko]

@gou4shi1 The bug was that a data packet 899264404 with a gap is inserted into the receiver buffer (899264403 is missing). But before the loss is recorded in the RCV loss list, the TSBPD thread drops the missing packet seqno 899264403.
I am afraid I forgot how does the m_iLargestSeq` fixes the concurrency issue.

The original issue description (#2192 (comment)):

When processData() received 899264404, it was added into the recv buffer, after recvbuf_acklock was unlocked, the tsbpd thread played 899264404 and dropped 899264403, then processData() immediately insert 899264403 back into loss list, after this, getFirstLostSeq() keep return 899264403

[maxsharabayko]

Please continue the discussion in #2881.