From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 872A8433EC for ; Thu, 9 Jun 2022 22:05:52 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 24B2E68B7EA; Fri, 10 Jun 2022 01:05:50 +0300 (EEST) Received: from iq.passwd.hu (iq.passwd.hu [217.27.212.140]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A53B068B3FF for ; Fri, 10 Jun 2022 01:05:43 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by iq.passwd.hu (Postfix) with ESMTP id 0500EE6A1A for ; Fri, 10 Jun 2022 00:05:44 +0200 (CEST) X-Virus-Scanned: amavisd-new at passwd.hu Received: from iq.passwd.hu ([127.0.0.1]) by localhost (iq.passwd.hu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AB-9HoLPyA79 for ; Fri, 10 Jun 2022 00:05:41 +0200 (CEST) Received: from iq (iq [217.27.212.140]) by iq.passwd.hu (Postfix) with ESMTPS id 323A3E5523 for ; Fri, 10 Jun 2022 00:05:41 +0200 (CEST) Date: Fri, 10 Jun 2022 00:05:41 +0200 (CEST) From: Marton Balint To: FFmpeg development discussions and patches In-Reply-To: <8eaadf2e-6c71-581a-677a-fa46a7f049ca@passwd.hu> Message-ID: <366220ee-ccee-286b-522f-53f1d2fc6c1b@passwd.hu> References: <20220111150745.233121-1-gijs@peskens.net> <8eaadf2e-6c71-581a-677a-fa46a7f049ca@passwd.hu> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH] libRIST: allow setting fifo size and fail on overflow. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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< 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".