[core] Refactored ACK window management to avoid false error reports#1888
Open
ethouris wants to merge 12 commits into
Open
[core] Refactored ACK window management to avoid false error reports#1888ethouris wants to merge 12 commits into
ethouris wants to merge 12 commits into
Conversation
ethouris
force-pushed
the
dev-refax-ack-window
branch
from
May 31, 2021 14:44
f3d43c3 to
a5de970
Compare
ethouris
marked this pull request as ready for review
May 31, 2021 15:19
added 3 commits
June 3, 2021 09:29
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+204
to
+265
| /* Updated old version remains for historical reasons | ||
| Status old_acknowledge(AckNode* r_aSeq, const size_t size, int& r_iHead, int& r_iTail, int32_t jrn, int32_t& w_ack, const steady_clock::time_point& currtime, int32_t& w_timediff) | ||
| { | ||
| // Head has not exceeded the physical boundary of the window | ||
| if (r_iHead >= r_iTail) | ||
| { | ||
| for (int i = r_iTail, n = r_iHead; i < n; ++ i) | ||
| { | ||
| // Looking for an identical ACK Seq. No. | ||
| if (seq == r_aSeq[i].iACKSeqNo) | ||
| // looking for an identical ACK AckNode. No. | ||
| if (jrn == r_aSeq[i].iJournal) | ||
| { | ||
| // Return the Data ACK it carried | ||
| r_ack = r_aSeq[i].iACK; | ||
| w_ack = r_aSeq[i].iAckSeq; | ||
|
|
||
| // Calculate RTT estimate | ||
| const int rtt = (int)count_microseconds(currtime - r_aSeq[i].tsTimeStamp); | ||
| w_timediff = (int32_t)count_microseconds(currtime - r_aSeq[i].tsTimeStamp); | ||
|
|
||
| if (i + 1 == r_iHead) | ||
| { | ||
| r_iTail = r_iHead = 0; | ||
| r_aSeq[0].iACKSeqNo = SRT_SEQNO_NONE; | ||
| r_aSeq[0].iJournal = SRT_SEQNO_NONE; | ||
| } | ||
| else | ||
| r_iTail = (i + 1) % size; | ||
|
|
||
| return rtt; | ||
| return OK; | ||
| } | ||
| } | ||
|
|
||
| // The record about ACK is not found in the buffer, RTT can not be calculated | ||
| return -1; | ||
| return ROGUE; | ||
| } | ||
|
|
||
| // Head has exceeded the physical window boundary, so it is behind tail | ||
| for (int j = r_iTail, n = r_iHead + (int)size; j < n; ++ j) | ||
| { | ||
| // Looking for an identical ACK Seq. No. | ||
| if (seq == r_aSeq[j % size].iACKSeqNo) | ||
| if (jrn == r_aSeq[j % size].iJournal) | ||
| { | ||
| // Return the Data ACK it carried | ||
| j %= size; | ||
| r_ack = r_aSeq[j].iACK; | ||
| w_ack = r_aSeq[j].iAckSeq; | ||
|
|
||
| // Calculate RTT estimate | ||
| const int rtt = (int)count_microseconds(currtime - r_aSeq[j].tsTimeStamp); | ||
| w_timediff = (int32_t)count_microseconds(currtime - r_aSeq[j].tsTimeStamp); | ||
|
|
||
| if (j == r_iHead) | ||
| { | ||
| r_iTail = r_iHead = 0; | ||
| r_aSeq[0].iACKSeqNo = -1; | ||
| r_aSeq[0].iJournal = -1; | ||
| } | ||
| else | ||
| r_iTail = (j + 1) % size; | ||
|
|
||
| return rtt; | ||
| return OK; | ||
| } | ||
| } | ||
|
|
||
| // The record about ACK is not found in the buffer, RTT can not be calculated | ||
| return -1; | ||
| return ROGUE; | ||
| } | ||
|
|
||
| } // namespace AckTools | ||
| */ |
Comment on lines
+125
to
+131
| /* | ||
| ACKWindow::Status old_acknowledge(int32_t jrn, int32_t& w_ackseq, const srt::sync::steady_clock::time_point& currtime, int32_t& w_timediff) | ||
| { | ||
| return ACKWindow::old_acknowledge(m_aSeq, SIZE, m_iHead, m_iTail, jrn, (w_ackseq), currtime, (w_timediff)); | ||
| } | ||
| unblock for testing | ||
| */ |
ethouris
added
Type: Maintenance
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #253
This refactors the
acknowledgefunction from Window tools, which not only returns what it was supposed to return so far as results, but also returns a status that allows to distinguish the reason as to why the ACK node could not be used this time to calculate RTT:Effectively, only if OK is returned should the RTT calculation be taken. If OLD, simply ignore the case (no log required), otherwise issue an IPE log message, as these situations shall never happen in a healthy code.
The body of
acknowledgehas been also completely rewritten for clarity and better performance. Also test cases have been provided.