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] Fixed SRTO_LOSSMAXTTL setting on accepted socket. Fixes #735. #843

Merged
merged 2 commits into from
Sep 3, 2019

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Sep 2, 2019

When accepting an incomming connection, the copy constructor was not copying the m_iMaxReorderTolerance setting from the ancestor.

TODO:

  • Check other missing settings.

Fixes #735.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Sep 2, 2019
@maxsharabayko maxsharabayko added this to the v1.4.0 milestone Sep 2, 2019
@maxsharabayko
Copy link
Collaborator Author

Other options, that are not copied to the accepted socket:

  • m_lMinimumPeerSrtVersion
  • m_sStreamName
  • m_bReuseAddr is just set to true with the comment "this must be true, because all accepted sockets share the same port with the listener"
  • m_bTsbPd. Only m_bOPT_TsbPd is copied, but anyway after a construction the m_bTsbPd=m_bOPT_TsbPd in CUDT::open(), so let's say copies lazily, :)
  • m_lSrtVersion. Should not this option be read-only?

@ethouris
Copy link
Collaborator

ethouris commented Sep 2, 2019

Reviewing:

  • m_lMinimumPeerSrtVersion is something to be set on the listener socket or connecting socket and it's resolved during handshake. Doesn't need to be copied to an accepted socket, it's useless. It simply causes to check the SRT version of the peer during the handshake and compares it; when it's not >= this minimum, the connection is rejected

  • m_sStreamName - it's only copied from the connecting socket from the peer side into an accepted socket for that connection. This setting on listener socket should be completely ignored.

  • m_bReuseAddr - This is only influences the CUDTUnited::updateMux function so that it allows to reuse the channel, and this is called from connect and bind calls. Not sure what would happen in case when you set SRTO_REUSEADDR to off on a socket that is then next called srt_listen on.

  • m_bTsbPd - that's not exactly true, please refer to CUDT::fillSrtHandshake_HSREQ. Generally the intention is that this flag should be set to true only if:

    a) Agent has set SRTO_TSBPDMODE
    b) Peer has set the SRT HS flag SRT_OPT_TSBPDRCV

Otherwise it means that the peer doesn't wish to receive in TSBPD mode so it shouldn't be set.

  • m_lSrtVersion - yes, and that was predicted to be modifiable for development purposes only

@maxsharabayko maxsharabayko marked this pull request as ready for review September 2, 2019 14:44
@ethouris
Copy link
Collaborator

ethouris commented Sep 3, 2019

One more thing: it would be nice to review the situation of the listener socket against a caller socket that may try to reuse the address. I think the "address reusage" (in practice - this has nothing to do with the SRTO_REUSEADDR option) for the listener socket should be:

  • All accepted sockets do share the UDP socket with the listener, so this should always reuse address
  • A caller socket may be allowed or forbidden to share the UDP socket with a listener socket

And this option should be also controller by SRTO_REUSEADDR, that is:

  • When on, the listener socket is allowed to share UDP socket with any caller socket that tries to bind to the same address. There shouldn't be any problems with dispatching packets into it, although this feature probably wasn't properly tested (not sure what happens when a handshake response comes in with the socket addressed - connections use @0)
  • When off, the listener socket is not allowed to share UDP socket with any other socket, although all accepted sockets still do share the UDP socket with the listener socket and each other (as this is so by design).

To be considered: a feature that causes that accepted sockets get assigned their own dedicated local outgoing port and get their own UDP socket with all facilities (queues). This would be turned on by a separate flag (possibly SRTO_REUSEPORT when turned off).

@rndi rndi merged commit f426e9d into Haivision:master Sep 3, 2019
@maxsharabayko maxsharabayko deleted the hotfix/reorder-tolerance branch October 29, 2019 14:05
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.

srt-live-transmit - strange behaviour with jitter
3 participants