Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Nicolas George:
> 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.
>
1. The common initial sequence rule indeed forced traditional compilers
to use the same offsets for the members of the common initial sequence
of two arbitrary structs, because there might be a union of these two in
another translation unit. But this is no longer true for lto compilers
that see the whole program at once (although I don't think that they
will deviate from the behaviour of traditional compilers in this regard).
2. But even if the offsets are fine, does not mean that the accesses are
fine. Notice the clause "anywhere that a declaration of the completed
type of the union is visible" above? It is accompanied with the
following example:
"The following is not a valid fragment (because the union type is not
visible within function f):
struct t1 { int m; };
struct t2 { int m; };
int f(struct t1 *p1, struct t2 *p2)
{
if (p1->m < 0)
p2->m = -p2->m;
return p1->m;
}
int g()
{
union {
struct t1 s1;
struct t2 s2;
} u;
/* ... */
return f(, );
}"
>> 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:
>
I'd rather have something that is actually supported and spec-compliant.
Anyway, I worry more about lto-compilers than about traditional
compilers accidentally clobbering something (after all, most of our
fields are sizeof(int)-sized or a multiple (double) thereof, so CPU
instruction sets can write this natively and there is no advantage in
touching padding).
> - 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,
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
On Fri, 2023-02-03 at 15:45 +0100, Nicolas George wrote:
> 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.
This is not valid reasoning. Consider code such as
int f(struct s1 *p1, struct s2 *p2) {
p1->abc = 0;
p2->abc = 42;
return p1->abc;
}
where the field abc belongs to a shared initial sequence in s1 and s2.
If there is no common union visible when this code is compiled, it
seems valid for the compiler to hardcode this to return 0, even if it
may be called with p1 and p2 pointing to the same address. LTO may
generate the equivalent of this code when compiling a combination of
assignments and reads from files using different definitions for a
struct.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Nicolas George:
> Andreas Rheinhardt (12023-02-01):
>> 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.
>
> No: read the second half of the previous paragraph: two structures with
> common first fields are compatible types. What we have been using is a
> deliberately supported construct.
>
"Moreover, two structure, union, or enumerated types declared in
separate translation units are compatible if their tags and members
satisfy the following requirements: If one is declared with a tag, the
other shall be declared with the same tag. If both are completed
anywhere within their respective translation units, then the following
additional requirements apply: there shall be a one-to-one
correspondence between their members such that each pair of
corresponding members are declared with compatible types; if one member
of the pair is declared with an alignment specifier, the other is
declared with an equivalent alignment specifier; and if one member of
the pair is declared with a name, the other is declared with the same
name. For two structures, corresponding members shall be declared in the
same order."
1. There is no one-to-one correspondence (aka a bijection) between the
elements of the complete and of the public structure.
2. In the case of AVFilterLink, there is not even an injection from the
public to the FF_INTERNAL_FIELDS structure (due to the former having a
big reserved array).
3. The fact that these structures share a common initial sequence does
not mean that this is legal. There are some guarantees regarding common
initial sequences in 6.5.2.3 (6):
"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.
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.
- Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Andreas Rheinhardt (12023-02-01): > 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. No: read the second half of the previous paragraph: two structures with common first fields are compatible types. What we have been using is a deliberately supported construct. 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
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
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Nicolas George:
> Nicolas George (12023-01-31):
>>> * it prevents filterlink internals from being visible in a
>>> public header, where they have no business being
>>> * it is a step towards hiding more of lavfi internals from public
>>> headers
>>> * the same pattern is already and ever more widely used in the other
>
> Note to the TC who will decide: I do not oppose the efforts mentioned in
> these two points (that are actually the same points twice), I only
> oppose this particular solution because of this drawback:
>
>> * It requires the developers to remember which field is public and which
>> field is private, which is not something relevant here (is is relevant
>> elsewhere).
>
> Without looking very far, I can think of several different ways of
> hiding the internal fields better without requiring changes to the
> implementation. I would not oppose such a change.
>
Details please. 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.
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.
- Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Nicolas George (12023-01-31): > > * it prevents filterlink internals from being visible in a > > public header, where they have no business being > > * it is a step towards hiding more of lavfi internals from public > > headers > > * the same pattern is already and ever more widely used in the other Note to the TC who will decide: I do not oppose the efforts mentioned in these two points (that are actually the same points twice), I only oppose this particular solution because of this drawback: > * It requires the developers to remember which field is public and which > field is private, which is not something relevant here (is is relevant > elsewhere). Without looking very far, I can think of several different ways of hiding the internal fields better without requiring changes to the implementation. I would not oppose such a change. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-31): > I still see no objective facts supporting your claim of exclusive > maintainership over the entirety of lavfi generic code and public API. The fact is very simple: I am the only one who understand how this code works. > So to avoid any further pointless bickering, I'm hereby asking the TC to > resolve this. Just so we are clear: you are a party in this, you can therefore not be a judge. > To summarize my view, this patch is an improvement because: > * it prevents filterlink internals from being visible in a > public header, where they have no business being > * it is a step towards hiding more of lavfi internals from public > headers > * the same pattern is already and ever more widely used in the other > libraries and ffmpeg CLI > * it is supported by Andreas (who submitted a more general analogue of > this patch over a year ago) and Paul > * I am not aware of anyone other than Nicolas being against it It is a worsening because: * It requires the developers to remember which field is public and which field is private, which is not something relevant here (is is relevant elsewhere). Of course, if you think about it two seconds, you realize it affects the person who knows the code very well and used/wants to work on it intensively more than the developers who move from one part of the code to another and have to re-learn everything. But thinking two seconds how your changes affects other people does not seem like your forte either. Therefore, I add this point: * If this change is applied, FFmpeg needs to find somebody else to maintain the core of libavfilter. (And I predict you will not.) -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Quoting Nicolas George (2023-01-31 15:31:44) > Anton Khirnov (12023-01-31): > > Do you? > > > > You keep implying in your emails that you are the authority on lavfi, > > but I see no objective support for this claim. MAINTAINERS only lists > > you as the maintainer for graphdump.c and two filters. > > And taken e.g. by the number of commits touching libavfilter/, mine is > > similar to yours, and both are far behind other people. > > I know the code, how it works. > > I know how the code needs to evolve to achieve new features without > breaking existing. > > I review and apply patches to fix bugs when they come; fortunately it > does not happen frequently. I still see no objective facts supporting your claim of exclusive maintainership over the entirety of lavfi generic code and public API. Considering yourself the maintainer in your own head is not enough. So to avoid any further pointless bickering, I'm hereby asking the TC to resolve this. To summarize my view, this patch is an improvement because: * it prevents filterlink internals from being visible in a public header, where they have no business being * it is a step towards hiding more of lavfi internals from public headers * the same pattern is already and ever more widely used in the other libraries and ffmpeg CLI * it is supported by Andreas (who submitted a more general analogue of this patch over a year ago) and Paul * I am not aware of anyone other than Nicolas being against it -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
On 1/31/23, Nicolas George wrote: > Paul B Mahol (12023-01-31): >> No, you do not. Calling your libavfilter framework code "complex" is >> disgrace >> to really complex code in non-framework part of libavfilter or else in >> FFmpeg libraries. >> >> libavfilter framework needs serious overhaul. >> It have numerous limitations and other stuff too much exposed to user, >> that really should not be. >> >> You do not maintain code at all, you just block patches and never >> contribute anymore anything useful besides blocking patches. > > … says the person who does not know that the duration between pts=3 and > pts=5 must be 2. > > This is a bad joke. The patch just returned duration of frame from framesync and not actual duration of frame. And I have not tested it extensively at all or even thought about it much. Why? Because I find whole API including libavfilter very silly and broken and limited. At least I contributed numerous filters used in production, otherwise libavfilter would probably not exist today at all in this form at least. It would be already rewritten several times and/or whole idea abandoned completely because of lack of interest to develop it or poor usage. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Paul B Mahol (12023-01-31): > No, you do not. Calling your libavfilter framework code "complex" is disgrace > to really complex code in non-framework part of libavfilter or else in > FFmpeg libraries. > > libavfilter framework needs serious overhaul. > It have numerous limitations and other stuff too much exposed to user, > that really should not be. > > You do not maintain code at all, you just block patches and never > contribute anymore anything useful besides blocking patches. … says the person who does not know that the duration between pts=3 and pts=5 must be 2. This is a bad joke. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-31): > Do you? > > You keep implying in your emails that you are the authority on lavfi, > but I see no objective support for this claim. MAINTAINERS only lists > you as the maintainer for graphdump.c and two filters. > And taken e.g. by the number of commits touching libavfilter/, mine is > similar to yours, and both are far behind other people. I know the code, how it works. I know how the code needs to evolve to achieve new features without breaking existing. I review and apply patches to fix bugs when they come; fortunately it does not happen frequently. > Maybe someone should replace those tricky bits with something simpler > then. Yes, do that, and I will laugh. The code is tricky because the needs are tricky. “Something simpler” is precisely what came from libav and I needed to fix a decade ago. > Ah yes, preventing one person from enforcing his opinion over those of > multiple other people is "authoritarianism". I don't think that word > means what you think it means. > > So take your pick - either stop opposing this patch or we have a vote > over it and see what other people think. Go ahead, have a vote. I no longer care. If it goes against me, that will make one less thing to consider my responsibility. We should never have asked ourselves “how should we change to entice forkers back”, we should have asked “how must the forkers change if they want to be accepted back”. You did not change. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Quoting Nicolas George (2023-01-31 15:03:34) > Anton Khirnov (12023-01-31): > > I find this a significant improvement in code quality, making it easier > > to maintain. > > You can say that, you do not maintain it. Do you? You keep implying in your emails that you are the authority on lavfi, but I see no objective support for this claim. MAINTAINERS only lists you as the maintainer for graphdump.c and two filters. And taken e.g. by the number of commits touching libavfilter/, mine is similar to yours, and both are far behind other people. > > - the most prolific lavfi contributor recently (Paul) has no objections > > to this patch > > - the second most prolific lavfi contributor recently (Andreas) told me > > on IRC he planned to write this same patch himself > > It is not a matter of volume, it is a matter of complexity. Since > Stefano is no longer involved in the coding, I am the only one who knows > how the tricky bits of libavfilter work. Maybe someone should replace those tricky bits with something simpler then. > > So if you insist on objecting to this patch, it seems to me that a vote > > would be in order. > > Well, go ahead, but please be aware that a vote cannot force me to > maintain that code. > > If this project goes over my objections “that patch makes my maintenance > work harder”, then be very careful to involve in your plans “find > somebody else willing to debug the code of libavfilter”. Good luck. > > In fact, I think I will take it very easy with maintenance duty until > further notice. I am really fed up with the direction this project is > taking. Authoritarianism, that was the other side of the fork, and we > all know what happened there. And who is to blame. Ah yes, preventing one person from enforcing his opinion over those of multiple other people is "authoritarianism". I don't think that word means what you think it means. So take your pick - either stop opposing this patch or we have a vote over it and see what other people think. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
On 1/31/23, Nicolas George wrote: > Anton Khirnov (12023-01-31): >> I find this a significant improvement in code quality, making it easier >> to maintain. > > You can say that, you do not maintain it. > >> Making it obvious which field is private and which is public is a >> feature, not a bug. > > Then I do not want this feature. Making people expend effort for no > reason at all. > >> I'll also note that >> - we've been switching to this precise pattern everywhere else in the >> project > > Well, too bad. > >> - the most prolific lavfi contributor recently (Paul) has no objections >> to this patch >> - the second most prolific lavfi contributor recently (Andreas) told me >> on IRC he planned to write this same patch himself > > It is not a matter of volume, it is a matter of complexity. Since > Stefano is no longer involved in the coding, I am the only one who knows > how the tricky bits of libavfilter work. No, you do not. Calling your libavfilter framework code "complex" is disgrace to really complex code in non-framework part of libavfilter or else in FFmpeg libraries. libavfilter framework needs serious overhaul. It have numerous limitations and other stuff too much exposed to user, that really should not be. You do not maintain code at all, you just block patches and never contribute anymore anything useful besides blocking patches. > >> So if you insist on objecting to this patch, it seems to me that a vote >> would be in order. > > Well, go ahead, but please be aware that a vote cannot force me to > maintain that code. > > If this project goes over my objections “that patch makes my maintenance > work harder”, then be very careful to involve in your plans “find > somebody else willing to debug the code of libavfilter”. Good luck. > > In fact, I think I will take it very easy with maintenance duty until > further notice. I am really fed up with the direction this project is > taking. Authoritarianism, that was the other side of the fork, and we > all know what happened there. And who is to blame. > > -- > 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-requ...@ffmpeg.org with subject "unsubscribe". > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-31): > I find this a significant improvement in code quality, making it easier > to maintain. You can say that, you do not maintain it. > Making it obvious which field is private and which is public is a > feature, not a bug. Then I do not want this feature. Making people expend effort for no reason at all. > I'll also note that > - we've been switching to this precise pattern everywhere else in the > project Well, too bad. > - the most prolific lavfi contributor recently (Paul) has no objections > to this patch > - the second most prolific lavfi contributor recently (Andreas) told me > on IRC he planned to write this same patch himself It is not a matter of volume, it is a matter of complexity. Since Stefano is no longer involved in the coding, I am the only one who knows how the tricky bits of libavfilter work. > So if you insist on objecting to this patch, it seems to me that a vote > would be in order. Well, go ahead, but please be aware that a vote cannot force me to maintain that code. If this project goes over my objections “that patch makes my maintenance work harder”, then be very careful to involve in your plans “find somebody else willing to debug the code of libavfilter”. Good luck. In fact, I think I will take it very easy with maintenance duty until further notice. I am really fed up with the direction this project is taking. Authoritarianism, that was the other side of the fork, and we all know what happened there. And who is to blame. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Quoting Nicolas George (2023-01-30 13:40:06) > Anton Khirnov (12023-01-30): > > This hack is used to limit the visibility of some AVFilterLink fields to > > only certain files. Replace it with the same pattern that is used e.g. > > in lavf AVStream/FFstream and avoid exposing these internal fields in a > > public header completely. > > --- > > libavfilter/avfilter.c | 191 +-- > > libavfilter/avfilter.h | 45 - > > libavfilter/avfiltergraph.c | 9 +- > > libavfilter/buffersink.c | 8 +- > > libavfilter/link_internal.h | 69 + > > libavfilter/tests/filtfmts.c | 9 +- > > 6 files changed, 196 insertions(+), 135 deletions(-) > > create mode 100644 libavfilter/link_internal.h > > This makes the code more verbose, less readable and harder to maintain, I find this a significant improvement in code quality, making it easier to maintain. > so no thanks. > > If you find a solution that does not require us to remember which field > is private and with field is public to prefix them with link-> or li->, > it would not have this issue; avoiding this requirement was a prime goal > of the current implementation. At least you did not add an indirection > like on some other places. Making it obvious which field is private and which is public is a feature, not a bug. I'll also note that - we've been switching to this precise pattern everywhere else in the project - the most prolific lavfi contributor recently (Paul) has no objections to this patch - the second most prolific lavfi contributor recently (Andreas) told me on IRC he planned to write this same patch himself So if you insist on objecting to this patch, it seems to me that a vote would be in order. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-30): > This hack is used to limit the visibility of some AVFilterLink fields to > only certain files. Replace it with the same pattern that is used e.g. > in lavf AVStream/FFstream and avoid exposing these internal fields in a > public header completely. > --- > libavfilter/avfilter.c | 191 +-- > libavfilter/avfilter.h | 45 - > libavfilter/avfiltergraph.c | 9 +- > libavfilter/buffersink.c | 8 +- > libavfilter/link_internal.h | 69 + > libavfilter/tests/filtfmts.c | 9 +- > 6 files changed, 196 insertions(+), 135 deletions(-) > create mode 100644 libavfilter/link_internal.h This makes the code more verbose, less readable and harder to maintain, so no thanks. If you find a solution that does not require us to remember which field is private and with field is public to prefix them with link-> or li->, it would not have this issue; avoiding this requirement was a prime goal of the current implementation. At least you did not add an indirection like on some other places. -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
On 1/30/23, Anton Khirnov wrote: > This hack is used to limit the visibility of some AVFilterLink fields to > only certain files. Replace it with the same pattern that is used e.g. > in lavf AVStream/FFstream and avoid exposing these internal fields in a > public header completely. Looks fine on quick look, but wait for Nicolas comment. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
This hack is used to limit the visibility of some AVFilterLink fields to
only certain files. Replace it with the same pattern that is used e.g.
in lavf AVStream/FFstream and avoid exposing these internal fields in a
public header completely.
---
libavfilter/avfilter.c | 191 +--
libavfilter/avfilter.h | 45 -
libavfilter/avfiltergraph.c | 9 +-
libavfilter/buffersink.c | 8 +-
libavfilter/link_internal.h | 69 +
libavfilter/tests/filtfmts.c | 9 +-
6 files changed, 196 insertions(+), 135 deletions(-)
create mode 100644 libavfilter/link_internal.h
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index c2ecdffa6f5..2a1f99bd656 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -34,15 +34,14 @@
#include "libavutil/rational.h"
#include "libavutil/samplefmt.h"
-#define FF_INTERNAL_FIELDS 1
-#include "framequeue.h"
-
#include "audio.h"
#include "avfilter.h"
#include "filters.h"
#include "formats.h"
+#include "framequeue.h"
#include "framepool.h"
#include "internal.h"
+#include "link_internal.h"
static void tlog_ref(void *ctx, AVFrame *ref, int end)
{
@@ -148,6 +147,7 @@ int ff_append_outpad_free_name(AVFilterContext *f,
AVFilterPad *p)
int avfilter_link(AVFilterContext *src, unsigned srcpad,
AVFilterContext *dst, unsigned dstpad)
{
+FilterLinkInternal *li;
AVFilterLink *link;
av_assert0(src->graph);
@@ -166,9 +166,10 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
return AVERROR(EINVAL);
}
-link = av_mallocz(sizeof(*link));
-if (!link)
+li = av_mallocz(sizeof(*li));
+if (!li)
return AVERROR(ENOMEM);
+link = >l;
src->outputs[srcpad] = dst->inputs[dstpad] = link;
@@ -179,17 +180,20 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
link->type= src->output_pads[srcpad].type;
av_assert0(AV_PIX_FMT_NONE == -1 && AV_SAMPLE_FMT_NONE == -1);
link->format = -1;
-ff_framequeue_init(>fifo, >graph->internal->frame_queues);
+ff_framequeue_init(>fifo, >graph->internal->frame_queues);
return 0;
}
void avfilter_link_free(AVFilterLink **link)
{
+FilterLinkInternal *li;
+
if (!*link)
return;
+li = ff_link_internal(*link);
-ff_framequeue_free(&(*link)->fifo);
+ff_framequeue_free(>fifo);
ff_frame_pool_uninit((FFFramePool**)&(*link)->frame_pool);
av_channel_layout_uninit(&(*link)->ch_layout);
@@ -209,29 +213,35 @@ static void filter_unblock(AVFilterContext *filter)
{
unsigned i;
-for (i = 0; i < filter->nb_outputs; i++)
-filter->outputs[i]->frame_blocked_in = 0;
+for (i = 0; i < filter->nb_outputs; i++) {
+FilterLinkInternal * const li = ff_link_internal(filter->outputs[i]);
+li->frame_blocked_in = 0;
+}
}
void ff_avfilter_link_set_in_status(AVFilterLink *link, int status, int64_t
pts)
{
-if (link->status_in == status)
+FilterLinkInternal * const li = ff_link_internal(link);
+
+if (li->status_in == status)
return;
-av_assert0(!link->status_in);
-link->status_in = status;
-link->status_in_pts = pts;
+av_assert0(!li->status_in);
+li->status_in = status;
+li->status_in_pts = pts;
link->frame_wanted_out = 0;
-link->frame_blocked_in = 0;
+li->frame_blocked_in = 0;
filter_unblock(link->dst);
ff_filter_set_ready(link->dst, 200);
}
void ff_avfilter_link_set_out_status(AVFilterLink *link, int status, int64_t
pts)
{
+FilterLinkInternal * const li = ff_link_internal(link);
+
av_assert0(!link->frame_wanted_out);
-av_assert0(!link->status_out);
-link->status_out = status;
+av_assert0(!li->status_out);
+li->status_out = status;
if (pts != AV_NOPTS_VALUE)
ff_update_link_current_pts(link, pts);
filter_unblock(link->dst);
@@ -409,13 +419,15 @@ void ff_tlog_link(void *ctx, AVFilterLink *link, int end)
int ff_request_frame(AVFilterLink *link)
{
+FilterLinkInternal * const li = ff_link_internal(link);
+
FF_TPRINTF_START(NULL, request_frame); ff_tlog_link(NULL, link, 1);
av_assert1(!link->dst->filter->activate);
-if (link->status_out)
-return link->status_out;
-if (link->status_in) {
-if (ff_framequeue_queued_frames(>fifo)) {
+if (li->status_out)
+return li->status_out;
+if (li->status_in) {
+if (ff_framequeue_queued_frames(>fifo)) {
av_assert1(!link->frame_wanted_out);
av_assert1(link->dst->ready >= 300);
return 0;
@@ -423,8 +435,8 @@ int ff_request_frame(AVFilterLink *link)
/* Acknowledge status change. Filters using ff_request_frame() will
handle the change automatically. Filters can also check the
status directly but none do yet. */
-ff_avfilter_link_set_out_status(link, link->status_in,
