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.

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

[core] Made offset consistent with avail_bufsize. #2465

Merged
merged 2 commits into from
Oct 12, 2022

Conversation

gou4shi1
Copy link
Contributor

@gou4shi1 gou4shi1 commented Sep 27, 2022

@maxsharabayko maxsharabayko added the [core] Area: Changes in SRT library core label Sep 27, 2022
@maxsharabayko maxsharabayko added this to the Next release milestone Sep 27, 2022
@maxsharabayko maxsharabayko added the Type: Bug Indicates an unexpected problem or unintended behavior label Sep 27, 2022
@maxsharabayko
Copy link
Collaborator

One of the changes has been introduced in #2463.
I believe the remaining one is not correct. The available RCV buffer size has to be counted from the acknowledgment position.
The value is sent in the ACK packet back to the sender (required protocol behavior), as well as in SRT stats (not much sense to change the behavior).

@maxsharabayko
Copy link
Collaborator

In fact, I think using m_iRcvLastSkipAck for the strFullnessState(..) is also incorrect, because ACK position is needed instead of the drop position. I missed that one reviewing #2463. So I guess that very change has to be reverted.

@gou4shi1
Copy link
Contributor Author

One of the changes has been introduced in #2463.

Oops, my mistake, lol.

@gou4shi1
Copy link
Contributor Author

gou4shi1 commented Sep 27, 2022

Then the m_iRcvLastSkipAck in

LOGC(qrlog.Error, log << CONID() <<
        "SEQUENCE DISCREPANCY. BREAKING CONNECTION."
        " seq=" << rpkt.m_iSeqNo
        << " buffer=(" << m_iRcvLastSkipAck
        << ":" << m_iRcvCurrSeqNo                   // -1 = size to last index
        << "+" << CSeqNo::incseq(m_iRcvLastSkipAck, int(m_pRcvBuffer->capacity()) - 1)
        << "), " << (offset-avail_bufsize+1)
        << " past max. Reception no longer possible. REQUESTING TO CLOSE.");

should also be changed?

I created this PR because I found the buffer number in the SEQUENCE DISCREPANCY log is not consistent with the offset number.
I'm OK for both solutions, as long as logs are consistent.

@gou4shi1 gou4shi1 closed this Sep 27, 2022
@gou4shi1 gou4shi1 deleted the fix-getAvailRcvBufferSize branch September 27, 2022 14:51
@maxsharabayko
Copy link
Collaborator

Then the m_iRcvLastSkipAck in

LOGC(qrlog.Error, log << CONID() << "SEQUENCE DISCREPANCY. BREAKING CONNECTION." " seq=" << rpkt.m_iSeqNo << " buffer=(" << m_iRcvLastSkipAck << ":" << m_iRcvCurrSeqNo // -1 = size to last index << "+" << CSeqNo::incseq(m_iRcvLastSkipAck, int(m_pRcvBuffer->capacity()) - 1)

should also be changed?

Good catch!
Yes, here m_iRcvLastAck should be used instead of m_iRcvLastSkipAck. To have the same base with getAvailRcvBufferSizeNoLock() called above.

@gou4shi1 gou4shi1 restored the fix-getAvailRcvBufferSize branch September 27, 2022 16:55
@gou4shi1 gou4shi1 reopened this Sep 28, 2022
@gou4shi1 gou4shi1 changed the title [core] getAvailRcvBufferSize() use m_iRcvLastSkipAck instead of m_iRcvLastAck [core] make avail_bufsize consistent with offset Sep 28, 2022
@codecov-commenter

This comment was marked as off-topic.

@gou4shi1 gou4shi1 force-pushed the fix-getAvailRcvBufferSize branch 2 times, most recently from 68bcda4 to 0a9b802 Compare September 30, 2022 15:00
@gou4shi1 gou4shi1 changed the title [core] make avail_bufsize consistent with offset [core] Made offset consistent with avail_bufsize. Sep 30, 2022
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

All correct. Please consider one correction of the comment.

srtcore/core.cpp Outdated Show resolved Hide resolved
Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
@gou4shi1
Copy link
Contributor Author

ping

@maxsharabayko
Copy link
Collaborator

Oops, sorry, forgot about this PR 😃

@maxsharabayko maxsharabayko merged commit 04369b8 into Haivision:master Oct 12, 2022
@gou4shi1 gou4shi1 deleted the fix-getAvailRcvBufferSize branch October 13, 2022 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants