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

Removed payload size dependency and option #1130

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Feb 13, 2020

Fixes #876

@ethouris ethouris self-assigned this Feb 13, 2020
@ethouris ethouris added [core] Area: Changes in SRT library core Impact: Significant Priority: Low Type: Maintenance Work required to maintain or clean up the code labels Feb 13, 2020
@ethouris ethouris added this to the v1.5.0 milestone Feb 13, 2020
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.

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?

@ethouris
Copy link
Collaborator Author

In general:

The SRTO_PAYLOADSIZE option is completely removed, this setting is ignored, the option itself only remains as deprecated.

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:

calcMaxPayloadSize() - simply to calculate maximum payload size in a packet, regarding the MSS size, SRT header and possibly FEC. This option may only be used in the connecting and configuring phase; during connection it should rely on the precalculated - and no longer changeable - value stored in m_zUserPayloadSize.
userPayloadSize() - the cache of the above value per packet in m_zUserPayloadSize, available externally (mainly for CC).

@mbakholdina mbakholdina modified the milestones: v1.5.0, v1.5.0 - Sprint 17 Jun 11, 2020
@mbakholdina
Copy link
Collaborator

mbakholdina commented Jun 11, 2020

@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.

@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 17, v1.5.0 Jun 11, 2020
@maxsharabayko
Copy link
Collaborator

Is it still possible to limit the payload size to 1316?

@ethouris It feels like it will not be possible to limit to e.g. 1316 if needed then?

@ethouris
Copy link
Collaborator Author

Yes. But I don't think anyone uses this limit for anything. At best you can limit MSS.

@ethouris ethouris removed the request for review from rndi March 15, 2021 10:39
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 Priority: Low Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SRT default maximum live payload size 1316 -> 1456
3 participants