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 5477E42254 for ; Tue, 30 Aug 2022 14:07:55 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4F5F568B737; Tue, 30 Aug 2022 17:07:52 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 44C1968B737 for ; Tue, 30 Aug 2022 17:07:45 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id 56143240D0E for ; Tue, 30 Aug 2022 16:07:44 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id GXTiv-sA_5Vh for ; Tue, 30 Aug 2022 16:07:43 +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 95D29240D03 for ; Tue, 30 Aug 2022 16:07:43 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id 9B09A1601B2; Tue, 30 Aug 2022 16:07:43 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: <36a569a9-5e83-3729-c6b1-93a921a0c659@gmail.com> References: <20220829140717.26557-1-anton@khirnov.net> <20220829140717.26557-2-anton@khirnov.net> <1263f749-8e56-9ba9-0b77-c031a73c8124@gmail.com> <166184135578.25402.18011594660522293560@lain.khirnov.net> <36a569a9-5e83-3729-c6b1-93a921a0c659@gmail.com> Mail-Followup-To: FFmpeg development discussions and patches Date: Tue, 30 Aug 2022 16:07:43 +0200 Message-ID: <166186846359.25402.9011725525394947249@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-30 14:56:45) > > > > 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'. > > IMO it's not really ambiguous. If you don't state that's the intention, > which you're doing in this patch, then "can be written" has one literal > meaning. I would disagree here. Consider an autogrowing fifo in an out-of-memory situation. What "can be written" into it? > > 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. > > av_fifo_write() says "In case nb_elems > av_fifo_can_write(f), nothing > is written and an error is returned.", which is definitely not > ambiguous, and you're changing it in patch 3/3 to include the case where > having enabled autogrow could result in the function succeeding when > nb_elems > av_fifo_can_write(f). That is quite clearly a bug in the documentation IMO. That line was not present in the original patches I sent, but added at some time later in the development (don't remember whether by myself or Andreas); then whichever of us added it forgot to update it in the patch adding AV_FIFO_FLAG_AUTO_GROW. > The behavior of the function remains intact, but a library user reading > the documentation in ffmpeg 5.1 and the documentation in what will be > 5.2 after this patch could rightly assume the function was changed and > will behave differently between versions (Which is not the case, but to > find out you'll have to read the implementation, or the git history, or > test code with both versions). So this is technically an API break. Technically yes, but the unfortunate fact of the matter is that our API documentation simply is not, and never was, sufficiently complete and precise to be the sole source of truth. Plenty of things are missing, obsolete, inconsistent, and sometimes just wrong. I wish it were otherwise, and I believe the situation is slowly improving, but we just don't have the resources to make our docs anywhere close to perfect any time soon. So unfortunately people have to test their code, and testing in this case would immediately reveal how it actually works. As a consequence we have to be pragmatic when choosing whether to change code to match the docs or vice versa. > > > > > More generally: > > - a FIFO conceptually has a well-defined size at any given moment > > - that size is can_read() + can_write() > > But this could (should?) have been av_fifo_size2(). That way can_write() > could effectively become a generic "can write", instead of begin stuck > as "can write without the chance of failure". Maybe, but it's a bit late for that. Actually I remember considering an av_fifo_size2(), but then decided against it, probably because it could confuse people into thinking it's like av_fifo_size(), which it most definitely is not. -- 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".