diff mbox series

[FFmpeg-devel] libavformat: add librist protocol

Message ID 20210126205145.20448-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 Jan. 26, 2021, 8:51 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      |  32 +++++
 libavformat/Makefile    |   1 +
 libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
 libavformat/protocols.c |   1 +
 5 files changed, 290 insertions(+)
 create mode 100644 libavformat/librist.c

Comments

Paul B Mahol Jan. 29, 2021, 11:31 a.m. UTC | #1
Will apply soon.
Marton Balint Jan. 30, 2021, 11:57 p.m. UTC | #2
On Tue, 26 Jan 2021, 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      |  32 +++++
> libavformat/Makefile    |   1 +
> libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
> libavformat/protocols.c |   1 +
> 5 files changed, 290 insertions(+)
> create mode 100644 libavformat/librist.c
>

[...]

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

Can you avoid these on the stack? If librist adds new members to them, 
this might break, and there is now API for freeing them properly.

Thanks,
Marton
Marton Balint Jan. 31, 2021, midnight UTC | #3
On Sun, 31 Jan 2021, Marton Balint wrote:

>
>
> On Tue, 26 Jan 2021, 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      |  32 +++++
>> libavformat/Makefile    |   1 +
>> libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
>> libavformat/protocols.c |   1 +
>> 5 files changed, 290 insertions(+)
>> create mode 100644 libavformat/librist.c
>>
>
> [...]
>
>> +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;
>
> Can you avoid these on the stack? If librist adds new members to them, 
> this might break, and there is now API for freeing them properly.

Obviously what I meant is avoiding librist structs directly in our data 
structures.

Thanks,
Marton
James Almer Jan. 31, 2021, 12:29 a.m. UTC | #4
On 1/30/2021 8:57 PM, Marton Balint wrote:
> 
> 
> On Tue, 26 Jan 2021, 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      |  32 +++++
>> libavformat/Makefile    |   1 +
>> libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
>> libavformat/protocols.c |   1 +
>> 5 files changed, 290 insertions(+)
>> create mode 100644 libavformat/librist.c
>>
> 
> [...]
> 
>> +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;
> 
> Can you avoid these on the stack? If librist adds new members to them, 
> this might break, and there is now API for freeing them properly.

Not against this suggestion, but it should be ok to have them in stack.
For rist_peer_config, he's calling rist_peer_config_defaults_set() first 
thing on the struct, so any new field will always be properly 
initialized by the library. And rist_logging_settings is zeroed by 
rist_logging_set() in the scenario where it allocs it, so it's the same 
as zeroing it ourselves as part of the RISTContext allocation.

Any changes to either struct should go alongside a soname bump in 
librist, too.

> 
> Thanks,
> 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".
Marton Balint Jan. 31, 2021, 11 a.m. UTC | #5
On Tue, 26 Jan 2021, 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      |  32 +++++
> libavformat/Makefile    |   1 +
> libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
> libavformat/protocols.c |   1 +
> 5 files changed, 290 insertions(+)
> create mode 100644 libavformat/librist.c


[...]


> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -690,6 +690,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.

You should mention the default here. And by the way, why have you selected
9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
makes more sense no?

What you can also do is to make -1 the default, and then make it 1316 for
write mode and 9968 otherwise, if really there is a common use for bigger
packets.

[..]

> +static int librist_close(URLContext *h)
> +{
> +    RISTContext *s = h->priv_data;
> +    int ret = 0;
> +
> +    s->read_peer = NULL;
> +    s->write_peer = NULL;
> +
> +    if (s->write_ctx)
> +        ret = rist_destroy(s->read_ctx);
> +    if (s->read_ctx)
> +        ret = rist_destroy(s->read_ctx);
> +    s->read_ctx = NULL;
> +    s->write_ctx = NULL;
> +
> +    return risterr2ret(ret);

This returns an error even if there is no rist error.
Maybe you should simply return 0, and avoid assigning rets.

> +}
> +
> +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->write_ctx, s->profile, 0, logging_settings);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (flags & AVIO_FLAG_READ)
> +        ret = rist_receiver_create(&s->read_ctx, s->profile, logging_settings);
> +    if (ret < 0)
> +        goto err;

