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

Address comments after review of the version 01 by Mikolaj #61

Open
5 of 13 tasks
maxsharabayko opened this issue Sep 3, 2020 · 0 comments
Open
5 of 13 tasks

Address comments after review of the version 01 by Mikolaj #61

maxsharabayko opened this issue Sep 3, 2020 · 0 comments
Assignees

Comments

@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Sep 3, 2020

  • Page 7:

The diagram on Figure 2 shows CIF mark though the contents is described as "Packet Contents (depends on the packet type)".

  • Page 10:

Version: You forgot to mention that per backward compatibility the caller must set always version 4 here for the induction
handshake, regardless of what version it currently supports. The listener must respond with version 5, if it supports
version 5 (or actually the highest version it supports). This is because of the existing implementations with version 4
where the listener copies the exact value passed by the caller, so passing version 4 is the only possibility to recognize it.
In the conclusion handshake the caller side should this time show the real version it supports.

  • Page 11:

Extension Field (16 bits):

The listener sets here the value designated as SRT MAGIC CODE, 0x4A17. This is necessary to make sure that the listener
that responds with version 5 is really an SRT listener supporting handshake version 5, not any independent derivative
work of UDT library that has also increased version number to 5. The value is an attempt to encode the "Haivision" name
with hex ciphers, initially it was 0x4A171510, but the number was cut to 16 bits due to the need of adding the encryption
field.

Not that all I have written here deserves to be put into the description, just wanted to make you get the motivation :)

HANDSHAKE TYPE.

You haven't read the comment attached to the symbols, especially for DONE:

URQ_DONE = -3,      // Special value used only in state-switching, to state that nothing should be sent in response

I personally advice that you put these types in the following order:

INDUCTION
WAVEAHAND
CONCLUSION
AGREEMENT

and DONE should not be in this list at all.

  • Page 20:

Really you don't have to paste the whole layout of the control packet there. You can refer to the general layout
description of the control packet and the TSI (type-specific information) and CIF fields. It should be also enough
if you describe the overall rules for the control packet, that is:

  1. For standard control packets, the command type is set in "Control type" field and "Reserved" is 0.

  2. For extended control packets, the command type is UMSG_EXT (0x7FFF) and the command type is in "Reserved".

Having that, you should be clear for, e.g., UMSG_KEEPALIVE that you only need to describe the TSI and CIF fields
as, well, filled with zeros.

In control packets the timestamp is usually the time when the packet was sent, and the destination socket ID is
always the same. It is necessary to dispatch correctly the packet to the socket to which it is destined, if multiple
sockets are bound to the same port.

Also in other packets: the TSI field is usually in use, and if not, you should specify that it must be zero.

  • Page 21:

LAPSN: Do you have the definition of "acknowledgement" anywhere and specified that the "last acknowledged packet"
is the last packet received as contiguous (there is no loss between this packet and the previous acknowledged packet)?

  • Page 22:

It's better to say "range of lost packets" not "List of lost packets" because the "list" is whole contents of
UMSG_LOSSREPORT type message.

  • Page 24:

The TSI field doesn't contain the ACK sequence number, but the ACK Journal, the same exactly value as it is
described in TSI by the UMSG_ACK message.

  • Page 24:, 4.2.1 (Message Mode)

I'd mention here that the Message Mode is a concept used by the SCTP protocol, whereas the Stream Mode is the
mode used by TCP protocol.

  • Page 25:

Initiator is the side that first sends the handshake message of type URQ_CONCLUSION, not "extended handshake".

  • Page 27:

The fragment describing the encryption configuration advertisement is mentioned kinda out-of-the-blue. I'd keep
here the shortest possible mention that there's the key length advertisement placed in this field and the whole
procedure will be described by the KMX process description. This as well should be moved to 4.3.1.1, as this
advertisement takes place exclusively in the induction phase (also waveahand, in case of rendezvous).

  • Page 28:

"Encryption Field: Advertised cipher family and block size"

I'd enumerate here the possible values, including 0, which means that the value is not advertised.

  • Page 29:

I'd really describe the cookie preparation process separately and then refer to it from both caller-listener
handshake process, as SYN cookie, and for rendezvous HSv5 and "cookie contest".

  • Page 35:

The diagram at Figure 17 makes a user a false impression that the latency is counted towards the reception
time. This actually isn't true - SRT latency is counted towards the EXPECTED RECEPTION TIME, which is calculated
out of the connection start time and the packet's timestamp. To this time there's added the value of the
latency and this results in delivery time. Note also that the moment when the packet is to be ready to play
and the moment when the application plays it, also exists, and it's a similar value as the one designated
as "Sending Delay" (the value that is dependent on the timer accuracy, system performance, and thread layout).

@maxsharabayko maxsharabayko self-assigned this Sep 3, 2020
@mbakholdina mbakholdina changed the title Review by Mikolaj Address comments after review of the version 01 by Mikolaj Feb 12, 2021
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

1 participant