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

Clarifications regarding control packets without control information field (CIF) #118

Open
sthilden opened this issue Apr 3, 2023 · 3 comments

Comments

@sthilden
Copy link
Contributor

sthilden commented Apr 3, 2023

Reading the specification for control packets, I want to ask about some clarification regarding packets without control information field (CIF).

For the packet types Keep-Alive, Shutdown, and AckAck, this draft specifies that the "control packets do not contain Control Information Field (CIF)". For the Peer Error packet, this is not explicitly specified, but no Control Field Information is shown in the packet structure.

However, the Haivision SRT library does insert a 32-bit word of zeros as the CIF for all these packet types. Also, in the Haivision SRT Technical Overview document from 2018, it is specified that the packet types Keep-Alive, Shutdown, and AckAck contain a 32-bit word of zeros as the control information field (CIF).

Sending packets without any CIF padding to the Haivison SRT library seems to work just fine. The library accepts the packets with or without the zero padding, at least for Keep-Alive, Shutdown, and AckAck, which I have tested.

As the Haivision SRT library inserts a 32-bit word of zero padding, it means another SRT implementation must support that there could be padding present, and not discard packets with padding as an invalid packet.

So, I wonder about the following;

  • Is the current draft correct in that there should be no control information field (CIF) for Keep-Alive, Shutdown, AckAck, and, Peer Error packets? In that case, it should be specified that an implementation must assume that there can be CIF data present, to ensure compatibility with the Haivision SRT library.
  • Or, should the draft specify that there should actually be a 32-bit word of zero padding present as the CIF?
@maxsharabayko
Copy link
Collaborator

Hi @sthilden
Thanks for raising an important question!
Indeed, the SRT library accepts control packets both with or without zero padding. I am not sure what's the benefit of having the padding. This might be a legacy of the UDT library, that can be removed given existing receivers of control packets ignore this padding.
Potentially those packets could be extended in the future to contain CIF. For example, the SHUTDOWN packet may be extended with the "reason for shutdown". The modified structure may be signaled using the "subtype" field.

Answering your question, I believe there should be no control information field (CIF) for Keep-Alive, Shutdown, AckAck, and, Peer Error packets. Obviously, there MAY be this zero padding, and the specification cannot enforce the opposite.

Note that Wireshark's SRT dissector currently expects zero padding and reports a warning if it is not there. That's because the dissector was derived from the UDT dissector with this parsing of zero padding. Something to fix as well.

@sthilden
Copy link
Contributor Author

Thank you for your answer, @maxsharabayko!

To ensure that other implementations are compatible with the existing versions of the open source SRT library, I think it would be beneficial to clarify in the specification that there may be padding.

At least to me, e.g. the sentence "Shutdown control packets do not contain Control Information Field (CIF)." suggests that including any CIF data for a shutdown packet would be invalid.

I suggest to include something in the specification to clarify this, either that

  1. specifically, there may be 32-bits of trailing zero-padding in the control packets defined without CIF, and this should be ignored, or
  2. there may be trailing padding / trailing data to any control packets, and this should be ignored

I am not sure what is the correct way to go, though. Please let me know your thoughts on this.

@maxsharabayko
Copy link
Collaborator

I am thinking your suggestion no. 2 might be better, to allow padding up to the MTU side.
Possible future format extensions may be signaled using the "subtype" field.

there may be trailing padding / trailing data to any control packets, and this should be ignored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants