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

Marton Balint cus at passwd.hu
Wed Dec 23 23:29:45 EET 2020



On Wed, 23 Dec 2020, 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      |  29 +++++
> libavformat/Makefile    |   1 +
> libavformat/librist.c   | 227 ++++++++++++++++++++++++++++++++++++++++
> libavformat/protocols.c |   1 +
> 5 files changed, 263 insertions(+)
> create mode 100644 libavformat/librist.c
>
> diff --git a/configure b/configure
> index 90914752f1..462f2dcf64 100755
> --- a/configure
> +++ b/configure
> @@ -259,6 +259,7 @@ External library support:
>   --enable-libpulse        enable Pulseaudio input via libpulse [no]
>   --enable-librabbitmq     enable RabbitMQ library [no]
>   --enable-librav1e        enable AV1 encoding via rav1e [no]
> +  --enable-librist         enable RIST via librist [no]
>   --enable-librsvg         enable SVG rasterization via librsvg [no]
>   --enable-librubberband   enable rubberband needed for rubberband filter [no]
>   --enable-librtmp         enable RTMP[E] support via librtmp [no]
> @@ -1797,6 +1798,7 @@ EXTERNAL_LIBRARY_LIST="
>     libpulse
>     librabbitmq
>     librav1e
> +    librist
>     librsvg
>     librtmp
>     libshine
> @@ -3488,6 +3490,8 @@ unix_protocol_select="network"
> # external library protocols
> libamqp_protocol_deps="librabbitmq"
> libamqp_protocol_select="network"
> +librist_protocol_deps="librist"
> +librist_protocol_select="network"
> librtmp_protocol_deps="librtmp"
> librtmpe_protocol_deps="librtmp"
> librtmps_protocol_deps="librtmp"
> @@ -6404,6 +6408,7 @@ enabled libopus           && {
> enabled libpulse          && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new
> enabled librabbitmq       && require_pkg_config librabbitmq "librabbitmq >= 0.7.1" amqp.h amqp_new_connection
> enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.1.0" rav1e.h rav1e_context_new
> +enabled librist           && require_pkg_config librist "librist >= 0.2" librist/librist.h rist_receiver_create
> enabled librsvg           && require_pkg_config librsvg librsvg-2.0 librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo
> enabled librtmp           && require_pkg_config librtmp librtmp librtmp/rtmp.h RTMP_Socket
> enabled librubberband     && require_pkg_config librubberband "rubberband >= 1.8.1" rubberband/rubberband-c.h rubberband_new -lstdc++ && append librubberband_extralibs "-lstdc++"
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index de377a9546..58627040f5 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -673,6 +673,35 @@ 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 receiving and sending data.
> +
> + at item log_level
> +Set loglevel for RIST logging messages.
> +
> + at item secret
> +Set override of encryption secret, by default is unset.
> +
> + at item encryption
> +Set encryption type, by default is disabled.
> +Acceptable values are 128 and 256.

What about 192?

> + at end table

Missing docs for pkt_size.

Some usage examples could be useful.

> +
> @section rtmp
> 
> Real-Time Messaging Protocol.
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 97d868081b..799e16c59e 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -652,6 +652,7 @@ OBJS-$(CONFIG_UNIX_PROTOCOL)             += unix.o
> 
> # external library protocols
> OBJS-$(CONFIG_LIBAMQP_PROTOCOL)          += libamqp.o
> +OBJS-$(CONFIG_LIBRIST_PROTOCOL)          += librist.o
> OBJS-$(CONFIG_LIBRTMP_PROTOCOL)          += librtmp.o
> OBJS-$(CONFIG_LIBRTMPE_PROTOCOL)         += librtmp.o
> OBJS-$(CONFIG_LIBRTMPS_PROTOCOL)         += librtmp.o
> diff --git a/libavformat/librist.c b/libavformat/librist.c
> new file mode 100644
> index 0000000000..dbffb5cdc1
> --- /dev/null
> +++ b/libavformat/librist.c
> @@ -0,0 +1,227 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * Reliable Internet Streaming Transport protocol
> + */
> +
> +#include "libavutil/avassert.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/parseutils.h"
> +#include "libavutil/time.h"
> +
> +#include "avformat.h"
> +#include "internal.h"
> +#include "network.h"
> +#include "os_support.h"
> +#include "url.h"
> +
> +#include <librist/librist.h>
> +
> +typedef struct RISTContext {
> +    const AVClass *class;
> +    int fd;

uint32_t. Also you should probably name this flow_id, not fd, as it is not 
a file descriptor.

> +
> +    int profile;
> +    int buffer_size;
> +    int packet_size;
> +    int log_level;
> +    int encryption;
> +    char *secret;
> +
> +    struct rist_logging_settings *logging_settings;
> +    struct rist_peer_config *peer_config;
> +    struct rist_peer *peer;
> +    struct rist_ctx *rist_ctx;
> +} RISTContext;
> +
> +#define D AV_OPT_FLAG_DECODING_PARAM
> +#define E AV_OPT_FLAG_ENCODING_PARAM
> +#define OFFSET(x) offsetof(RISTContext, x)
> +static const AVOption librist_options[] = {
> +    { "rist_profile","set profile",     OFFSET(profile),     AV_OPT_TYPE_INT,   {.i64=RIST_PROFILE_MAIN},     0, 2, .flags = D|E, "profile" },
> +    { "simple",      NULL,              0,                   AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_SIMPLE},   0, 0, .flags = D|E, "profile" },
> +    { "main",        NULL,              0,                   AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_MAIN},     0, 0, .flags = D|E, "profile" },
> +    { "advanced",    NULL,              0,                   AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_ADVANCED}, 0, 0, .flags = D|E, "profile" },
> +    { "buffer_size", "set buffer_size", OFFSET(buffer_size), AV_OPT_TYPE_INT,   {.i64=0},                     0, INT_MAX, .flags = D|E },
> +    { "pkt_size",    "set packet size", OFFSET(packet_size), AV_OPT_TYPE_INT,   {.i64=INT_MAX},               1, INT_MAX, .flags =   E },