You should move these context creating downward to other read/write mode
checks.

> +
> +    ret = rist_peer_config_defaults_set(peer_config);
> +    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)));

Simply av_strlcpy(peer_config->secret, s->secret, RIST_MAX_STRING_SHORT).

> +
> +    if (s->secret && (s->encryption == 128 || s->encryption == 256))
> +        peer_config->key_size = s->encryption;
> +
> +    if (s->buffer_size) {
> +        peer_config->recovery_length_min = s->buffer_size;
> +        peer_config->recovery_length_max = s->buffer_size;
> +    }
> +
> +    if (flags & AVIO_FLAG_READ)
> +        ret = rist_peer_create(s->read_ctx, &s->read_peer, &s->peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (flags & AVIO_FLAG_WRITE)
> +        ret = rist_peer_create(s->write_ctx, &s->write_peer, &s->peer_config);
> +    if (ret < 0)
> +        goto err;
> +
> +    if (flags & AVIO_FLAG_READ)
> +        ret = rist_start(s->read_ctx);
> +    if (ret < 0)
> +        goto err;
> +    if (flags & AVIO_FLAG_WRITE)
> +        ret = rist_start(s->write_ctx);
> +    if (ret < 0)
> +        goto err;

Use a single check:

if (flags & AVIO_FLAG_READ) {
    rist_receiver_create()
    rist_peer_create()
    rist_start()
}
if (flags & AVIO_FLAG_WRITE) {
    rist_sender_create()
    rist_peer_create()
    rist_start()
}

Also make sure you are not leaking s->read_ctx/s->write_ctx on errors.

And does it really make sense to use READ and WRITE at the same time? Can
it work? Nicolas's original suggestion was to simply return error if both
flags (AVIO_FLAG_READWRITE) are set.

> +
> +    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->read_ctx, &data_block, POLLING_TIME);
> +    if (ret < 0)
> +        return risterr2ret(ret);
> +
> +    if (ret > 0 && data_block->payload) {

Is it possible that data_block->payload is NULL? If not, then either 
remove the check or make it an assert. If yes, then you are leaking 
data_block if it is indeed NULL.

> +        if (data_block->payload_len > size)
> +            av_log(h, AV_LOG_WARNING, "Part of datagram lost due to insufficient buffer size\n");
> +        size = FFMIN(size, data_block->payload_len);
> +        memcpy(buf, data_block->payload, size);
> +        rist_receiver_data_block_free(&data_block);
> +        return size;
> +    }
> +
> +    return AVERROR(EAGAIN);
> +}
> +

Thanks,
Marton
Paul B Mahol Jan. 31, 2021, 11:06 a.m. UTC | #6
On Sun, Jan 31, 2021 at 12:00 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Tue, 26 Jan 2021, 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      |  32 +++++
> > libavformat/Makefile    |   1 +
> > libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
> > libavformat/protocols.c |   1 +
> > 5 files changed, 290 insertions(+)
> > create mode 100644 libavformat/librist.c
>
>
> [...]
>
>
> > --- a/doc/protocols.texi
> > +++ b/doc/protocols.texi
> > @@ -690,6 +690,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.
>
> You should mention the default here. And by the way, why have you selected
> 9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
> makes more sense no?
>
>
NO


> What you can also do is to make -1 the default, and then make it 1316 for
> write mode and 9968 otherwise, if really there is a common use for bigger
> packets.
>
>
NO


