Re: [FFmpeg-devel] [PATCH] libRIST: allow setting fifo size and fail on overflow.

[Marton Balint <cus@passwd.hu>]

On Tue, 1 Feb 2022, Marton Balint wrote:

>
>
> On Tue, 11 Jan 2022, Gijs Peskens wrote:
>
>>  Introduce fifo_size and overrun_nonfatal params to configure fifo buffer
>>  behavior.
>>
>>  Use newly introduced RIST_DATA_FLAGS_OVERFLOW flag to check for overrun
>>  and error out in that case.
>>  ---
>>  doc/protocols.texi    |  9 +++++++++
>>  libavformat/librist.c | 37 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+)

Applied with some fixes.

Regards,
Marton

>>
>>  diff --git a/doc/protocols.texi b/doc/protocols.texi
>>  index d207df0b52..f1acf0cc77 100644
>>  --- a/doc/protocols.texi
>>  +++ b/doc/protocols.texi
>> @@  -745,6 +745,15 @@ Set internal RIST buffer size in milliseconds for 
>> @@  retransmission of data.
>>  Default value is 0 which means the librist default (1 sec). Maximum value
>>  is 30
>>  seconds.
>>
>>  +@item fifo_size
>>  +Size of the librist receiver output fifo in number of packets. This must
>>  be a
>>  +power of 2.
>>  +Defaults to 8192 (vs the libRIST default of 1024).
>>  +
>>  +@item overrun_nonfatal=@var{1|0}
>>  +Survive in case of libRIST fifo buffer overrun. Default
>>  +value is 0.
>
> Please use "librist" (all lowercase) consistently.
>
>>  +
>> @ item pkt_size
>>  Set maximum packet size for sending data. 1316 by default.
>>
>>  diff --git a/libavformat/librist.c b/libavformat/librist.c
>>  index 378b635ea7..87c14eb265 100644
>>  --- a/libavformat/librist.c
>>  +++ b/libavformat/librist.c
>> @@  -43,6 +43,9 @@
>>      ((patch) + ((minor)* 0x100) + ((major) *0x10000))
>>  #define FF_LIBRIST_VERSION
>>  FF_LIBRIST_MAKE_VERSION(LIBRIST_API_VERSION_MAJOR,
>>  LIBRIST_API_VERSION_MINOR, LIBRIST_API_VERSION_PATCH)
>>  #define FF_LIBRIST_VERSION_41 FF_LIBRIST_MAKE_VERSION(4, 1, 0)
>>  +#define FF_LIBRIST_VERSION_42 FF_LIBRIST_MAKE_VERSION(4, 2, 0)
>>  +
>>  +#define FF_LIBRIST_FIFO_SIZE_DEFAULT 8192
>
> FF_LIBRIST prefix is not really needed, as this is not in a header.
>
>>
>>  typedef struct RISTContext {
>>      const AVClass *class;
>> @@  -52,6 +55,8 @@ typedef struct RISTContext {
>>      int packet_size;
>>      int log_level;
>>      int encryption;
>>  +    int fifo_size;
>>  +    int overrun_nonfatal;
>>      char *secret;
>>
>>      struct rist_logging_settings logging_settings;
>> @@  -70,6 +75,8 @@ static const AVOption librist_options[] = {
>>      { "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 in ms", OFFSET(buffer_size),
>>      AV_OPT_TYPE_INT, {.i64=0},                 0, 30000, .flags = D|E },
>>  +    { "fifo_size", "Set libRIST fifo buffer. Size must be power of 2",
>>  OFFSET(fifo_size), AV_OPT_TYPE_INT, {.i64=FF_LIBRIST_FIFO_SIZE_DEFAULT}, 2
>>  << 9, 2 << 15, .flags = D|E },
>
> Minimum and maximum binary notation is very strange. Use 1<<power, or maybe 
> I'd just write 1024 and 65536. Much more readable. Are these arbitrary 
> limits, or the implementation only supports these? Maybe you can be a bit 
> more lax, e.g. 256 minimum and 262144 max.
>
>>  +    { "overrun_nonfatal", "survive in case of libRIST receiving circular
>>  buffer overrun", OFFSET(overrun_nonfatal), AV_OPT_TYPE_BOOL, {.i64 = 0},
>>  0, 1,    D },
>>      { "pkt_size",    "set packet size", OFFSET(packet_size),
>>      AV_OPT_TYPE_INT,   {.i64=1316},                  1, MAX_PAYLOAD_SIZE,
>>      .flags = D|E },
>>      { "log_level",   "set loglevel",    OFFSET(log_level),
>>      AV_OPT_TYPE_INT,   {.i64=RIST_LOG_INFO},        -1, INT_MAX, .flags =
>>      D|E },
>>      { "secret", "set encryption secret",OFFSET(secret),
>>      AV_OPT_TYPE_STRING,{.str=NULL},                  0, 0,       .flags =
>>      D|E },
>> @@  -161,6 +168,20 @@ static int librist_open(URLContext *h, const char 
>> @@  *uri, int flags)
>>      if (ret < 0)
>>          goto err;
>>
>>  +    //Prior to 4.2.0 there was a bug in libRIST which made this call
>>  always fail.
>>  +#if FF_LIBRIST_VERSION >= FF_LIBRIST_VERSION_42
>>  +    if (flags & AVIO_FLAG_READ) {
>>  +        ret = rist_receiver_set_output_fifo_size(s->ctx, s->fifo_size);
>>  +        if (ret != 0) {
>>  +            goto err;
>>  +        }
>>  +    }
>>  +#else
>>  +    if (s->fifo_size != FF_LIBRIST_FIFO_SIZE_DEFAULT) {
>
> Isn't this supposed to be if (s->fifo_size != 1024)? Considering that we are 
> aiming for a bigger default in ffmpeg compared to what is in the library as 
> default, maybe the user should be aware that we cannot set it?
>
>>  +        av_log(h, AV_LOG_ERROR, "libRIST prior to 0.2.7 has a bug which
>>  fails setting the fifo buffer size\n");
>>  +    }
>>  +#endif
>>  +
>
> 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".
>
_______________________________________________
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".