[FFmpeg-devel] libavformat: add librist protocol

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   | 233 ++++++++++++++++++++++++++++++++++++++++
 libavformat/protocols.c |   1 +
 5 files changed, 272 insertions(+)
 create mode 100644 libavformat/librist.c

On 12/23/2020 8:32 PM, Paul B Mahol wrote:
> +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;
> +
> +    if (s->peer_config)
> +        free((void *)s->peer_config);
> +    s->peer_config = NULL;
> +
> +    if (s->logging_settings)
> +        free((void *)s->logging_settings);

This is not a good idea. Afaik there could be more than one c library at 
play, the one linked by librist, and the one linked by libavformat.
This is why most libraries always end up adding custom free functions.

I see that rist_logging_set(), or anything in logging.h from this 
library for that matter, is undocumented, but reading the source code it 
seems it accepts your own allocated rist_logging_settings struct.
So IMO first consider asking for some doxy addition to the functions in 
order to make that officially supported, and then allocate it yourself 
in librist_open() with av_malloc(sizeof(*s->logging_settings)). 
Alternatively, they could provide a new rist_logging_free() function of 
sorts.

And same for peer_config above, except that rist_parse_address() does 
not accept your own allocated rist_peer_config struct. It only accepts 
an already initialized one from a previous call, so either that's 
changed, or a custom free() or a reset_to_defaults() function is added.

> +    s->logging_settings = NULL;
> +
> +    return 0;
> +}
> +
> +static int librist_open(URLContext *h, const char *uri, int flags)
> +{
> +    RISTContext *s = h->priv_data;
> +    int ret;
> +
> +    ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb, h, NULL, stderr);
> +    if (ret < 0)
> +        return risterr2ret(ret);

On 12/23/2020 8:53 PM, James Almer wrote:
> On 12/23/2020 8:32 PM, Paul B Mahol wrote:
>> +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;
>> +
>> +    if (s->peer_config)
>> +        free((void *)s->peer_config);
>> +    s->peer_config = NULL;
>> +
>> +    if (s->logging_settings)
>> +        free((void *)s->logging_settings);
> 
> This is not a good idea. Afaik there could be more than one c library at 
> play, the one linked by librist, and the one linked by libavformat.
> This is why most libraries always end up adding custom free functions.
> 
> I see that rist_logging_set(), or anything in logging.h from this 
> library for that matter, is undocumented, but reading the source code it 
> seems it accepts your own allocated rist_logging_settings struct.
> So IMO first consider asking for some doxy addition to the functions in 
> order to make that officially supported, and then allocate it yourself 
> in librist_open() with av_malloc(sizeof(*s->logging_settings)). 
> Alternatively, they could provide a new rist_logging_free() function of 
> sorts.

I guess you could also just keep it in heap instead of allocating it. No 
need to free it that way since it'll be part of RISTContext.

> 
> And same for peer_config above, except that rist_parse_address() does 
> not accept your own allocated rist_peer_config struct. It only accepts 
> an already initialized one from a previous call, so either that's 
> changed, or a custom free() or a reset_to_defaults() function is added.

Not so much this one, for the same reason you can't pass a freshly 
allocated one.

