[FFmpeg-devel] [PATCH] avformat/opensrt: add Haivision Open SRT protocol

[Michael Niedermayer]

On Mon, Jan 15, 2018 at 09:02:01AM -0500, Nablet Developer wrote:
> protocol requires libsrt (https://github.com/Haivision/srt) to be
> installed

This overall looks good, a few issues/suggestions i spoted are below:

[...]

> + at table @option
> + at item conntimeo=@var{milliseconds}
> +Connection timeout, in milliseconds. SRT cannot connect for RTT > 1500 msec
> +(2 handshake exchanges) with the default connect timeout of 3 seconds. This option
> +applies to the caller and rendezvous connection modes. The connect timeout is 10 times
> +the value set for the rendezvous mode (which can be used as a workaround for this
> +connection problem with earlier versions).

newly added time based options should be in seconds
the user can achive micro or milli seconds by using suffixes
also see AV_OPT_TYPE_DURATION, it may fit here


> +
> + at item fc=@var{bytes}
> +Flight Flag Size (Window Size), in bytes. FC is actually an internal parameter and
> +you should set it to not less than @option{recv_buffer_size} and @option{mss}.
> +The default value is relatively large, therefore unless you set a very large
> +receiver buffer, you do not need to change this option. Default value is 25600.
> +
> + at item inputbw=@var{bytes/seconds}
> +Sender nominal input rate, in bytes per seconds. Used along with @option{oheadbw},
> +when @option{maxbw} is set to relative (0), to calculate maximum sending rate when
> +recovery packets are sent along with main media stream:
> + at option{inputbw} * (100 + @option{oheadbw}) / 100
> +if @option{inputbw} is not set while @option{maxbw} is set to relative (0), the actual
> +ctual input rate is evaluated inside the library. Default value is 0.
> +
> + at item iptos=@var{tos}
> +IP Type of Service. Applies to sender only. Default value is 0xB8.
> +
> + at item ipttl=@var{ttl}
> +IP Time To Live. Applies to sender only. Default value is 64.
> +
> + at item listen_timeout=@var{milliseconds}
> +Set listen timeout, expressed in milliseconds.
> +
> + at item maxbw=@var{bytes/seconds}
> +Maximum sending bandwidth, in bytes per seconds.
> +-1 infinite (CSRTCC limit is 30mbps)
> +0 relative to input rate (see @option{inputbw})
> +>0 absolute limit value
> +Default value is 0 (relative)
> +

> + at item mode=@var{0|1|2}
> +Connection mode.
> +0 (caller) opens client connection.
> +1 (listener) starts server to listen for incoming connections.
> +2 (rendezvous) use Rendez-Vous connection mode.
> +Default valus is 0 (caller).

This looks like the user would have to use litteral integers
teh code supports the named identifers in brackets directly.


> +
> + at item mss=@var{bytes}
> +Maximum Segment Size, in bytes. Used for buffer allocation and rate calculation using
> +packet counter assuming fully filled packets. The smallest MSS between the peers is
> +used. This is 1500 by default in the overall internet. This is the maximum size of the
> +UDP packet and can be only decreased, unless you have some unusual dedicated network
> +settings. Default value is 1500.
> +
> + at item nakreport=@var{1|0}
> +If set to 1, Receiver will send `UMSG_LOSSREPORT` messages periodically until the
> +lost packet is retransmitted or intentionally dropped. Default value is 1.
> +
> + at item oheadbw=@var{percents}
> +Recovery bandwidth overhead above input rate, in percents. See @option{inputbw}.
> +Default value is 25%.
> +
> + at item passphrase=@var{string}
> +HaiCrypt Encryption/Decryption Passphrase string, length from 10 to 79 characters.
> +The passphrase is the shared secret between the sender and the receiver.
> +It is used to generate the Key Encrypting Key using PBKDF2 (Password-Based
> +Key Deriviation Function). It is used only if @option{pbkeylen} is non-zero.
> +t is used on the receiver only if the received data is encrypted.
> +The configured passphrase cannot be get back (write-only).
> +
> + at item pbkeylen=@var{bytes}
> +Sender encryption key length, in bytes. Only can be set to 0, 16, 24 and 32.
> +Enable sender encryption if not 0. Not required on receiver (set to 0),
> +key size obtained from sender in HaiCrypt handshake. Default value is 0.
> +
> + at item recv_buffer_size=@var{bytes}
> +Set receive buffer size, expressed bytes.
> +
> + at item send_buffer_size=@var{bytes}
> +Set send buffer size, expressed bytes.
> +
> + at item timeout=@var{microseconds}
> +Set raise error timeout, expressed in microseconds.
> +
> +This option is only relevant in read mode: if no data arrived in more
> +than this time interval, raise error.
> +
> + at item tlpktdrop=@var{1|0}
> +Too-late Packet Drop. When enabled on receiver, it skips missing packets that
> +have not been delivered in time and deliver the following packets to the application
> +when their time-to-play has come. It also send a fake ACK to sender. When enabled on
> +sender and enabled on the receiving peer, sender drops the older packets that have no
> +chance to be delivered in time. It was automatically enabled in sender if receiver
> +supports it.
> +

> + at item tsbpddelay=@var{milliseconds}
> +Timestamp-based Packet Delivery Delay, in milliseconds.
> +Used to absorb burst of missed packet retransmission.

please use a seconds based parameter.
Its very confusing if each parameter has a different base unit across the codebase


[...]

> +static int opensrt_listen_connect(int fd, const struct sockaddr *addr, socklen_t addrlen, int timeout, URLContext *h, int will_try_next)
> +{
> +    struct pollfd p = {fd, POLLOUT, 0};
> +    int ret;
> +    socklen_t optlen;
> +
> +    if (opensrt_socket_nonblock(fd, 1) < 0)
> +        av_log(NULL, AV_LOG_DEBUG, "ff_socket_nonblock failed\n");

av_log() should idealy use a context so the developer and user knows where a
message has come from


thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180127/6a179345/attachment.sig>