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

[Nicolas George <george@nsup.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3000 bytes --]

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).

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.

Regards,

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".