[FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12023-01-31):
>> Details please.
> 
> I was not going to spend time explaining unless I had assurance it was
> because the issue of requiring changes in the internal implementation
> and developers habits just to hide some fields was taken into account.
> Coming from somebody else, I would have expected the question to be only
> to find the first excuses to shoot the proposal down.
> 
> But you more or less found the same ideas I did anyway.
> 
>> I can only think of the following:
>> a) Allow the use of -fms-extensions. This allows structs with tags and
>> typedefs thereof to be used as unnamed struct members with the members
>> of the unnamed structure being treated as members of the enclosing
>> structure. One could then use a pointer to the big internal structure
>> internally and one does not need to remember whether a field is internal
>> or not. There is still a problem, though: One needs to cast from
>> AVFilterContext.(inputs|outputs). This should be done via dedicated
>> inlined getters, but this is a bit more typing. E.g.
>> "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it
>> might also be shorter if someone has a short name.
>> GCC, Clang, MSVC and the IIRC the intel compilers support this.
>>
>> b) Add a big #define AVFILTERLINK in avfilter.h that expands to the
>> public part of AVFilterLink and change the declaration of AVFilterLink
>> to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal
>> struct via
>> "struct FilterLinkInternal {
>>     AVFILTERLINK
>>     /* actual internal fields */
>> };"
>> This has the downside of actually being an aliasing violation and of
>> adding considerable ugliness to avfilter.h, in particular during
>> deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via
>> #if in the implementation of a macro). I also don't know whether it
>> plays nicely with tools that deal with source code annotations.
> 
> Or use a “#include "avfilter_link_internal_fields.h" instead of a macro.
> 
>> c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally
>> using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter).
>> d) Same as c), but strip this stuff from installed headers.
>>
>> I consider b)-d) as inferior to a), which I consider superior to Anton's
>> proposal, but the big drawback is its reliance on a compiler extension.
> 
> Once and for all: If the solution requires changing things in the
> internal implementation C files and changing the habits of the people
> who work on these files (that goes together), then it is inferior to a
> solution that does not require that.
> 
> Your solution (a) has this flaw, plus relies on an extension that is
> probably not available on all supported platforms (Microsoft I am
> looking at you).
> 

It's called -fms-extensions for a reason.

> My favor goes to (d). Probably with a stricter test for the (c) part
> because the internal fields are not necessary for the filters and
> therefore could be hidden there too.
> 

This can be accomplished with a), too. All one has to do is use multiple
levels of extensions.

- Andreas

PS: Upon rethinking, it is not only b) that contains undefined
behaviour; it is b)-d) as well as the current code. The reason is that
the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is
not compatible with the type of AVFilterLink from the files without
FF_INTERNAL_FIELDS. This also makes derived types, like the types of
function declarations containing pointers to AVFilterLink incompatible
and this is a violation of 6.2.7(2) of C11. I wonder if this will become
a real problem with lto someday.