diff mbox series

[FFmpeg-devel] libavformat: add librist protocol

Message ID 20201223181029.17021-1-onemda@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] libavformat: add librist protocol
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Paul B Mahol Dec. 23, 2020, 6:10 p.m. UTC
This work is sponsored by Open Broadcast Systems.

Signed-off-by: Paul B Mahol <onemda@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

Comments

Marton Balint Dec. 23, 2020, 9:29 p.m. UTC | #1
On Wed, 23 Dec 2020, Paul B Mahol wrote:

> This work is sponsored by Open Broadcast Systems.
>
> Signed-off-by: Paul B Mahol <onemda@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
> 
> +@section rist
> +
> +Reliable Internet Streaming Transport protocol
> +
> +The accepted options are:
> +@table @option
> +@item rist_profile
> +Supported values:
> +@table @samp
> +@item simple
> +@item main
> +This one is default.
> +@item advanced
> +@end table
> +
> +@item buffer_size
> +Set internal RIST buffer size for receiving and sending data.
> +
> +@item log_level
> +Set loglevel for RIST logging messages.
> +
> +@item secret
> +Set override of encryption secret, by default is unset.
> +
> +@item encryption
> +Set encryption type, by default is disabled.
> +Acceptable values are 128 and 256.

What about 192?

> +@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
Paul B Mahol Dec. 23, 2020, 9:59 p.m. UTC | #2
On Wed, Dec 23, 2020 at 10:30 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Wed, 23 Dec 2020, Paul B Mahol wrote:
>
> > This work is sponsored by Open Broadcast Systems.
> >
> > Signed-off-by: Paul B Mahol <onemda@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
> >
> > +@section rist
> > +
> > +Reliable Internet Streaming Transport protocol
> > +
> > +The accepted options are:
> > +@table @option
> > +@item rist_profile
> > +Supported values:
> > +@table @samp
> > +@item simple
> > +@item main
> > +This one is default.
> > +@item advanced
> > +@end table
> > +
> > +@item buffer_size
> > +Set internal RIST buffer size for receiving and sending data.
> > +
> > +@item log_level
> > +Set loglevel for RIST logging messages.
> > +
> > +@item secret
> > +Set override of encryption secret, by default is unset.
> > +
> > +@item encryption
> > +Set encryption type, by default is disabled.
> > +Acceptable values are 128 and 256.
>
> What about 192?
>

 Unofficial.


>
> > +@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.
>

But it is used for same thing.


>
> > +
> > +    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?
>

Same thing.


>
> > +    { "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.
>

Not used, thus not really relevant.


>
> > +    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.
>

Not officially.


>
> > +
> > +    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.
>

That is already handled.


>
> > +
> > +    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.
>

Are you really sure it is not needed?


> > +
> > +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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Dec. 23, 2020, 10:29 p.m. UTC | #3
Marton Balint (12020-12-23):
> > +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.

It is indeed not a file descriptor at all, returning it like that would
cause very serious bugs.

It seems this library does not make its file descriptors at all. It
means it cannot be used by FFmpeg in non-blocking mode, nor by any
application with a standard Unix event loop.

Regards,
Paul B Mahol Dec. 23, 2020, 10:35 p.m. UTC | #4
On Wed, Dec 23, 2020 at 11:29 PM Nicolas George <george@nsup.org> wrote:

> Marton Balint (12020-12-23):
> > > +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.
>
> It is indeed not a file descriptor at all, returning it like that would
> cause very serious bugs.
>
>
I nowhere see proof that this cause any bugs.


> It seems this library does not make its file descriptors at all. It
> means it cannot be used by FFmpeg in non-blocking mode, nor by any
> application with a standard Unix event loop


This is fortunately completely false statement. It works just fine.

.
>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Dec. 23, 2020, 10:39 p.m. UTC | #5
Paul B Mahol (12020-12-23):
> This work is sponsored by Open Broadcast Systems.

Thanks for the disclosure.

> 
> Signed-off-by: Paul B Mahol <onemda@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
>  
> +@section rist
> +
> +Reliable Internet Streaming Transport protocol
> +
> +The accepted options are:
> +@table @option
> +@item rist_profile
> +Supported values:
> +@table @samp
> +@item simple
> +@item main
> +This one is default.
> +@item advanced
> +@end table
> +
> +@item buffer_size
> +Set internal RIST buffer size for receiving and sending data.
> +
> +@item log_level
> +Set loglevel for RIST logging messages.
> +
> +@item secret
> +Set override of encryption secret, by default is unset.
> +
> +@item encryption
> +Set encryption type, by default is disabled.
> +Acceptable values are 128 and 256.
> +@end table
> +
>  @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;
> +
> +    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 },
> +    { "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 },
> +    { "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);
> +    if (ret < 0)
> +        return ret;

Here and everywhere: the return values of this library have nothing to
do with AVERROR codes, they cannot be returned like this.

> +

> +    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;
> +    }

It should return an error for AVIO_FLAG_READ_WRITE.

> +

> +    ret = rist_parse_address(uri, (void *)&s->peer_config);

This cast looks wrong. s->peer_config should probably be const. Same for
others.

> +    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);
> +    }
> +
> +    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)));
> +
> +    if (s->secret && (s->encryption == 128 || s->encryption == 256))
> +        s->peer_config->key_size = s->encryption;
> +
> +    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);