INT_MAX for pkt_size is probably not a good default. pkt_size is used (at 
least in other protocols) to determine the maximum allowed packet size for 
output, and the maximum supported packet size for input. I am not sure 
what you can pump into RIST (mpegts most likely?) but probably it should 
be determined for that usage. e.g. 1316?

Also the URLContext->max_packet_size should be set based on the 
packet_size.

> +    { "log_level",   "set loglevel",    OFFSET(log_level),   AV_OPT_TYPE_INT,   {.i64=-1},                   -1, INT_MAX, .flags = D|E },
> +    { "secret", "set encryption secret",OFFSET(secret),      AV_OPT_TYPE_STRING,{.str=""},                    0, 0,       .flags = D|E },

default should rather be NULL, no?

> +    { "encryption","set encryption type",OFFSET(encryption), AV_OPT_TYPE_INT   ,{.i64=0},                     0, INT_MAX, .flags = D|E },
> +    { NULL }
> +};
> +
> +static int log_cb(void *arg, enum rist_log_level log_level, const char *msg)
> +{
> +    int level;
> +
> +    switch (log_level) {
> +    case RIST_LOG_ERROR:    level = AV_LOG_ERROR;   break;
> +    case RIST_LOG_WARN:     level = AV_LOG_WARNING; break;
> +    case RIST_LOG_NOTICE:   level = AV_LOG_VERBOSE; break;
> +    case RIST_LOG_INFO:     level = AV_LOG_INFO;    break;
> +    case RIST_LOG_DEBUG:    level = AV_LOG_DEBUG;   break;
> +    case RIST_LOG_DISABLE:  level = AV_LOG_QUIET;   break;
> +    case RIST_LOG_SIMULATE: level = AV_LOG_TRACE;   break;
> +    default: level = AV_LOG_PANIC;
> +    }
> +
> +    av_log(arg, level, "%s", msg);
> +
> +    return 0;
> +}
> +
> +static int librist_open(URLContext *h, const char *uri, int flags)
> +{
> +    RISTContext *s = h->priv_data;
> +    int ret;
> +
> +    s->fd = rist_flow_id_create();
> +
> +    ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb, h, NULL, stderr);

stderr should not be given, because we are using our own log callback.

> +    if (ret < 0)
> +        return ret;

RIST errors should be mapped to ffmpeg errors. It seems rist is using -1 
everywhere, so that should be mapped to AVERROR_EXTERNAL if there is no 
way to acquire more precise error.

You are leaking logging_settings, and a lot of other stuff on failure 
paths after this. url_close is only called automatically if url_open was 
successful.

> +
> +    if (flags & AVIO_FLAG_WRITE) {
> +        ret = rist_sender_create(&s->rist_ctx, s->profile, s->fd, s->logging_settings);
> +        if (ret < 0)
> +            return ret;
> +    } else {
> +        ret = rist_receiver_create(&s->rist_ctx, s->profile, s->logging_settings);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    ret = rist_parse_address(uri, (void *)&s->peer_config);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (((s->encryption == 128 || s->encryption == 256) && !s->secret) ||
> +        ((s->peer_config->key_size == 128 || s->peer_config->key_size == 256) && !s->peer_config->secret[0])) {
> +        av_log(h, AV_LOG_ERROR, "secret is mandatory if encryption is enabled\n");
> +        return AVERROR(EINVAL);
> +    }

192-byte encryption is also supported as far as I see.

> +
> +    if (s->secret && s->peer_config->secret[0] == 0)
> +        strncpy(s->peer_config->secret, s->secret, FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret)));