> [..]
>
> > +static int librist_close(URLContext *h)
> > +{
> > +    RISTContext *s = h->priv_data;
> > +    int ret = 0;
> > +
> > +    s->read_peer = NULL;
> > +    s->write_peer = NULL;
> > +
> > +    if (s->write_ctx)
> > +        ret = rist_destroy(s->read_ctx);
> > +    if (s->read_ctx)
> > +        ret = rist_destroy(s->read_ctx);
> > +    s->read_ctx = NULL;
> > +    s->write_ctx = NULL;
> > +
> > +    return risterr2ret(ret);
>
> This returns an error even if there is no rist error.
> Maybe you should simply return 0, and avoid assigning rets.
>
> > +}
> > +
> > +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->write_ctx, s->profile, 0,
> logging_settings);
> > +    if (ret < 0)
> > +        goto err;
> > +
> > +    if (flags & AVIO_FLAG_READ)
> > +        ret = rist_receiver_create(&s->read_ctx, s->profile,
> logging_settings);
> > +    if (ret < 0)
> > +        goto err;
>
> You should move these context creating downward to other read/write mode
> checks.
>


NO


>
> > +
> > +    ret = rist_peer_config_defaults_set(peer_config);
> > +    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)));
>
> Simply av_strlcpy(peer_config->secret, s->secret, RIST_MAX_STRING_SHORT).
>

Really? Are you sure?


>
> > +
> > +    if (s->secret && (s->encryption == 128 || s->encryption == 256))
> > +        peer_config->key_size = s->encryption;
> > +
> > +    if (s->buffer_size) {
> > +        peer_config->recovery_length_min = s->buffer_size;
> > +        peer_config->recovery_length_max = s->buffer_size;
> > +    }
> > +
> > +    if (flags & AVIO_FLAG_READ)
> > +        ret = rist_peer_create(s->read_ctx, &s->read_peer,
> &s->peer_config);
> > +    if (ret < 0)
> > +        goto err;
> > +
> > +    if (flags & AVIO_FLAG_WRITE)
> > +        ret = rist_peer_create(s->write_ctx, &s->write_peer,
> &s->peer_config);
> > +    if (ret < 0)
> > +        goto err;
> > +
> > +    if (flags & AVIO_FLAG_READ)
> > +        ret = rist_start(s->read_ctx);
> > +    if (ret < 0)
> > +        goto err;
> > +    if (flags & AVIO_FLAG_WRITE)
> > +        ret = rist_start(s->write_ctx);
> > +    if (ret < 0)
> > +        goto err;
>
> Use a single check:
>
> if (flags & AVIO_FLAG_READ) {
>     rist_receiver_create()
>     rist_peer_create()
>     rist_start()
> }
> if (flags & AVIO_FLAG_WRITE) {
>     rist_sender_create()
>     rist_peer_create()
>     rist_start()
> }
>
> Also make sure you are not leaking s->read_ctx/s->write_ctx on errors.
>
> And does it really make sense to use READ and WRITE at the same time? Can
> it work? Nicolas's original suggestion was to simply return error if both
> flags (AVIO_FLAG_READWRITE) are set.
>

I think it can work.


>
> > +
> > +    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->read_ctx, &data_block,
> POLLING_TIME);
> > +    if (ret < 0)
> > +        return risterr2ret(ret);
> > +
> > +    if (ret > 0 && data_block->payload) {
>
> Is it possible that data_block->payload is NULL? If not, then either
> remove the check or make it an assert. If yes, then you are leaking
> data_block if it is indeed NULL.
>
> > +        if (data_block->payload_len > size)
> > +            av_log(h, AV_LOG_WARNING, "Part of datagram lost due to
> insufficient buffer size\n");
> > +        size = FFMIN(size, data_block->payload_len);
> > +        memcpy(buf, data_block->payload, size);
> > +        rist_receiver_data_block_free(&data_block);
> > +        return size;
> > +    }
> > +
> > +    return AVERROR(EAGAIN);
> > +}
> > +
>
> Thanks,
> 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".
Marton Balint Jan. 31, 2021, 11:29 a.m. UTC | #7
On Sun, 31 Jan 2021, Paul B Mahol wrote:

