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

Fixed documentation on option restrictions #1759

Merged
merged 10 commits into from Feb 5, 2021

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Jan 25, 2021

Fixes #1747

Fixed description on options restrictions:

  1. "Binding" renamed to "Restrict", as it describes restrictions on setting an option
  2. Introduced "pre-bind" for options that must not be altered after binding
  3. Fixed SRTO_BINDTODEVICE that should be restricted as pre-bind.

There's a problem here though. Only some selected options should not be altered after binding, as binding applies some very specific settings that must be applied before and these settings apply to the binding process itself. ONLY those options should be "pre-bind", all others that are connection-restricted, should be "pre". Fixing this should be done also in the code so that it's connected state, not open state checked in the option setting procedure.

@ethouris ethouris added [core] Area: Changes in SRT library core [docs] Area: Improvements or additions to documentation Priority: High Type: Bug Indicates an unexpected problem or unintended behavior labels Jan 25, 2021
@ethouris
Copy link
Collaborator Author

ethouris commented Jan 25, 2021

Not every option must be pre-bind. This concerns exclusively options that:

  • Change the behavior and functionality of the srt_bind call
  • Concern or set option on the internally used UDP socket
  • Concern any kind of resource used by the multiplexer

The following options are currently marked pre-bind. Need to check those that REALLY HAVE TO be pre-bind. This is to be reviewed and options that are not checked here below should get the restrictions lifted up to "connected".

  • SRTO_IPTOS - applies to UDP socket
  • SRTO_IPTTL - applies to UDP socket
  • SRTO_IPV6ONLY - applies to UDP socket and binding rules
  • SRTO_LATENCY - no application to muxer
  • SRTO_NAKREPORT - no application to muxer
  • SRTO_PEERLATENCY - no application to muxer
  • SRTO_RCVBUF - applies to the buffer (multiplexer resource)
  • SRTO_RCVLATENCY - no application to muxer
  • SRTO_REUSEADDR - applies to binding rules
  • SRTO_SNDBUF - applies to the buffer (multiplexer resource)
  • SRTO_TLPKTDROP - no application to muxer
  • SRTO_TRANSTYPE - no application to muxer
  • SRTO_TSBPDMODE - no application to muxer
  • SRTO_UDP_RCVBUF - applies to UDP socket
  • SRTO_UDP_SNDBUF - applies to UDP socket

@maxsharabayko maxsharabayko added this to the v1.4.3 milestone Jan 25, 2021
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.

Consider minor comments.
Also would be good to split documentation change and SRTO_BINDTODEVICE into different commits for merging (or PRs).

Comment on lines 135 to 136
- `pre-bind`: Like `pre`, but the option is not allowed to be altered after
calling `srt_bind()`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe some note would be useful here, mentioning that srt_connect() will bind implicitly even if srt_bind(..) was not called.
Or:
"Like pre, but additionally the option is not allowed to be altered after calling srt_bind(), whichever happens first."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe even "pre-bind" should go before "pre" as more restrictive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it's more restrictive, but OTOH it's less common among options. Mentioning this bind-when-connect might be a good idea.

docs/APISocketOptions.md Show resolved Hide resolved
@maxsharabayko
Copy link
Collaborator

SRTO_LOSSMAXTTL could be a post option, to be able to change it in runtime if needed.
The same for SRTO_RETRANSMITALGO. Just thinking so far. 🤔

@ethouris
Copy link
Collaborator Author

Possible. Depends on whether altering this option could destroy some algorithm during the transmission, or refers to some resource of the multiplexer.

srtcore/core.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

Minor edits

Comment on lines 130 to 136
- `pre-bind`: The option cannot be altered on a socket that is already bound (by calling
`srt_bind()` or any other function doing this, including automatic binding when trying to
connect, as well as accepted sockets).
**NOTE**: The `pre-bind` characteristic applies exclusively to options that:
- Change the behavior and functionality of the `srt_bind` call
- Concern or set an option on the internally used UDP socket
- Concern any kind of resource used by the multiplexer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alignment issue

Screenshot 2021-02-04 at 15 04 37

Copy link
Collaborator

Choose a reason for hiding this comment

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

I moved the "NOTE" sentence up into the preceding paragraph because the extra indentation was applying code format to the text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, but it doesn't work. I think it would be simply easier if this whole NOTE with the enumeration is moved past the post entry as a new paragraph

Comment on lines +130 to +132
- `pre-bind`: The option cannot be altered on a socket that is already bound (by calling
`srt_bind()` or any other function doing this, including automatic binding when trying to
connect, as well as accepted sockets).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- `pre-bind`: The option cannot be altered on a socket that is already bound (by calling
`srt_bind()` or any other function doing this, including automatic binding when trying to
connect, as well as accepted sockets).
- `pre-bind`: The option cannot be altered on a socket that is already bound (by calling `srt_bind()` or any other function doing this, including automatic binding when trying to connect, as well as accepted sockets). The `pre-bind` characteristic applies exclusively to options that:
- change the behavior and functionality of the `srt_bind` call
- concern or set an option on the internally used UDP socket
- concern any kind of resource used by the multiplexer```

@maxsharabayko maxsharabayko merged commit dcd62ca into Haivision:master Feb 5, 2021
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 [docs] Area: Improvements or additions to documentation Priority: High Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APISocketOptions.md is missing an indicator for runtime-updatable options
3 participants