diff mbox series

[FFmpeg-devel] libavformat: add librist protocol

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

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. 24, 2020, 11:08 a.m. UTC
This work is sponsored by Open Broadcast Systems.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 configure               |   5 +
 doc/protocols.texi      |  32 ++++++
 libavformat/Makefile    |   1 +
 libavformat/librist.c   | 242 ++++++++++++++++++++++++++++++++++++++++
 libavformat/protocols.c |   1 +
 5 files changed, 281 insertions(+)
 create mode 100644 libavformat/librist.c

Comments

Nicolas George Dec. 24, 2020, 11:32 a.m. UTC | #1
Paul B Mahol (12020-12-24):
> This work is sponsored by Open Broadcast Systems.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  configure               |   5 +
>  doc/protocols.texi      |  32 ++++++
>  libavformat/Makefile    |   1 +
>  libavformat/librist.c   | 242 ++++++++++++++++++++++++++++++++++++++++
>  libavformat/protocols.c |   1 +
>  5 files changed, 281 insertions(+)
>  create mode 100644 libavformat/librist.c

NAK. AVIO_FLAG_READ_WRITE not handled, incorrect timeout, payload
possibly silently discarded. All already pointed.
Paul B Mahol Dec. 24, 2020, 12:59 p.m. UTC | #2
On Thu, Dec 24, 2020 at 12:32 PM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12020-12-24):
> > This work is sponsored by Open Broadcast Systems.
> >
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  configure               |   5 +
> >  doc/protocols.texi      |  32 ++++++
> >  libavformat/Makefile    |   1 +
> >  libavformat/librist.c   | 242 ++++++++++++++++++++++++++++++++++++++++
> >  libavformat/protocols.c |   1 +
> >  5 files changed, 281 insertions(+)
> >  create mode 100644 libavformat/librist.c
>
> NAK. AVIO_FLAG_READ_WRITE not handled, incorrect timeout, payload
> possibly silently discarded. All already pointed.
>

I have zero respect about your disrespectful and extremely rude reviews.

Why such flag should be handled? It does not make sense.
timeout is correct.
payload is never silently discarded, it is make sure that it is never
bigger than max packet size, its just your imagination.



>
> --
>   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. 24, 2020, 1:01 p.m. UTC | #3
Paul B Mahol (12020-12-24):
> I have zero respect about your disrespectful and extremely rude reviews.
> 
> Why such flag should be handled? It does not make sense.
> timeout is correct.
> payload is never silently discarded, it is make sure that it is never
> bigger than max packet size, its just your imagination.

Patch still blocked until you're ready to discuss this cordially.
James Almer Dec. 24, 2020, 1:10 p.m. UTC | #4
On 12/24/2020 8:08 AM, Paul B Mahol wrote:
> +typedef struct RISTContext {
> +    const AVClass *class;
> +
> +    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;

[...]

> +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->rist_ctx, s->profile, 0, logging_settings);
> +    } else {
> +        ret = rist_receiver_create(&s->rist_ctx, s->profile, logging_settings);
> +    }
> +    if (ret < 0)
> +        goto err;
> +
> +    ret = rist_parse_address(uri, (const struct rist_peer_config **)&peer_config);
> +    if (ret < 0)
> +        goto err;

Look at 
https://code.videolan.org/rist/librist/-/blob/4fd217662cee398f4706c815958a1efe48b2093f/src/rist.c#L637
rist_parse_address() does not fill peer_config with default values if it 
doesn't allocate it itself. It assumes it was allocated during a 
previous call and not by the caller.

Unless the library provides a defaults()/reset() kind of function, it 
doesn't look like you can really use your own rist_peer_config struct.
Can you talk with the devs about it? Or even just submit a PR yourself. 
It should be very trivial to implement.
Paul B Mahol Dec. 24, 2020, 1:16 p.m. UTC | #5
Look guys I will apply this patch soon.

Your reviews are very bad!

Why your reviews are extremely bad?

You really have no skills or experience to say anything useful in this
patch.

Simple proof is that I posted one variant where you objected about
allocation and than
your are not happy how I resolved it. Instead you object how librist is
implemented.

[auto-censored entries]

You are just very rude and disrespectfully developers and community overall.

Good ridance!


On Thu, Dec 24, 2020 at 2:10 PM James Almer <jamrial@gmail.com> wrote:

