-
Notifications
You must be signed in to change notification settings - Fork 867
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
Removed payload size dependency and option #1130
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide a description on the changes this PR introduces, and estimate the impact of those changes on the behavior of SRT in live mode.
E.g. is the average payload size calculated correctly?
Which payload size is used to calculate byte statistics for missing packets: default or average?
Is is still possible to limit the payload size to 1316?
In general: The There's still an internally calculated payload size that is needed for various calculations in congestion control module. This is then calculated as starting from the maximum possible value. Maximum possible value is simply MSS minus UDP and SRT header size, although it also undergoes correction with the extra FEC header in case when filter is on. Also, when payloadsize is set to a nonzero value, which is required for correct functioning of the live CC, it's still checked if a single sending function doesn't try to send more at once. New functions:
|
@maxsharabayko Please review the PR, consider the issue #1322 while reviewing and let @ethouris know if any additional tests should be done on his side with regards to #1322. |
@ethouris It feels like it will not be possible to limit to e.g. 1316 if needed then? |
Yes. But I don't think anyone uses this limit for anything. At best you can limit MSS. |
Fixes #876