On Wed, Dec 23, 2020 at 10:30 PM Marton Balint <c...@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 <one...@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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