> On Sun, Jan 31, 2021 at 12:00 PM Marton Balint <cus@passwd.hu> wrote:
>
>>
>>
>> On Tue, 26 Jan 2021, 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      |  32 +++++
>> > libavformat/Makefile    |   1 +
>> > libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
>> > libavformat/protocols.c |   1 +
>> > 5 files changed, 290 insertions(+)
>> > create mode 100644 libavformat/librist.c
>>
>>
>> [...]
>>
>>
>> > --- a/doc/protocols.texi
>> > +++ b/doc/protocols.texi
>> > @@ -690,6 +690,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.
>>
>> You should mention the default here. And by the way, why have you selected
>> 9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
>> makes more sense no?
>>
>>
> NO

Please explain why, and give a reason for 9968 then.

>> > +    if (s->secret && peer_config->secret[0] == 0)
>> > +        av_strlcpy(peer_config->secret, s->secret,
>> FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret)));
>>
>> Simply av_strlcpy(peer_config->secret, s->secret, RIST_MAX_STRING_SHORT).
>>
>
> Really? Are you sure?

It looks to me that is exactly what is needed here.

Thanks,
Marton
Paul B Mahol Jan. 31, 2021, 11:33 a.m. UTC | #8
On Sun, Jan 31, 2021 at 12:29 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Sun, 31 Jan 2021, Paul B Mahol wrote:
>
> > On Sun, Jan 31, 2021 at 12:00 PM Marton Balint <cus@passwd.hu> wrote:
> >
> >>
> >>
> >> On Tue, 26 Jan 2021, 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      |  32 +++++
> >> > libavformat/Makefile    |   1 +
> >> > libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
> >> > libavformat/protocols.c |   1 +
> >> > 5 files changed, 290 insertions(+)
> >> > create mode 100644 libavformat/librist.c
> >>
> >>
> >> [...]
> >>
> >>
> >> > --- a/doc/protocols.texi
> >> > +++ b/doc/protocols.texi
> >> > @@ -690,6 +690,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.
> >>
> >> You should mention the default here. And by the way, why have you
> selected
> >> 9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
> >> makes more sense no?
> >>
> >>
> > NO
>
> Please explain why, and give a reason for 9968 then.
>

because that is max allowed value that works, I really wonder why it should
be lowered.


>
> >> > +    if (s->secret && peer_config->secret[0] == 0)
> >> > +        av_strlcpy(peer_config->secret, s->secret,
> >> FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret)));
> >>
> >> Simply av_strlcpy(peer_config->secret, s->secret,
> RIST_MAX_STRING_SHORT).
> >>
> >
> > Really? Are you sure?
>
> It looks to me that is exactly what is needed here.
>

ok


> Thanks,
> 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".
Marton Balint Jan. 31, 2021, 11:46 a.m. UTC | #9
On Sun, 31 Jan 2021, Paul B Mahol wrote:

> On Sun, Jan 31, 2021 at 12:29 PM Marton Balint <cus@passwd.hu> wrote:
>
>>
>>
>> On Sun, 31 Jan 2021, Paul B Mahol wrote:
>>
>> > On Sun, Jan 31, 2021 at 12:00 PM Marton Balint <cus@passwd.hu> wrote:
>> >
>> >>
>> >>
>> >> On Tue, 26 Jan 2021, 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      |  32 +++++
>> >> > libavformat/Makefile    |   1 +
>> >> > libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
>> >> > libavformat/protocols.c |   1 +
>> >> > 5 files changed, 290 insertions(+)
>> >> > create mode 100644 libavformat/librist.c
>> >>
>> >>
>> >> [...]
>> >>
>> >>
>> >> > --- a/doc/protocols.texi
>> >> > +++ b/doc/protocols.texi
>> >> > @@ -690,6 +690,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.
>> >>
>> >> You should mention the default here. And by the way, why have you
>> selected
>> >> 9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
>> >> makes more sense no?
>> >>
>> >>
>> > NO
>>
>> Please explain why, and give a reason for 9968 then.
>>
>
> because that is max allowed value that works,