> 
>> +    s->logging_settings = NULL;
>> +
>> +    return 0;
>> +}
>> +
>> +static int librist_open(URLContext *h, const char *uri, int flags)
>> +{
>> +    RISTContext *s = h->priv_data;
>> +    int ret;
>> +
>> +    ret = rist_logging_set(&s->logging_settings, s->log_level, 
>> log_cb, h, NULL, stderr);
>> +    if (ret < 0)
>> +        return risterr2ret(ret);
>

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   | 233 ++++++++++++++++++++++++++++++++++++++++
>  libavformat/protocols.c |   1 +
>  5 files changed, 272 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..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..dacdd4a859
> --- /dev/null
> +++ b/libavformat/librist.c
> @@ -0,0 +1,233 @@
> +/*
> + * 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;
> +

> +    if (s->peer_config)
> +        free((void *)s->peer_config);
> +    s->peer_config = NULL;
> +
> +    if (s->logging_settings)
> +        free((void *)s->logging_settings);

Still need to get rid of these casts.

> +    s->logging_settings = NULL;
> +
> +    return 0;
> +}
> +
> +static int librist_open(URLContext *h, const char *uri, int flags)
> +{
> +    RISTContext *s = h->priv_data;
> +    int ret;
> +
> +    ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb, h, NULL, stderr);
> +    if (ret < 0)
> +        return risterr2ret(ret);
> +
> +    if (flags & AVIO_FLAG_WRITE) {
> +        ret = rist_sender_create(&s->rist_ctx, s->profile, 0, s->logging_settings);
> +    } else {
> +        ret = rist_receiver_create(&s->rist_ctx, s->profile, s->logging_settings);
> +    }
> +    if (ret < 0)
> +        goto err;
> +
> +    ret = rist_parse_address(uri, (const struct rist_peer_config **)&s->peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    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");

> +        ret = AVERROR(EINVAL);
> +        goto err;

ret will be passed to risterr2ret().

> +    }
> +
> +    if (s->secret && s->peer_config->secret[0] == 0)
> +        av_strlcpy(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)
> +        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);

Already mentioned: the 5 millisecond timeout not acceptable. Set it to 0
in non-blocking mode and as large as possible in blocking mode.

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

> +        size = FFMIN(size, data_block->payload_len);
> +        memcpy(buf, data_block->payload, size);
> +        return size;

It looks like part of the payload can be silently discarded. It cannot
be allowed.

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

Regards,

On Thu, 24 Dec 2020, Nicolas George wrote:

>> +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);
>
> Already mentioned: the 5 millisecond timeout not acceptable. Set it to 0
> in non-blocking mode and as large as possible in blocking mode.

Actually it should be POLLING_TIME as defined in libavformat/network.h for 
blocking mode if you want behaviour to be consistent with other protocols. 
The function cannot block indefinitely even in blocking mode to be able 
allow generic code in libavformat/avio.c:retry_transfer_wrapper to abort a 
blocking wait.

Other protocols are directly using poll/select to wait for data with 
POLLING_TIME timeout, but since librist does not provide a way to poll for 
data, this is the best we can do (and return EAGAIN even in blocking mode 
if the POLLING_TIME elapses, because retry_transfer_wrapper will retry in 
that case anyway)

Regards,
Marton

Marton Balint (12020-12-25):
> Actually it should be POLLING_TIME as defined in libavformat/network.h for
> blocking mode if you want behaviour to be consistent with other protocols.
> The function cannot block indefinitely even in blocking mode to be able
> allow generic code in libavformat/avio.c:retry_transfer_wrapper to abort a
> blocking wait.

You are right, I forgot about it. It should be 0 in non-blocking mode
and POLLING_TIME in blocking mode. Definitely not 5.

> Other protocols are directly using poll/select to wait for data with
> POLLING_TIME timeout, but since librist does not provide a way to poll for

Note that it is far from a perfect solution. In particular, it makes our
network protocols unsuitable for idle connections with embedded
applications, because these timeouts prevent the device from sleeping.
At some point, we will need to redesign our protocols API around an
event loop. In fact, all our I/O and threading probably need to be
merged into a single event loop and scheduler.

> data, this is the best we can do (and return EAGAIN even in blocking mode if
> the POLLING_TIME elapses, because retry_transfer_wrapper will retry in that
> case anyway)

It looks to me that librist is not ready for prime time. We could
integrate it as experimental, to help it getting there. But I do not
know if the people involved would be receptive.

Regards,

Hello Nicolas,

We already had a file descriptor signaling method on a
private branch. Here is the link:


https://code.videolan.org/rist/librist/-/commits/descriptor_method/

I assume this would work?

Is the idea to add our file handle to a larger array on a
master select loop inside ffmpeg?
After the select is triggered, is it a requirement that the
data also be read with a standard "read" call on the file
descriptor or is calling an API acceptable? In the solution
above, we use an API to retrieve the data.
Sending the data within the file descriptor context would
require serialization of the rist_data_block structure and
having it deserialized by the calling application. 

Regards,

Sergio


---------- Forwarded message ---------
Date: Mon, 28 Dec 2020, 12:24
Subject: Re: [FFmpeg-devel] [PATCH] libavformat: add
librist protocol
To: FFmpeg development discussions and patches <
ffmpeg-devel@ffmpeg.org>


Marton Balint (12020-12-25):

> Actually it should be POLLING_TIME as defined in
libavformat/network.h for

> blocking mode if you want behaviour to be consistent with
other protocols.

> The function cannot block indefinitely even in blocking
mode to be able

> allow generic code in
libavformat/avio.c:retry_transfer_wrapper to abort a

> blocking wait.



You are right, I forgot about it. It should be 0 in non-
blocking mode

and POLLING_TIME in blocking mode. Definitely not 5.



> Other protocols are directly using poll/select to wait
for data with

> POLLING_TIME timeout, but since librist does not provide
a way to poll for



Note that it is far from a perfect solution. In particular,
it makes our

network protocols unsuitable for idle connections with
embedded

applications, because these timeouts prevent the device
from sleeping.

At some point, we will need to redesign our protocols API
around an

event loop. In fact, all our I/O and threading probably
need to be

merged into a single event loop and scheduler.



> data, this is the best we can do (and return EAGAIN even
in blocking mode if

> the POLLING_TIME elapses, because retry_transfer_wrapper
will retry in that

> case anyway)



It looks to me that librist is not ready for prime time. We
could

integrate it as experimental, to help it getting there. But
I do not

know if the people involved would be receptive.



Regards,

On Mon, 28 Dec 2020, Sergio M. Ammirata, Ph.D. wrote:

> Hello Nicolas,
>
> We already had a file descriptor signaling method on a
> private branch. Here is the link:
>
>
> https://code.videolan.org/rist/librist/-/commits/descriptor_method/
>
> I assume this would work?
>
> Is the idea to add our file handle to a larger array on a
> master select loop inside ffmpeg?

Just to make it clear, this is only a plan for the replacement of 
the current polling based interrupt logic, and it probably won't happen 
anytime soon.

> After the select is triggered, is it a requirement that the
> data also be read with a standard "read" call on the file
> descriptor or is calling an API acceptable? In the solution
> above, we use an API to retrieve the data.
> Sending the data within the file descriptor context would
> require serialization of the rist_data_block structure and
> having it deserialized by the calling application.

I'd say it is too early to say which method will fit ffmpeg's approach 
best, on the other hand ffmpeg will have to support many protocols with 
different polling capabilities, so I'd expect ffmpeg should be able to 
adapt to both.

And maybe my comment made the impression that fd based polling is needed 
for proper ffmpeg support, but as far as I see it that is not really the 
case, at least not for the current ffmpeg code. We are just used to having 
a poll() call and a read() call, because the read() call does not usually 
provide timeout capability, so we always had to use poll() first. But 
in librist the read() call already have a timeout parameter, so we can 
simply use that instead of poll()+read().

Regards,
Marton

>
> Regards,
>
> Sergio
>
>
> ---------- Forwarded message ---------
> Date: Mon, 28 Dec 2020, 12:24
> Subject: Re: [FFmpeg-devel] [PATCH] libavformat: add
> librist protocol
> To: FFmpeg development discussions and patches <
> ffmpeg-devel@ffmpeg.org>
>
>
> Marton Balint (12020-12-25):
>
>> Actually it should be POLLING_TIME as defined in
> libavformat/network.h for
>
>> blocking mode if you want behaviour to be consistent with
> other protocols.
>
>> The function cannot block indefinitely even in blocking
> mode to be able
>
>> allow generic code in
> libavformat/avio.c:retry_transfer_wrapper to abort a
>
>> blocking wait.
>
>
>
> You are right, I forgot about it. It should be 0 in non-
> blocking mode
>
> and POLLING_TIME in blocking mode. Definitely not 5.
>
>
>
>> Other protocols are directly using poll/select to wait
> for data with
>
>> POLLING_TIME timeout, but since librist does not provide
> a way to poll for
>
>
>
> Note that it is far from a perfect solution. In particular,
> it makes our
>
> network protocols unsuitable for idle connections with
> embedded
>
> applications, because these timeouts prevent the device
> from sleeping.
>
> At some point, we will need to redesign our protocols API
> around an
>
> event loop. In fact, all our I/O and threading probably
> need to be
>
> merged into a single event loop and scheduler.
>
>
>
>> data, this is the best we can do (and return EAGAIN even
> in blocking mode if
>
>> the POLLING_TIME elapses, because retry_transfer_wrapper
> will retry in that
>
>> case anyway)
>
>
>
> It looks to me that librist is not ready for prime time. We
> could
>
> integrate it as experimental, to help it getting there. But
> I do not
>
> know if the people involved would be receptive.
>
>
>
> Regards,
>
>
>
>
> _______________________________________________
> 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".

Sergio M. Ammirata, Ph.D. (12020-12-28):
> We already had a file descriptor signaling method on a
> private branch. Here is the link:
> 
> 
> https://code.videolan.org/rist/librist/-/commits/descriptor_method/
> 
> I assume this would work?
>
> Is the idea to add our file handle to a larger array on a
> master select loop inside ffmpeg?
> After the select is triggered, is it a requirement that the
> data also be read with a standard "read" call on the file
> descriptor or is calling an API acceptable? In the solution
> above, we use an API to retrieve the data.

IIUC, it adds a function where the application gives one end of a
self-pipe (https://cr.yp.to/docs/selfpipe.html) to get notification when
something is ready. It should work nicely. Draining the pipe explicitly
or through an API call, both are fine, as long as the proper way is
documented.

You could also just give access to the actual socket file descriptor. It
is what Xlib does, for example
(https://linux.die.net/man/3/connectionnumber), it gives the socket to
the X11 server to the application, the application poll()s it, and when
it is readable, it can call XPending() / XNextEvent().

> Sending the data within the file descriptor context would
> require serialization of the rist_data_block structure and
> having it deserialized by the calling application. 

Of course, no need for that.

As Marton said, FFmpeg is currently underdeveloped in the async
protocols department. I want to change that, but that is not for today.

But FFmpeg is not the only library around here. To make sure this new
library's API is really usable by complex application, I think good
advice would be to make sure it works with a few common event loops. I
think probably GLib's main event loop
(https://developer.gnome.org/glib/2.66/glib-The-Main-Event-Loop.html),
because it is at the core of most GNOME applications. And possibly libev
(http://software.schmorp.de/pkg/libev.html), because it is lightweight
and simple yet efficient.

Maybe the best idea would be to have doc/examples/gtk.c and
doc/examples/libev.c to show how to properly use the API in an
asynchronous application.

Anyway, thanks for reacting so efficiently.

<rant>

What people do not realize is that threads do not make network
programming simpler. We often see advice, about network programming,
"just use threads", but it is a misconception.

Threads are very useful for performance, but when it comes to network
programming, they do not help (except for DNS). I must be more accurate:
POSIX threads do not help with Unix-style sockets.

The issue is that POSIX threads and Unix sockets are completely separate
APIs, without any kind of integration. When a thread is blocked on a
socket operation, it does not communicate with other threads. To
implement things as basic as a timeout with threads, it requires at
least another thread, and some kind of asynchronous cancellation
operation, which is tricky, both for portability and for concurrency
problems.

Really, a loop around poll() or equivalent is the only working way to
use Unix sockets. When we try to use threads with sockets, we end up
having a poll() loop in each thread, and then we might as well have a
single loop without threads. Also, event loops scale much better than
threads.

I write this under the assumption we want to do things properly, i.e.
have an application that can sleep when there is nothing on the network.
So many applications have a periodic wake-up just to check for timeouts
and other interrupts; FFmpeg have one ten times per second. For embedded
applications, it is terrible, because it prevents the device from going
to deep sleep, and drains the battery.

</rant>

Regards,

Understood ... thanks for the advice.

Is there else blocking the integration of the library based
module into ffmpeg? We would prefer not to have it in
"experimental mode".

Regards,

Sergio

On Tue, 2020-12-29 at 23:03 +0100, Nicolas George wrote:
> Sergio M. Ammirata, Ph.D. (12020-12-28):
> 
> We already had a file descriptor signaling method on a
> private branch. Here is the link:
> 
> 
> https://code.videolan.org/rist/librist/-/commits/descriptor_method/
> 
> I assume this would work?
> 
> Is the idea to add our file handle to a larger array on a
> master select loop inside ffmpeg?
> After the select is triggered, is it a requirement that the
> data also be read with a standard "read" call on the file
> descriptor or is calling an API acceptable? In the solution
> above, we use an API to retrieve the data.
> 
> IIUC, it adds a function where the application gives one end of a
> self-pipe (https://cr.yp.to/docs/selfpipe.html) to get notification when
> something is ready. It should work nicely. Draining the pipe explicitly
> or through an API call, both are fine, as long as the proper way is
> documented.
> 
> You could also just give access to the actual socket file descriptor. It
> is what Xlib does, for example
> (https://linux.die.net/man/3/connectionnumber), it gives the socket to
> the X11 server to the application, the application poll()s it, and when
> it is readable, it can call XPending() / XNextEvent().
> 
> 
> Sending the data within the file descriptor context would
> require serialization of the rist_data_block structure and
> having it deserialized by the calling application. 
> 
> Of course, no need for that.
> 
> As Marton said, FFmpeg is currently underdeveloped in the async
> protocols department. I want to change that, but that is not for today.
> 
> But FFmpeg is not the only library around here. To make sure this new
> library's API is really usable by complex application, I think good
> advice would be to make sure it works with a few common event loops. I
> think probably GLib's main event loop
> (https://developer.gnome.org/glib/2.66/glib-The-Main-Event-Loop.html),
> because it is at the core of most GNOME applications. And possibly libev
> (http://software.schmorp.de/pkg/libev.html), because it is lightweight
> and simple yet efficient.
> 
> Maybe the best idea would be to have doc/examples/gtk.c and
> doc/examples/libev.c to show how to properly use the API in an
> asynchronous application.
> 
> Anyway, thanks for reacting so efficiently.
> 
> <rant>
> 
> What people do not realize is that threads do not make network
> programming simpler. We often see advice, about network programming,
> "just use threads", but it is a misconception.
> 
> Threads are very useful for performance, but when it comes to network
> programming, they do not help (except for DNS). I must be more accurate:
> POSIX threads do not help with Unix-style sockets.
> 
> The issue is that POSIX threads and Unix sockets are completely separate
> APIs, without any kind of integration. When a thread is blocked on a
> socket operation, it does not communicate with other threads. To
> implement things as basic as a timeout with threads, it requires at
> least another thread, and some kind of asynchronous cancellation
> operation, which is tricky, both for portability and for concurrency
> problems.
> 
> Really, a loop around poll() or equivalent is the only working way to
> use Unix sockets. When we try to use threads with sockets, we end up
> having a poll() loop in each thread, and then we might as well have a
> single loop without threads. Also, event loops scale much better than
> threads.
> 
> I write this under the assumption we want to do things properly, i.e.
> have an application that can sleep when there is nothing on the network.
> So many applications have a periodic wake-up just to check for timeouts
> and other interrupts; FFmpeg have one ten times per second. For embedded
> applications, it is terrible, because it prevents the device from going
> to deep sleep, and drains the battery.
> 
> </rant>
> 
> Regards,
> 
>

Sergio M. Ammirata, Ph.D. (12020-12-30):
> Is there else blocking the integration of the library based
> module into ffmpeg?

The comments made by James and me need to be addressed.

> We would prefer not to have it in "experimental mode".

For that, you need to be really really sure that the API is stable. In
particular, you will not be able to change the layout of the various
_config structs. As a more concrete example, this:

commit 29641ac74f89d9a8e6a187a9b218678f1ada27ab
Author: Sergio Ammirata <sergio@ammirata.net>
Date:   2020-12-25 14:15:22 -0500

    Change the new free functions to accept pointer of pointer so they
    can be nulled

cannot happen if librist is used by something serious.

I would recommend not to rush things. Designing a good library API takes
time. If you freeze it too early, you will probably get stuck with
things you will regret later. Having it as "experimental" in FFmpeg is
just a way of having more time to make things really good before
freezing.

Regards,