Re: [FFmpeg-devel] [PATCH 2/2] libRIST: allow setting fifo size and fail on overflow.
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) *0x1))
#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, 3, .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
Re: [FFmpeg-devel] [PATCH 2/2] libRIST: allow setting fifo size and fail on overflow.
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) *0x1))
#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, 3, .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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or
[FFmpeg-devel] [PATCH 2/2] libRIST: allow setting fifo size and fail on overflow.
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(+)
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) *0x1))
#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, 3, .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
--
2.32.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