> On 12/24/2020 8:08 AM, Paul B Mahol wrote:
> > +typedef struct RISTContext {
> > +    const AVClass *class;
> > +
> > +    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;
>
> [...]
>
> > +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->rist_ctx, s->profile, 0,
> logging_settings);
> > +    } else {
> > +        ret = rist_receiver_create(&s->rist_ctx, s->profile,
> logging_settings);
> > +    }
> > +    if (ret < 0)
> > +        goto err;
> > +
> > +    ret = rist_parse_address(uri, (const struct rist_peer_config
> **)&peer_config);
> > +    if (ret < 0)
> > +        goto err;
>
> Look at
>
> https://code.videolan.org/rist/librist/-/blob/4fd217662cee398f4706c815958a1efe48b2093f/src/rist.c#L637
> rist_parse_address() does not fill peer_config with default values if it
> doesn't allocate it itself. It assumes it was allocated during a
> previous call and not by the caller.
>
> Unless the library provides a defaults()/reset() kind of function, it
> doesn't look like you can really use your own rist_peer_config struct.
> Can you talk with the devs about it? Or even just submit a PR yourself.
> It should be very trivial to implement.
> _______________________________________________
> 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".
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..aa682f1586 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -673,6 +673,38 @@  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 retransmission of data.
+
+@item pkt_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..9aa9e02a58
--- /dev/null
+++ b/libavformat/librist.c
@@ -0,0 +1,242 @@ 
+/*
+ * 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 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=9968},                  1, 9968,    .flags = D|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=NULL},                  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 risterr2ret(int err)
+{
+    switch (err) {
+    case RIST_ERR_MALLOC:
+        return AVERROR(ENOMEM);
+    default:
+        return AVERROR_EXTERNAL;
+    }
+}
+
+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_close(URLContext *h)
+{
+    RISTContext *s = h->priv_data;
+    int ret = 0;
+
+    s->peer = NULL;
+
+    if (s->rist_ctx)
+        ret = rist_destroy(s->rist_ctx);
+    if (ret < 0)
+        return risterr2ret(ret);
+    s->rist_ctx = NULL;
+
+    return 0;
+}
+
+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->rist_ctx, s->profile, 0, logging_settings);
+    } else {
+        ret = rist_receiver_create(&s->rist_ctx, s->profile, logging_settings);
+    }
+    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)));
+
+    if (s->secret && (s->encryption == 128 || s->encryption == 256))
+        peer_config->key_size = s->encryption;
+
+    peer_config->version = RIST_PEER_CONFIG_VERSION;
+    peer_config->virt_dst_port = RIST_DEFAULT_VIRT_DST_PORT;
+    peer_config->recovery_mode = RIST_DEFAULT_RECOVERY_MODE;
+    peer_config->recovery_maxbitrate = RIST_DEFAULT_RECOVERY_MAXBITRATE;
+    peer_config->recovery_maxbitrate_return = RIST_DEFAULT_RECOVERY_MAXBITRATE_RETURN;
+    peer_config->recovery_length_min = RIST_DEFAULT_RECOVERY_LENGHT_MIN;
+    peer_config->recovery_length_max = RIST_DEFAULT_RECOVERY_LENGHT_MAX;
+    peer_config->recovery_reorder_buffer = RIST_DEFAULT_RECOVERY_REORDER_BUFFER;
+    peer_config->recovery_rtt_min = RIST_DEFAULT_RECOVERY_RTT_MIN;
+    peer_config->recovery_rtt_max = RIST_DEFAULT_RECOVERY_RTT_MAX;
+    peer_config->congestion_control_mode = RIST_DEFAULT_CONGESTION_CONTROL_MODE;
+    peer_config->min_retries = RIST_DEFAULT_MIN_RETRIES;
+    peer_config->max_retries = RIST_DEFAULT_MAX_RETRIES;
+
+    if (s->buffer_size) {
+        peer_config->recovery_length_min = s->buffer_size;
+        peer_config->recovery_length_max = s->buffer_size;
+    }
+
+    ret = rist_peer_create(s->rist_ctx, &s->peer, &s->peer_config);
+    if (ret < 0)
+        goto err;
+
+    ret = rist_start(s->rist_ctx);
+    if (ret < 0)
+        goto err;
+
+    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->rist_ctx, &data_block, 5);
+    if (ret < 0)
+        return risterr2ret(ret);
+
+    if (ret > 0 && data_block->payload) {
+        size = FFMIN(size, data_block->payload_len);
+        memcpy(buf, data_block->payload, size);
+        return size;
+    }
+
+    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 = size;
+    ret = rist_sender_data_write(s->rist_ctx, &data_block);
+    if (ret < 0)
+        return risterr2ret(ret);
+    else if (ret)
+        return ret;
+    else // remove this part when new librist release appears
+        return size;
+}
+
+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,
+    .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;