The timeout should be much longer.

> +    if (ret < 0)
> +        return ret;
> +
> +    if (ret > 0) {
> +        size = FFMIN(size, data_block->payload_len);
> +        memcpy(buf, data_block->payload, size);
> +        return size;
> +    }
> +

> +    return 0;

This is reached when no packet is available in the timeout, but
returning 0 here means a 0-sized packet. This is not correct.

> +}
> +
> +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);
> +    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;
> +}

As pointed, this is completely wrong. And therefore, fd should be
completely local.

> +
> +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,
Nicolas George Dec. 23, 2020, 10:43 p.m. UTC | #6
Paul B Mahol (12020-12-23):
> I nowhere see proof that this cause any bugs.

get_file_handle() returns a file descriptor for use with select() or
poll(). If you give it a random number (fd is a random number), the
application will poll it as if it were a file descriptor. At best, it
causes EBADFD. At worse the application starts polling an existing and
unrelated file descriptor, causing random behaviors.

> This is fortunately completely false statement. It works just fine.

It works fine because this part of the code is completely not tested by
any of the fftools.
Marton Balint Dec. 23, 2020, 10:48 p.m. UTC | #7
On Wed, 23 Dec 2020, Paul B Mahol wrote:

> On Wed, Dec 23, 2020 at 10:30 PM Marton Balint <cus@passwd.hu> wrote:
>
>>
>>
>>
>> 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?
>>
>
> Same thing.

Surely there is no significant difference, I just feel that it cleaner to 
use NULL as unspecified default.

[...]

>> > +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.
>>
>
> Not used, thus not really relevant.

OK, but it still confused me that stderr may be used for anything. So 
yet again I believe it is cleaner to use NULL.

[...]

>> Some error handling should be done, e.g. reject encryption sizes not
>> supported, reject encryption without secret given, etc.
>>
>
> That is already handled.

As far as I see invalid encryption sizes are not rejected but silently 
ignored.

>> > +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.
>>
>
> Are you really sure it is not needed?

Nicolas already explained the use case for this callback, so yes, I think 
it is not needed.

Regards,
Marton
diff mbox series

Patch

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
 
+@section rist
+
+Reliable Internet Streaming Transport protocol
+
+The accepted options are:
+@table @option
+@item rist_profile
+Supported values:
+@table @samp
+@item simple
+@item main
+This one is default.
+@item advanced
+@end table
+
+@item buffer_size
+Set internal RIST buffer size for receiving and sending data.
+
+@item log_level
+Set loglevel for RIST logging messages.
+
+@item secret
+Set override of encryption secret, by default is unset.
+
+@item encryption
+Set encryption type, by default is disabled.
+Acceptable values are 128 and 256.
+@end table
+
 @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;
+
+    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 },
+    { "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 },
+    { "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);
+    if (ret < 0)
+        return ret;
+
+    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);
+    }
+
+    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)));
+
+    if (s->secret && (s->encryption == 128 || s->encryption == 256))
+        s->peer_config->key_size = s->encryption;
+
+    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);
+        return size;
+    }
+
+    return 0;
+}
+
+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);
+    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;
+}
+
+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;