[FFmpeg-devel] [PATCH] libavformat: add librist protocol

[Marton Balint]

On Tue, 26 Jan 2021, Paul B Mahol wrote:

> This work is sponsored by Open Broadcast Systems.
>
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
> configure               |   5 +
> doc/protocols.texi      |  32 +++++
> libavformat/Makefile    |   1 +
> libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
> libavformat/protocols.c |   1 +
> 5 files changed, 290 insertions(+)
> create mode 100644 libavformat/librist.c


[...]


> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -690,6 +690,38 @@ Example usage:
> -f rtp_mpegts -fec prompeg=l=8:d=4 rtp://@var{hostname}:@var{port}
> @end example
> 
> + at section rist
> +
> +Reliable Internet Streaming Transport protocol
> +
> +The accepted options are:
> + at table @option
> + at item rist_profile
> +Supported values:
> + at table @samp
> + at item simple
> + at item main
> +This one is default.
> + at item advanced
> + at end table
> +
> + at item buffer_size
> +Set internal RIST buffer size for retransmission of data.
> +
> + at item pkt_size
> +Set internal RIST buffer size for receiving and sending data.

You should mention the default here. And by the way, why have you selected
9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
makes more sense no?

What you can also do is to make -1 the default, and then make it 1316 for
write mode and 9968 otherwise, if really there is a common use for bigger
packets.

[..]

> +static int librist_close(URLContext *h)
> +{
> +    RISTContext *s = h->priv_data;
> +    int ret = 0;
> +
> +    s->read_peer = NULL;
> +    s->write_peer = NULL;
> +
> +    if (s->write_ctx)
> +        ret = rist_destroy(s->read_ctx);
> +    if (s->read_ctx)
> +        ret = rist_destroy(s->read_ctx);
> +    s->read_ctx = NULL;
> +    s->write_ctx = NULL;
> +
> +    return risterr2ret(ret);

This returns an error even if there is no rist error.
Maybe you should simply return 0, and avoid assigning rets.

> +}
> +
> +static int librist_open(URLContext *h, const char *uri, int flags)
> +{
> +    RISTContext *s = h->priv_data;
> +    struct rist_logging_settings *logging_settings = &s->logging_settings;
> +    struct rist_peer_config *peer_config = &s->peer_config;
> +    int ret;
> +
> +    ret = rist_logging_set(&logging_settings, s->log_level, log_cb, h, NULL, NULL);
> +    if (ret < 0)
> +        return risterr2ret(ret);
> +
> +    if (flags & AVIO_FLAG_WRITE)
> +        ret = rist_sender_create(&s->write_ctx, s->profile, 0, logging_settings);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (flags & AVIO_FLAG_READ)
> +        ret = rist_receiver_create(&s->read_ctx, s->profile, logging_settings);
> +    if (ret < 0)
> +        goto err;

You should move these context creating downward to other read/write mode
checks.

> +
> +    ret = rist_peer_config_defaults_set(peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    ret = rist_parse_address(uri, (const struct rist_peer_config **)&peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (((s->encryption == 128 || s->encryption == 256) && !s->secret) ||
> +        ((peer_config->key_size == 128 || peer_config->key_size == 256) && !peer_config->secret[0])) {
> +        av_log(h, AV_LOG_ERROR, "secret is mandatory if encryption is enabled\n");
> +        librist_close(h);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (s->secret && peer_config->secret[0] == 0)
> +        av_strlcpy(peer_config->secret, s->secret, FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret)));

Simply av_strlcpy(peer_config->secret, s->secret, RIST_MAX_STRING_SHORT).

> +
> +    if (s->secret && (s->encryption == 128 || s->encryption == 256))
> +        peer_config->key_size = s->encryption;
> +
> +    if (s->buffer_size) {
> +        peer_config->recovery_length_min = s->buffer_size;
> +        peer_config->recovery_length_max = s->buffer_size;
> +    }
> +
> +    if (flags & AVIO_FLAG_READ)
> +        ret = rist_peer_create(s->read_ctx, &s->read_peer, &s->peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (flags & AVIO_FLAG_WRITE)
> +        ret = rist_peer_create(s->write_ctx, &s->write_peer, &s->peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (flags & AVIO_FLAG_READ)
> +        ret = rist_start(s->read_ctx);
> +    if (ret < 0)
> +        goto err;
> +    if (flags & AVIO_FLAG_WRITE)
> +        ret = rist_start(s->write_ctx);
> +    if (ret < 0)
> +        goto err;

Use a single check:

if (flags & AVIO_FLAG_READ) {
    rist_receiver_create()
    rist_peer_create()
    rist_start()
}
if (flags & AVIO_FLAG_WRITE) {
    rist_sender_create()
    rist_peer_create()
    rist_start()
}

Also make sure you are not leaking s->read_ctx/s->write_ctx on errors.

And does it really make sense to use READ and WRITE at the same time? Can
it work? Nicolas's original suggestion was to simply return error if both
flags (AVIO_FLAG_READWRITE) are set.

> +
> +    h->max_packet_size = s->packet_size;
> +
> +    return 0;
> +
> +err:
> +    librist_close(h);
> +
> +    return risterr2ret(ret);
> +}
> +
> +static int librist_read(URLContext *h, uint8_t *buf, int size)
> +{
> +    RISTContext *s = h->priv_data;
> +    const struct rist_data_block *data_block;
> +    int ret;
> +
> +    ret = rist_receiver_data_read(s->read_ctx, &data_block, POLLING_TIME);
> +    if (ret < 0)
> +        return risterr2ret(ret);
> +
> +    if (ret > 0 && data_block->payload) {

Is it possible that data_block->payload is NULL? If not, then either 
remove the check or make it an assert. If yes, then you are leaking 
data_block if it is indeed NULL.

> +        if (data_block->payload_len > size)
> +            av_log(h, AV_LOG_WARNING, "Part of datagram lost due to insufficient buffer size\n");
> +        size = FFMIN(size, data_block->payload_len);
> +        memcpy(buf, data_block->payload, size);
> +        rist_receiver_data_block_free(&data_block);
> +        return size;
> +    }
> +
> +    return AVERROR(EAGAIN);
> +}
> +

Thanks,
Marton