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 639DB407BE for ; Tue, 30 Aug 2022 12:57:00 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 5BAB968BA1A; Tue, 30 Aug 2022 15:56:58 +0300 (EEST) Received: from mail-oa1-f52.google.com (mail-oa1-f52.google.com [209.85.160.52]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 44E6268B9C1 for ; Tue, 30 Aug 2022 15:56:52 +0300 (EEST) Received: by mail-oa1-f52.google.com with SMTP id 586e51a60fabf-11f34610d4aso5633059fac.9 for ; Tue, 30 Aug 2022 05:56:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=QvMgcCJQ1J4Lgqes91bRm7pfWyuxCwDabwvpWBoGwEo=; b=StBWX4wTzL4wbKnNORpGeS/465vQIOIRb1SumStOIIdhklPlWipvrXML608BeIHBwR dH0cfi10wTOHbGxE/7s49duEq34FeJt/3EY83Ho0qTRN260GbuguzT4uEl7pJbbIcNx1 3ZLqYJUtHQM9M+44YEgE93Gfc+Wt2Iv46q/ke60g0fdylTXYpa/kKi3Bn81G8fHEqiXE 1CZf/VZMiXP8r76DzL46sRNw+/sN/E4GX9iUjWaoPdJ6kQhZAqn2YeyLyGfBqHEgou0H SNxoW4EnXJzniTUAqf5NqxhHbcmbTgMgtDFc/Wte7KsMcGyHqN2xHKCBBy6DaZ/i+Pdl c+2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=QvMgcCJQ1J4Lgqes91bRm7pfWyuxCwDabwvpWBoGwEo=; b=csMF1hrlSpsyTYBpIXs84TJWI/Qpcp9HFX/LDITO6Mn5dKWwdXEgbV47pzfsTUZacd oTgfi7nrsFvG0szFpOg8jotu1HOBV7xtB/jC9REsHwQklxykrfAPHB0j0AIQHtFevHQd 7xW30NCCJp9zvzp74FeEtQBfLA7+UpTD9sQ3oVc6CPnNbVVbDzTdNwGs1xqbUt+VBAd/ SM7rz2oe86FFOTLKvHoH2Ca/4XkTixK1kEP70gAI5F/FnkVww2EFLAb4QPH8en0eoMnr JUXGDLWDjs0qH/xngrBR55ANSL3Ffc/Dm6TZTkRkseMATKgGGsiPfGY6lu5/xqt79GLD I19Q== X-Gm-Message-State: ACgBeo1t0RZ2PCWCx2O0bjXe/itejnR/HKqiOTPqU8+rbjgIlPn1fovb iSv2QvdHbr2E6cjyjNfxh8hWZDbC2+0= X-Google-Smtp-Source: AA6agR4tAtIV1HFruYxwiZw9blV5ByUV0tyaPaTeGBBSzf4/TSVfsp4zbCrmobPJhG1ttFHSoqFWvQ== X-Received: by 2002:a05:6870:3048:b0:11e:9378:b142 with SMTP id u8-20020a056870304800b0011e9378b142mr8826333oau.255.1661864210079; Tue, 30 Aug 2022 05:56:50 -0700 (PDT) Received: from [192.168.0.13] ([191.97.187.183]) by smtp.gmail.com with ESMTPSA id i19-20020a056871029300b0011c8c2c9020sm7777791oae.33.2022.08.30.05.56.46 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 05:56:47 -0700 (PDT) Message-ID: <36a569a9-5e83-3729-c6b1-93a921a0c659@gmail.com> Date: Tue, 30 Aug 2022 09:56:45 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org 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> From: James Almer In-Reply-To: <166184135578.25402.18011594660522293560@lain.khirnov.net> 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-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 8/30/2022 3:35 AM, Anton Khirnov wrote: > 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'. 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. > 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). 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. > > 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". > - 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(). Maybe, but only if we find a usecase for it. Hendrik had an opinion about what av_fifo_can_write() should report, but it was on IRC. But otherwise, knowing that anything we do will be breaking, if the current behavior was your intention all along as the author of the API, then i guess this set can go in unless someone else chimes. _______________________________________________ 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".