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 DADC240B86 for ; Fri, 3 Feb 2023 14:45:50 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D037E68BDD3; Fri, 3 Feb 2023 16:45:46 +0200 (EET) Received: from nef.ens.fr (nef2.ens.fr [129.199.96.40]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C015068BBF7 for ; Fri, 3 Feb 2023 16:45:39 +0200 (EET) X-ENS-nef-client: 129.199.129.80 ( name = phare.normalesup.org ) Received: from phare.normalesup.org (phare.normalesup.org [129.199.129.80]) by nef.ens.fr (8.14.4/1.01.28121999) with ESMTP id 313Ejcf9010763 for ; Fri, 3 Feb 2023 15:45:38 +0100 Received: by phare.normalesup.org (Postfix, from userid 1001) id 912D7EB5BB; Fri, 3 Feb 2023 15:45:38 +0100 (CET) Date: Fri, 3 Feb 2023 15:45:38 +0100 From: Nicolas George To: FFmpeg development discussions and patches Message-ID: References: <167517470800.4503.4882536660599256492@lain.khirnov.net> <167517869808.4503.13103529212008047943@lain.khirnov.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.4.3 (nef.ens.fr [129.199.96.32]); Fri, 03 Feb 2023 15:45:38 +0100 (CET) Subject: Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS 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: Andreas Rheinhardt (12023-02-01): > "One special guarantee is made in order to simplify the use of unions: > if a union contains several structures that share a common initial > sequence (see below), and if the union object currently contains one of > these structures, it is permitted to inspect the common initial part of > any of them anywhere that a declaration of the completed type of the > union is visible. Two structures share a common initial sequence if > corresponding members have compatible types (and, for bit-fields, the > same widths) for a sequence of one or more initial members." > > But there just is no union between the FF_INTERNAL_FIELDS > !defined(FF_INTERNAL_FIELDS) structures in the whole codebase. It is not necessary that there is one: it is enough that there could be one in another compilation unit, the code that handles these structures does not know what exists in the rest of the code base. This guarantee was made FOR unions, but it does not REQUIRE unions, it applies to anything that is semantically equivalent to a union. > Furthermore, said guarantee is only for inspecting, i.e. reading. For > example, for the following two structs sharing a common initial sequence, > > struct Small { > uint64_t foo; > uint32_t bar; > }; > struct Big { > uint64_t foo; > uint32_t bar; > uint32_t baz; > }; > > if one had a union { struct Small; struct Big; }, a write to Small.bar > may clobber Big.baz. That is a good point. It does not apply when the first field of Big is Small itself, which is the case that I absolutely know is supported, because in that case Big will embed the padding of Small. I had not thought of this. Fortunately, even if we are not 100% in the case that is officially supported, there are multiple reasons that each should be enough to guarantee that our code will work: - Between the public part of the structure and the #ifede FF_INTERNAL_FIELDS, there are a lot of "not part of the public API" fields", changing ch_layout will not clobber incfg because incfg is known at this point. - Even if there was no fields in between, reserved[] offers the same protection. - And even if there was no gap, we are good because applications, and even filters, are not supposed to modify AVFilterLink, only the framework and the framework knows the whole structure. In fact, that last remark makes me think of another solution, for the slightly longer term: make AVFilterLink entirely opaque. The only documented reason for application to access AVFilterLink was to get the parameters of buffersink, but buffersink has a real API to get this since 2016. Anyway, if you prefer using a compilation option, I have no objection. My only concern is that when a developer writes: if (link->x == ... && link->y == ... && link->status_in == ... && link->min_samples == ..) they have to remember that no, status_in is different from all the others. And it is especially bad in this particular case because the distinction is not public / private, which makes some semantic sense and might be easier to remember, the distinction is actually public-or-private-because-of-a-comment / private-because-of-a-ifdef. But even if the distinction really was public / private, I consider it unacceptable if we can do otherwise. And we can. Regards, -- Nicolas George _______________________________________________ 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".