diff mbox series

[FFmpeg-devel,2/2] libRIST: allow setting fifo size and fail on overflow.

Message ID 20211117141929.1164334-2-gijs@peskens.net
State New
Headers show
Series [FFmpeg-devel,1/2] libRIST: Correctly initialize logging_settings | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Gijs Peskens Nov. 17, 2021, 2:19 p.m. UTC 
Introduce fifo_size and overrun_nonfatal params to configure fifo buffer
behavior.
Fifo size is used to left shift 2, since libRIST only accepts powers of 2.

Use newly introduced RIST_DATA_FLAGS_OVERFLOW flag to check for overrun
and error out in that case.
---
 libavformat/librist.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Marton Balint Nov. 18, 2021, 7:35 p.m. UTC | #1 
On Wed, 17 Nov 2021, Gijs Peskens wrote:

> Introduce fifo_size and overrun_nonfatal params to configure fifo buffer
> behavior.
> Fifo size is used to left shift 2, since libRIST only accepts powers of 2.

Can you please make fifo_size simply mean the number of packets? User 
facing options should be easily understandable by the user. Even if 
librist (at the moment) only supports power of two values.

>
> Use newly introduced RIST_DATA_FLAGS_OVERFLOW flag to check for overrun
> and error out in that case.
> ---
> libavformat/librist.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/libavformat/librist.c b/libavformat/librist.c
> index 378b635ea7..6f68868f3c 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_DEFAULT_SHIFT 13
>
> typedef struct RISTContext {
>     const AVClass *class;
> @@ -52,6 +55,8 @@ typedef struct RISTContext {
>     int packet_size;
>     int log_level;
>     int encryption;
> +    int fifo_shift;
> +    bool 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, applied as: buffer_size=2^fifo_size", OFFSET(fifo_shift), AV_OPT_TYPE_INT, {.i64=FF_LIBRIST_FIFO_DEFAULT_SHIFT}, 10, 63, .flags = D|E },
> +    { "overrun_nonfatal", "survive in case of libRIST receiving circular buffer overrun", OFFSET(overrun_nonfatal), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1,    D },

This changes existing default behaviour slightly by exiting in case of an 
overflow. I guess that it makes it consistent with udp.c, so fine with me 
if you think it is better that way.

>     { "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,19 @@ 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, 2 << s->fifo_shift);
> +        if (ret != 0)
> +            goto err;
> +    }
> +#else
> +    if (s->fifo_buffer_size != FF_LIBRIST_FIFO_DEFAULT) {
> +        av_log(h, AV_LOG_ERROR, "libRIST prior to 0.2.7 has a bug which fails setting the fifo buffer size");
> +    }
> +#endif
> +
>     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");
> @@ -223,8 +243,24 @@ static int librist_read(URLContext *h, uint8_t *buf, int size)
>         return AVERROR_EXTERNAL;
>     }
>
> +#if FF_LIBRIST_VERSION >= FF_LIBRIST_VERSION_42
> +    if (data_block->flags & RIST_DATA_FLAGS_OVERFLOW == RIST_DATA_FLAGS_OVERFLOW) {
> +        if (!s->overrun_nonfatal) {
> +            av_log(h, AV_LOG_ERROR, "Fifo buffer overrun. "
> +                    "To avoid, increase fifo_size URL option. "
> +                    "To survive in such case, use overrun_nonfatal option\n");
> +            size = AVERROR(EIO);
> +            goto out_free;
> +        } else {
> +            av_log(h, AV_LOG_WARNING, "Fifo buffer buffer overrun. "
> +                    "Surviving due to overrun_nonfatal option\n");
> +        }
> +    }
> +#endif
> +
>     size = data_block->payload_len;
>     memcpy(buf, data_block->payload, size);
> +out_free:
> #if FF_LIBRIST_VERSION < FF_LIBRIST_VERSION_41
>     rist_receiver_data_block_free((struct rist_data_block**)&data_block);
> #else
> -- 
> 2.32.0

doc/protocols.texi update is missing for the new options.

Thanks,
Marton
Gijs Peskens Nov. 20, 2021, 10:52 a.m. UTC | #2 
Thanks for reviewing again!

On 18-11-2021 20:35, Marton Balint wrote:
>
>
> On Wed, 17 Nov 2021, Gijs Peskens wrote:
>
>> Introduce fifo_size and overrun_nonfatal params to configure fifo buffer
>> behavior.
>> Fifo size is used to left shift 2, since libRIST only accepts powers 
>> of 2.
>
> Can you please make fifo_size simply mean the number of packets? User 
> facing options should be easily understandable by the user. Even if 
> librist (at the moment) only supports power of two values.
>
Yes, will do. libRIST will error out with a log message when it's not 
accepted, so I'd forego checking if the chosen number is a power of 2 on 
the ffmpeg side.
>>
>> Use newly introduced RIST_DATA_FLAGS_OVERFLOW flag to check for overrun
>> and error out in that case.
>> ---
>> libavformat/librist.c | 36 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/libavformat/librist.c b/libavformat/librist.c
>> index 378b635ea7..6f68868f3c 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_DEFAULT_SHIFT 13
>>
>> typedef struct RISTContext {
>>     const AVClass *class;
>> @@ -52,6 +55,8 @@ typedef struct RISTContext {
>>     int packet_size;
>>     int log_level;
>>     int encryption;
>> +    int fifo_shift;
>> +    bool 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, applied as: 
>> buffer_size=2^fifo_size", OFFSET(fifo_shift), AV_OPT_TYPE_INT, 
>> {.i64=FF_LIBRIST_FIFO_DEFAULT_SHIFT}, 10, 63, .flags = D|E },
>> +    { "overrun_nonfatal", "survive in case of libRIST receiving 
>> circular buffer overrun", OFFSET(overrun_nonfatal), AV_OPT_TYPE_BOOL, 
>> {.i64 = 0}, 0, 1,    D },
>
> This changes existing default behaviour slightly by exiting in case of 
> an overflow. I guess that it makes it consistent with udp.c, so fine 
> with me if you think it is better that way.
If desired I can reverse the defaults so existing workflows are not 
impacted (though I doubt many exist due to the young age of the librist 
module).
Though the consistency with udp feels more logical to me.
>
>>     { "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,19 @@ 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, 2 << 
>> s->fifo_shift);
>> +        if (ret != 0)
>> +            goto err;
>> +    }
>> +#else
>> +    if (s->fifo_buffer_size != FF_LIBRIST_FIFO_DEFAULT) {
>> +        av_log(h, AV_LOG_ERROR, "libRIST prior to 0.2.7 has a bug 
>> which fails setting the fifo buffer size");
>> +    }
>> +#endif
>> +
>>     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");
>> @@ -223,8 +243,24 @@ static int librist_read(URLContext *h, uint8_t 
>> *buf, int size)
>>         return AVERROR_EXTERNAL;
>>     }
>>
>> +#if FF_LIBRIST_VERSION >= FF_LIBRIST_VERSION_42
>> +    if (data_block->flags & RIST_DATA_FLAGS_OVERFLOW == 
>> RIST_DATA_FLAGS_OVERFLOW) {
>> +        if (!s->overrun_nonfatal) {
>> +            av_log(h, AV_LOG_ERROR, "Fifo buffer overrun. "
>> +                    "To avoid, increase fifo_size URL option. "
>> +                    "To survive in such case, use overrun_nonfatal 
>> option\n");
>> +            size = AVERROR(EIO);
>> +            goto out_free;
>> +        } else {
>> +            av_log(h, AV_LOG_WARNING, "Fifo buffer buffer overrun. "
>> +                    "Surviving due to overrun_nonfatal option\n");
>> +        }
>> +    }
>> +#endif
>> +
>>     size = data_block->payload_len;
>>     memcpy(buf, data_block->payload, size);
>> +out_free:
>> #if FF_LIBRIST_VERSION < FF_LIBRIST_VERSION_41
>>     rist_receiver_data_block_free((struct 
>> rist_data_block**)&data_block);
>> #else
>> -- 
>> 2.32.0
>
> doc/protocols.texi update is missing for the new options.
>
Will update, ty!


> 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".
diff mbox series

Patch

diff --git a/libavformat/librist.c b/libavformat/librist.c
index 378b635ea7..6f68868f3c 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_DEFAULT_SHIFT 13
 
 typedef struct RISTContext {
     const AVClass *class;
@@ -52,6 +55,8 @@  typedef struct RISTContext {
     int packet_size;
     int log_level;
     int encryption;
+    int fifo_shift;
+    bool 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, applied as: buffer_size=2^fifo_size", OFFSET(fifo_shift), AV_OPT_TYPE_INT, {.i64=FF_LIBRIST_FIFO_DEFAULT_SHIFT}, 10, 63, .flags = D|E },
+    { "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,19 @@  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, 2 << s->fifo_shift);
+        if (ret != 0)
+            goto err;
+    }
+#else 
+    if (s->fifo_buffer_size != FF_LIBRIST_FIFO_DEFAULT) {
+        av_log(h, AV_LOG_ERROR, "libRIST prior to 0.2.7 has a bug which fails setting the fifo buffer size");
+    }
+#endif
+
     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");
@@ -223,8 +243,24 @@  static int librist_read(URLContext *h, uint8_t *buf, int size)
         return AVERROR_EXTERNAL;
     }
 
+#if FF_LIBRIST_VERSION >= FF_LIBRIST_VERSION_42
+    if (data_block->flags & RIST_DATA_FLAGS_OVERFLOW == RIST_DATA_FLAGS_OVERFLOW) {
+        if (!s->overrun_nonfatal) {
+            av_log(h, AV_LOG_ERROR, "Fifo buffer overrun. "
+                    "To avoid, increase fifo_size URL option. "
+                    "To survive in such case, use overrun_nonfatal option\n");
+            size = AVERROR(EIO);
+            goto out_free;
+        } else {
+            av_log(h, AV_LOG_WARNING, "Fifo buffer buffer overrun. "
+                    "Surviving due to overrun_nonfatal option\n");
+        }
+    }
+#endif
+
     size = data_block->payload_len;
     memcpy(buf, data_block->payload, size);
+out_free:
 #if FF_LIBRIST_VERSION < FF_LIBRIST_VERSION_41
     rist_receiver_data_block_free((struct rist_data_block**)&data_block);
 #else