av_strlcpy

> +
> +    if (s->secret && (s->encryption == 128 || s->encryption == 256))
> +        s->peer_config->key_size = s->encryption;

Some error handling should be done, e.g. reject encryption sizes not 
supported, reject encryption without secret given, etc.

> +
> +    if (s->buffer_size) {
> +        s->peer_config->recovery_length_min = s->buffer_size;
> +        s->peer_config->recovery_length_max = s->buffer_size;
> +    }
> +
> +    ret = rist_peer_create(s->rist_ctx, &s->peer, s->peer_config);
> +    if (ret < 0)
> +        return ret;
> +
> +    ret = rist_start(s->rist_ctx);
> +    if (ret < 0)
> +        return ret;
> +
> +    return 0;
> +}
> +
> +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->rist_ctx, &data_block, 5);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (ret > 0) {
> +        size = FFMIN(size, data_block->payload_len);
> +        memcpy(buf, data_block->payload, size);

Some error message about the truncation should be logged, or maybe a hard 
error is even better if a truncation would occur?

> +        return size;
> +    }
> +
> +    return 0;

This should return AVERROR(EAGAIN).

> +}
> +
> +static int librist_write(URLContext *h, const uint8_t *buf, int size)
> +{
> +    RISTContext *s = h->priv_data;
> +    struct rist_data_block data_block = { 0 };
> +    int ret;
> +
> +    data_block.ts_ntp = 0;
> +    data_block.payload = buf;
> +    data_block.payload_len = FFMIN(s->packet_size, size);

If you set h->max_packet_size then this is ensured by the framework.

> +    ret = rist_sender_data_write(s->rist_ctx, &data_block);
> +    if (ret < 0)
> +        return ret;
> +    else if (ret)
> +        return ret;
> +    else // remove this part when new librist release appears
> +        return size;
> +}
> +
> +static int librist_close(URLContext *h)
> +{
> +    RISTContext *s = h->priv_data;
> +    int ret;
> +
> +    s->peer = NULL;
> +
> +    ret = rist_destroy(s->rist_ctx);
> +    if (ret < 0)
> +        return ret;
> +    s->rist_ctx = NULL;
> +
> +    if (s->peer_config)
> +        free((void *)s->peer_config);
> +    s->peer_config = NULL;
> +
> +    if (s->logging_settings)
> +        free((void *)s->logging_settings);
> +    s->logging_settings = NULL;
> +
> +    return 0;
> +}
> +
> +static int librist_get_file_handle(URLContext *h)
> +{
> +    RISTContext *s = h->priv_data;
> +
> +    return s->fd;
> +}

I don't think this is right, s->fd is a flow id, not an ordinary file 
descriptor. You probably don't need this callback anyway.

> +
> +static const AVClass librist_class = {
> +    .class_name = "librist",
> +    .item_name  = av_default_item_name,
> +    .option     = librist_options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
> +const URLProtocol ff_librist_protocol = {
> +    .name                = "rist",
> +    .url_open            = librist_open,
> +    .url_read            = librist_read,
> +    .url_write           = librist_write,
> +    .url_close           = librist_close,
> +    .url_get_file_handle = librist_get_file_handle,
> +    .priv_data_size      = sizeof(RISTContext),
> +    .flags               = URL_PROTOCOL_FLAG_NETWORK,
> +    .priv_data_class     = &librist_class,
> +};
> diff --git a/libavformat/protocols.c b/libavformat/protocols.c
> index 7df18fbb3b..c4fc446bb6 100644
> --- a/libavformat/protocols.c
> +++ b/libavformat/protocols.c
> @@ -61,6 +61,7 @@ extern const URLProtocol ff_udp_protocol;
> extern const URLProtocol ff_udplite_protocol;
> extern const URLProtocol ff_unix_protocol;
> extern const URLProtocol ff_libamqp_protocol;
> +extern const URLProtocol ff_librist_protocol;
> extern const URLProtocol ff_librtmp_protocol;
> extern const URLProtocol ff_librtmpe_protocol;
> extern const URLProtocol ff_librtmps_protocol;

Regards,
Marton


More information about the ffmpeg-devel mailing list