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

Will apply soon.

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

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

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

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

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

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

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

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

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

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. 

[...]

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,

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.

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,

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

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,

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