And is this some librist or protocol limitation?

> I really wonder why it should  be lowered.

I suggested to lower it for write only. 9968 is not divisible by 188, so 
libavformat will split packets inside mpegts packet, instead of at mpegts 
packet boundaries, which will surely cause issues on the some receivers.

Regards,
Marton
Sergio M. Ammirata, Ph.D. Jan. 31, 2021, 9:33 p.m. UTC | #10
librist has an internal buffer limit of 10000 bytes (the
protocol has none).

For writing, the data you give librist, will go out "as is"
to the network with some added protocol overhead bytes (28
bytes without encryption enabled and 36 bytes with
encryption enabled).

To avoid your data being fragmented because of network MTU,
you should really limit the write size to what you want
your IP packets to be. For mpegts, for example, 1316 is a
good number to keep the overall packet size below the
typical internet MTU size of 1500 (1400 to be safe).

I suggest this "max" write size be a configurable setting
in ffmpeg so that a higher number can be used if you are
running it where the network supports jumbo packets.

For reading, you can keep the 9964 number always.

Most implementations of RIST "simple and main profiles"
have been focused on mpegts payloads. However, librist has
a more generic approach and will replicate the payload
passed to it, byte by byte to the other side. Within the
protocol itself, librist pretends to be sending an mpegts
payload and will inter-operate well with other
implementations when mpegts is the payload sent. When you
send other payload types, the other side needs to have
librist and/or needs to know how to process such payloads. 
For example, ffmpeg to ffmpeg with librist on both sides
will be able to transfer any payload/protocol that can be
auto-detected on the receiver end. 

Regards,

Sergio


On Sun, 2021-01-31 at 12:46 +0100, Marton Balint wrote:
> 
> On Sun, 31 Jan 2021, Paul B Mahol wrote:
> 
> On Sun, Jan 31, 2021 at 12:29 PM Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
> On Sun, 31 Jan 2021, Paul B Mahol wrote:
> 
> On Sun, Jan 31, 2021 at 12:00 PM Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
> On Tue, 26 Jan 2021, 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      |  32 +++++
> libavformat/Makefile    |   1 +
> libavformat/librist.c   | 251 ++++++++++++++++++++++++++++++++++++++++
> libavformat/protocols.c |   1 +
> 5 files changed, 290 insertions(+)
> create mode 100644 libavformat/librist.c
> 
> 
> [...]
> 
> 
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -690,6 +690,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.
> 
> You should mention the default here. And by the way, why have you
> selected
> 9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
> makes more sense no?
> 
> 
> NO
> 
> Please explain why, and give a reason for 9968 then.
> 
> 
> because that is max allowed value that works,
> 
> And is this some librist or protocol limitation?
> 
> I really wonder why it should  be lowered.
> 
> I suggested to lower it for write only. 9968 is not divisible by 188, so 
> libavformat will split packets inside mpegts packet, instead of at mpegts 
> packet boundaries, which will surely cause issues on the some receivers.
> 
> 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".
Rodney Baker Feb. 1, 2021, 2:16 p.m. UTC | #11
On Monday, 1 February 2021 8:03:28 ACDT Sergio M. Ammirata, Ph.D. wrote:
> librist has an internal buffer limit of 10000 bytes (the
> protocol has none).
> 
> For writing, the data you give librist, will go out "as is"
> to the network with some added protocol overhead bytes (28
> bytes without encryption enabled and 36 bytes with
> encryption enabled).
> 
> To avoid your data being fragmented because of network MTU,
> you should really limit the write size to what you want
> your IP packets to be. For mpegts, for example, 1316 is a
> good number to keep the overall packet size below the
> typical internet MTU size of 1500 (1400 to be safe).
> 
> I suggest this "max" write size be a configurable setting
> in ffmpeg so that a higher number can be used if you are
> running it where the network supports jumbo packets.

