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

fix bandwidth limitation problem #1996

Closed
wants to merge 9 commits into from

Conversation

cg82616424
Copy link
Contributor

if set a very low bandwidth(on some IOT devices) limitation, the real bandwidth is not very accuracy with the number set by SRTO_MAXBW; main reason is that checkrexmit timer will call update function and let sender worker sendout a message。so if we need very low bandwidth limitation, this problem must be fixed;

@cg82616424 cg82616424 closed this May 12, 2021
@cg82616424 cg82616424 reopened this May 12, 2021
@maxsharabayko maxsharabayko added the [core] Area: Changes in SRT library core label May 12, 2021
@maxsharabayko maxsharabayko added this to the v1.4.4 milestone May 12, 2021
@cg82616424
Copy link
Contributor Author

hi @maxsharabayko ,will you accept this pull request?

@cg82616424
Copy link
Contributor Author

or you have some more good ideas to fix this problem?

@maxsharabayko
Copy link
Collaborator

maxsharabayko commented May 14, 2021

Hi @cg82616424
Bandwidth limitation is applied in a bit different place: in the CSndQueue::worker thread.
See the usage of CUDT::m_tsNextSendTime in CUDT::packData(..) and CSndUList logic.

Delaying a decision to mark a packet for retransmission might have a negative impact on loss recovery.

The use case you have described is related to #638, #713, and #1738. The logic of bandwidth limitation needs to be improved with all those cases in mind. I am afraid I can't accept this PR until this improvement work is started.

BTW consider using the SRTO_RETRANSMITALGO=1 socket option to reduce retransmission overhead (if you don't already use it).

@cg82616424
Copy link
Contributor Author

cg82616424 commented May 14, 2021

Hi @maxsharabayko : thanks for your reply:
about your question,

Bandwidth limitation is applied in a bit different place: in the CSndQueue::worker thread.
See the usage of CUDT::m_tsNextSendTime in CUDT::packData(..) and CSndUList logic.

yes, I have readed this logic;
but consider that:
the function:
checkRexmitTimer will call m_pSndUList->update after m_iReXmitCount * rtt_syn + COMM_SYN_INTERVAL_US + count_microseconds(m_tdSendInterval), no matter packet loss is occured or not(may be m_tdSendInterval is very long, the sender do not send a packet to receiver, so the receiver will not send ACK to sender too);
if update function is called,m_pTimer->interrupt() may be called; this may let the sender worker sent out a packet, which m_tsNextSendTime may not reallay match the "right" NextSendTime;

so I think, this may be a bug.

@cg82616424
Copy link
Contributor Author

so, as I described above.
If we dont want this problem affect Rexmit, we at least do not let function update called if Rexmit do not really happen, right?

@cg82616424
Copy link
Contributor Author

Hi @maxsharabayko is this really a bug?

@maxsharabayko maxsharabayko modified the milestones: v1.4.4, Backlog Aug 12, 2021
@cg82616424 cg82616424 closed this Oct 20, 2021
@cg82616424
Copy link
Contributor Author

close it

@mbakholdina mbakholdina modified the milestones: Backlog, v1.4.5 Apr 13, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants