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 863C441270 for ; Tue, 30 Aug 2022 06:36:07 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2C45068BAFE; Tue, 30 Aug 2022 09:36:05 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9B57668B76F for ; Tue, 30 Aug 2022 09:35:58 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id 21A4A240D0E for ; Tue, 30 Aug 2022 08:35:58 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id UlWGUYcShbhr for ; Tue, 30 Aug 2022 08:35:57 +0200 (CEST) Received: from lain.khirnov.net (lain.khirnov.net [IPv6:2001:67c:1138:4306::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "lain.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id 75B1D240D03 for ; Tue, 30 Aug 2022 08:35:57 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id C94D91601B2; Tue, 30 Aug 2022 08:35:55 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: <1263f749-8e56-9ba9-0b77-c031a73c8124@gmail.com> References: <20220829140717.26557-1-anton@khirnov.net> <20220829140717.26557-2-anton@khirnov.net> <1263f749-8e56-9ba9-0b77-c031a73c8124@gmail.com> Mail-Followup-To: FFmpeg development discussions and patches Date: Tue, 30 Aug 2022 08:35:55 +0200 Message-ID: <166184135578.25402.18011594660522293560@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 2/3] lavu/fifo: clarify interaction of AV_FIFO_FLAG_AUTO_GROW with av_fifo_can_write() 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Quoting James Almer (2022-08-29 17:00:54) > On 8/29/2022 11:07 AM, Anton Khirnov wrote: > > --- > > libavutil/fifo.h | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/libavutil/fifo.h b/libavutil/fifo.h > > index 6c6bd78842..89872d0972 100644 > > --- a/libavutil/fifo.h > > +++ b/libavutil/fifo.h > > @@ -97,7 +97,13 @@ void av_fifo_auto_grow_limit(AVFifo *f, size_t max_elems); > > size_t av_fifo_can_read(const AVFifo *f); > > > > /** > > - * @return number of elements that can be written into the given FIFO. > > + * @return Number of elements that can be written into the given FIFO without > > + * growing it. > > + * > > + * In other words, this number of elements or less is guaranteed to fit > > + * into the FIFO. More data may be written when the > > + * AV_FIFO_FLAG_AUTO_GROW flag was specified at FIFO creation, but this > > + * may involve memory allocation, which can fail. > > This patch is an API break, because before it i was told > av_fifo_can_write() would tell me the amount of elements i could write > into the FIFO, regardless of how it was created, but now it legitimates > the one scenario where it was not reliable. An scenario i stumbled upon > in my code by following the documentation, which is in at least one > release, the LTS one. > > Instead of changing the documentation to fit the behavior, the behavior > should match the documentation. This means that if a call to > av_fifo_write() can succeed, then av_fifo_can_write() should reflect that. > > That said, it would be great if making av_fifo_can_write() tell the real > amount of elements one can write into the FIFO was possible without > breaking anything, but the doxy for av_fifo_grow2() says "On success, > the FIFO will be large enough to hold exactly inc + av_fifo_can_read() + > av_fifo_can_write()", a line that was obviously aware of the fact > av_fifo_can_write() ignored the autogrow feature, and would no longer be > true if said function is fixed. I disagree that this is a break. The issue in my view is that 'can be written' is ambiguous here, so we are interpreting it differently. Your interpretation is apparently 'maximum number of elements for which a write can possibly succeeed', whereas my intended interpretation was 'maximum number of elements for which a write is always guaranteed to succeed'. One of these interpretations is correct, because it matches the actual behaviour. So the right solution IMO is to clarify the documentation so it is no longer ambiguous, but I do not consider this an API break. More generally: - a FIFO conceptually has a well-defined size at any given moment - that size is can_read() + can_write() - you can change the size by growing the FIFO - AV_FIFO_FLAG_AUTO_GROW does not fundamentally change the above concepts, it merely calls av_fifo_grow2() when a write would otherwise fail Now we can introduce a function for 'maximum number that can possibly succeed' if you think it's useful - something like av_fifo_max_write(). -- Anton Khirnov _______________________________________________ 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".