If I can chime in as an experienced network designer/administrator, I concur 
with comments by Sergio and Marton. This should definitely be a configurable 
parameter for writing, with a default value of 1500 or less, and a maximum 
allowed value of 9216 (the highest jumbo frame MTU guaranteed to be supported 
by most modern networking equipment). 

The generally agreed (industry-wide) standard for jumbo packet MTU's is 9000 
bytes, but most will support up to 9216. That does, however, depend on the 
maximum MTU configured by the network admins at any given location or 
organisation. 

Any packets larger than that must be fragmented by the network equipment, and 
that means CPU/Process switching all such packets rather than hardware 
switching, which adds to CPU and memory usage on the network kit and gets the 
network admins offside.

The only time <1500 is really needed is for network links where tunnelling/
encapsulation is involved (e.g. VPN links using GRE, IPSec, IP-in-IP or some 
other encapsulation that results in a reduced MTU). In this case it should be 
able to be configured for values anywere from 1316 to 1422. Any network admin 
worth his salt will know how to check the maximum MTU for a given network path 
and advise the app owner to set the values accordingly. 
> 
> For reading, you can keep the 9964 number always.

No harm in that, agreed. 

[...]
Nicolas George Feb. 1, 2021, 2:22 p.m. UTC | #12
Sergio M. Ammirata, Ph.D. (12021-01-31):
> For writing, the data you give librist, will go out "as is"
> to the network with some added protocol overhead bytes (28
> bytes without encryption enabled and 36 bytes with
> encryption enabled).

Can you clarify something? Is this supposed to be a packet protocol or a
stream protocol?

I.e., if the reader is waiting for 100 octets, and the writer sent 20
then 30 then 40, will the reader get three reads of 20, 30, 40 or a
single read of 20+30+40=90?

> To avoid your data being fragmented because of network MTU,
> you should really limit the write size to what you want
> your IP packets to be. For mpegts, for example, 1316 is a
> good number to keep the overall packet size below the
> typical internet MTU size of 1500 (1400 to be safe).

It looks to me like reinventing the wheel that TCP had had years to make
nicely round. Do we need to implement the Nagle algorithm?

Regards,
Rodney Baker Feb. 1, 2021, 2:43 p.m. UTC | #13
On Tuesday, 2 February 2021 0:52:23 ACDT Nicolas George wrote:
> Sergio M. Ammirata, Ph.D. (12021-01-31):
> > For writing, the data you give librist, will go out "as is"
> > to the network with some added protocol overhead bytes (28
> > bytes without encryption enabled and 36 bytes with
> > encryption enabled).
> 
> Can you clarify something? Is this supposed to be a packet protocol or a
> stream protocol?
> 
> I.e., if the reader is waiting for 100 octets, and the writer sent 20
> then 30 then 40, will the reader get three reads of 20, 30, 40 or a
> single read of 20+30+40=90?
> 
> > To avoid your data being fragmented because of network MTU,
> > you should really limit the write size to what you want
> > your IP packets to be. For mpegts, for example, 1316 is a
> > good number to keep the overall packet size below the
> > typical internet MTU size of 1500 (1400 to be safe).
> 
> It looks to me like reinventing the wheel that TCP had had years to make
> nicely round. Do we need to implement the Nagle algorithm?
> 
> Regards,

Nagle can introduce unwanted latency which is not desirable for real-time 
streaming. Mind you, tcp is inappropriate for real-time streaming anyway - 
that's what rtsp was invented for. I don't think Nagle belongs at app level 
anyway - it's a network stack function (that can be disabled with the 
TCP_NODELAY setting). 

