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
Merged

Refax: removed m_iRcvLastSkipAck and its dependencies #2546

Show file tree
Hide file tree
Changes from 4 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
10 changes: 5 additions & 5 deletions srtcore/buffer_rcv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@gou4shi1 gou4shi1 Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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() : ""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

the lengthy evaluation

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Good suggestion. And get rid of the enable_debug_log argument.

Copy link
Collaborator Author

@ethouris ethouris Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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()))

return string();

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
Loading