Truth be told, the MTU should be (and is) controlled by the network stack too, 
but there's no reason to cause packet fragmentation (and the increased latency 
assocaited with packet fragmentation and reassembly) by deliberately 
configuring the app to generate packets larger than the largest possible MTU 
(but I don't really have a say in the matter - just providing advice based on 
hard-learned lessons and experience tuning networks where the largest 
bandwidth application - by orders of magnitude - is real-time streaming 
video). 

Regards,
Rodney.
Nicolas George Feb. 1, 2021, 2:45 p.m. UTC | #14
Rodney Baker (12021-02-02):
> Nagle can introduce unwanted latency which is not desirable for real-time 
> streaming. Mind you, tcp is inappropriate for real-time streaming anyway - 
> that's what rtsp was invented for. I don't think Nagle belongs at app level 
> anyway - it's a network stack function (that can be disabled with the 
> TCP_NODELAY setting). 

I know all that. Please do not answer ONLY to the last sentence of my
mail. You have not answered the important part:

> > Can you clarify something? Is this supposed to be a packet protocol or a
> > stream protocol?
> > 
> > I.e., if the reader is waiting for 100 octets, and the writer sent 20
> > then 30 then 40, will the reader get three reads of 20, 30, 40 or a
> > single read of 20+30+40=90?

Regards,
Sergio M. Ammirata, Ph.D. Feb. 1, 2021, 3:15 p.m. UTC | #15
> Can you clarify something? Is this supposed to be a
> packet protocol or astream protocol?
> I.e., if the reader is waiting for 100 octets, and the
> writer sent 20then 30 then 40, will the reader get three
> reads of 20, 30, 40 or asingle read of 20+30+40=90?

This is a packet protocol. In your example above, the
reader will get three reads of 20, 30 and 40
Sergio
On Mon, 2021-02-01 at 15:22 +0100, Nicolas George wrote:
> Sergio M. Ammirata, Ph.D. (12021-01-31):
> For writing, the data you give librist, will go out "as
> is"to the network with some added protocol overhead bytes
> (28bytes without encryption enabled and 36 bytes
> withencryption enabled).
> Can you clarify something? Is this supposed to be a
> packet protocol or astream protocol?
> I.e., if the reader is waiting for 100 octets, and the
> writer sent 20then 30 then 40, will the reader get three
> reads of 20, 30, 40 or asingle read of 20+30+40=90?
> To avoid your data being fragmented because of network
> MTU,you should really limit the write size to what you
> wantyour IP packets to be. For mpegts, for example, 1316
> is agood number to keep the overall packet size below
> thetypical internet MTU size of 1500 (1400 to be safe).
> It looks to me like reinventing the wheel that TCP had
> had years to makenicely round. Do we need to implement
> the Nagle algorithm?
> Regards,
> 
> _______________________________________________ffmpeg-
> devel mailing listffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> To unsubscribe, visit link above, or 
> emailffmpeg-devel-request@ffmpeg.org with subject
> "unsubscribe".
Nicolas George Feb. 1, 2021, 3:25 p.m. UTC | #16
Sergio M. Ammirata, Ph.D. (12021-02-01):
> This is a packet protocol. In your example above, the
> reader will get three reads of 20, 30 and 40

Thank you for the clarification. I looked a little in the code in the
meantime.

Considering what was said, h->max_packet_size should be set to the
maximum packet size allowed by librist. Unfortunately, this number seems
to be RIST_MAX_PACKET_SIZE and defined in a private.

It does not make sense to have packet_size an option if it is not
actually applied to librist, as it is in the current version of the
patch.

For reading, max_packet_size must not be too low. For writing, it can be
lowered, but it is not specific to this particular protocol.

Regards,
Sergio M. Ammirata, Ph.D. Feb. 1, 2021, 3:30 p.m. UTC | #17
On Mon, 2021-02-01 at 16:25 +0100, Nicolas George wrote:
> Sergio M. Ammirata, Ph.D. (12021-02-01):
> This is a packet protocol. In your example above,
> thereader will get three reads of 20, 30 and 40
> Thank you for the clarification. I looked a little in the
> code in themeantime.
> Considering what was said, h->max_packet_size should be
> set to themaximum packet size allowed by librist.
> Unfortunately, this number seemsto be
> RIST_MAX_PACKET_SIZE and defined in a private.
> It does not make sense to have packet_size an option if
> it is notactually applied to librist, as it is in the
> current version of thepatch.
> For reading, max_packet_size must not be too low. For
> writing, it can belowered, but it is not specific to this
> particular protocol.

Correct, the write size has more to do with the desired IP
packet size than the protocol itself. It would be the exact
equivalent to the pkt_size setting on the udp output
module.
Sergio
> Regards,
> 
> _______________________________________________ffmpeg-
> devel mailing listffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> To unsubscribe, visit link above, or 
> emailffmpeg-devel-request@ffmpeg.org with subject
> "unsubscribe".
diff mbox series

Patch

diff --git a/configure b/configure
index df298b4b9b..d66966e19f 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
@@ -3490,6 +3492,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"
@@ -6409,6 +6413,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.4.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 c0b511b7a4..3160f846ff 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -690,6 +690,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 3a8fbcbe5f..85f432c8d9 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..083bb25c4b
--- /dev/null
+++ b/libavformat/librist.c
@@ -0,0 +1,251 @@ 
+/*
+ * 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 *read_peer;
+    struct rist_peer *write_peer;
+    struct rist_ctx *read_ctx;
+    struct rist_ctx *write_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->read_peer = NULL;
+    s->write_peer = NULL;
+
+    if (s->write_ctx)
+        ret = rist_destroy(s->read_ctx);
+    if (s->read_ctx)
+        ret = rist_destroy(s->read_ctx);
+    s->read_ctx = NULL;
+    s->write_ctx = NULL;
+
+    return risterr2ret(ret);
+}
+
+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->write_ctx, s->profile, 0, logging_settings);
+    if (ret < 0)
+        goto err;
+
+    if (flags & AVIO_FLAG_READ)
+        ret = rist_receiver_create(&s->read_ctx, s->profile, logging_settings);
+    if (ret < 0)
+        goto err;
+
+    ret = rist_peer_config_defaults_set(peer_config);
+    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;
+
+    if (s->buffer_size) {
+        peer_config->recovery_length_min = s->buffer_size;
+        peer_config->recovery_length_max = s->buffer_size;
+    }
+
+    if (flags & AVIO_FLAG_READ)
+        ret = rist_peer_create(s->read_ctx, &s->read_peer, &s->peer_config);
+    if (ret < 0)
+        goto err;
+
+    if (flags & AVIO_FLAG_WRITE)
+        ret = rist_peer_create(s->write_ctx, &s->write_peer, &s->peer_config);
+    if (ret < 0)
+        goto err;
+
+    if (flags & AVIO_FLAG_READ)
+        ret = rist_start(s->read_ctx);
+    if (ret < 0)
+        goto err;
+    if (flags & AVIO_FLAG_WRITE)
+        ret = rist_start(s->write_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->read_ctx, &data_block, POLLING_TIME);
+    if (ret < 0)
+        return risterr2ret(ret);
+
+    if (ret > 0 && data_block->payload) {
+        if (data_block->payload_len > size)
+            av_log(h, AV_LOG_WARNING, "Part of datagram lost due to insufficient buffer size\n");
+        size = FFMIN(size, data_block->payload_len);
+        memcpy(buf, data_block->payload, size);
+        rist_receiver_data_block_free(&data_block);
+        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->write_ctx, &data_block);
+    if (ret < 0)
+        return risterr2ret(ret);
+
+    return ret;
+}
+
+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;