[FFmpeg-devel,08/21] fftools/ffmpeg_filter: add filtergraph private data

Start by moving OutputStream.filtered_frame to it, which really belongs
to the filtergraph rather than the output stream.
---
 fftools/ffmpeg.h          |  1 -
 fftools/ffmpeg_filter.c   | 31 ++++++++++++++++++++++++++-----
 fftools/ffmpeg_mux.c      |  1 -
 fftools/ffmpeg_mux_init.c |  4 ----
 4 files changed, 26 insertions(+), 11 deletions(-)

Anton Khirnov (12023-04-27):
> Start by moving OutputStream.filtered_frame to it, which really belongs
> to the filtergraph rather than the output stream.

Then just move them to OutputStream. This is just code noise for no
practical benefit, since ffmpeg is not a library and does not have to
deal with ABI stability or types declared in private headers.

On 4/28/2023 5:45 AM, Nicolas George wrote:
> Anton Khirnov (12023-04-27):
>> Start by moving OutputStream.filtered_frame to it, which really belongs
>> to the filtergraph rather than the output stream.
> 
> Then just move them to OutputStream. This is just code noise for no
> practical benefit, since ffmpeg is not a library and does not have to
> deal with ABI stability or types declared in private headers.

The longtime goal of this work is to have completely standalone modules 
within the CLI and each of them threaded. So while ffmpeg may not be a 
library, clearly defining what fields should be accessed from "the 
outside" and which shouldn't would have the same effect and benefits as 
if it was one, particularly ensuring no future accidental usage of 
fields that should not be touched. The decoder side for example has no 
need to touch a frame the filtering side is using internally, and more 
so if it could result in races.

There's also the fact Anton is maintaining this code and this helps him 
keep track of the nature of each field, plus the fact this same change 
has been done before elsewhere, so if you're going to argue you don't 
like it because in your opinion it's unnecessary, then that's not enough 
grounds to block it.

James Almer (12023-04-28):
> The longtime goal of this work is to have completely standalone modules
> within the CLI and each of them threaded. So while ffmpeg may not be a
> library, clearly defining what fields should be accessed from "the outside"
> and which shouldn't would have the same effect and benefits as if it was
> one, particularly ensuring no future accidental usage of fields that should
> not be touched. The decoder side for example has no need to touch a frame
> the filtering side is using internally, and more so if it could result in
> races.
> 
> There's also the fact Anton is maintaining this code and this helps him keep
> track of the nature of each field, plus the fact this same change has been
> done before elsewhere, so if you're going to argue you don't like it because
> in your opinion it's unnecessary, then that's not enough grounds to block
> it.

(As Anton repeatedly said he does not accept the usefulness of the role
of maintainer, invoking that last argument feels a little like trying to
have it both ways. But since I do acknowledge the usefulness of
maintainers, I do accept it.)

Thanks. This is slightly more convincing than what was explained until
now. But there are ways of clearly stating which fields are internal and
which fields are ok for outside access without littering the code with
casts, because fgp_from_fg() and co. are exactly that.

And during the work of turning all into threads, opportunities to split
the structure in more logical ways with less code noise will almost
certainly present themselves.

Anyway, I find it sad that in the recent years FFmpeg has more and more
followed “good practices” not because, being good, they bring actual
benefit to the code but just because they're “good practices”. We are
not underskilled Java developers following the rules from the textbook
without understanding their purpose; we are FFmpeg, we try new and
original ways of writing code to make it more efficient and more
elegant. FilterGraphPriv is neither efficient nor elegant.

But if I failed to convince you or others with this mail, I will not
fight this anymore.

Regards,

Nicolas George (12023-04-28):
> And during the work of turning all into threads, opportunities to split
> the structure in more logical ways with less code noise will almost
> certainly present themselves.

I had intended to not reply further on this, but I just had a severe
case of esprit de l'ecalier, so I just add this:

For example, it is entirely possible that a lot of these “private”
fields could become local variables and functions parameters in the
threads. That would be elegant and efficient.

But now we will never know, because they will all be stuffed in a
private context, according to “good practices”, and that will spare the
effort of checking what their exact scope is.

Regards,

Quoting Nicolas George (2023-04-29 10:46:31)
> Nicolas George (12023-04-28):
> > And during the work of turning all into threads, opportunities to split
> > the structure in more logical ways with less code noise will almost
> > certainly present themselves.
> 
> I had intended to not reply further on this, but I just had a severe
> case of esprit de l'ecalier, so I just add this:
> 
> For example, it is entirely possible that a lot of these “private”
> fields could become local variables and functions parameters in the
> threads. That would be elegant and efficient.
> 
> But now we will never know, because they will all be stuffed in a
> private context, according to “good practices”, and that will spare the
> effort of checking what their exact scope is.

That does not follow at all - just because something was moved once does
not mean it cannot be moved again if a better place is found later.

This change is an incremental improvement, I am not claiming it is the
best possible shape this code could ever have. In my experience, it is
much better to have a series of actually performed incremental
improvements, than to bikeshed endlessly about what the perfect final
endstate should be and never get anything done.

Anton Khirnov (12023-04-29):
> That does not follow at all - just because something was moved once does
> not mean it cannot be moved again if a better place is found later.

Good, then you should have no objection to replacing your private
structure with a “/* these fields are private to… */” comment.

On 4/29/2023 6:13 AM, Nicolas George wrote:
> Anton Khirnov (12023-04-29):
>> That does not follow at all - just because something was moved once does
>> not mean it cannot be moved again if a better place is found later.
> 
> Good, then you should have no objection to replacing your private
> structure with a “/* these fields are private to… */” comment.

History has shown that these notifications have had no effect on users, 
and even on this same project's developers. A good example was 
ffserver.c, which accessed an unholy amount of lavf private fields (both 
exposed in public headers and even internal ones), and first_dts from 
AVStream, which was not only accessed by prominent library users (One of 
which refuses to do things right and forces distros to use a patch to 
expose said field on their ffmpeg packages for the sake of supporting 
their application, thus making a pure recompilation from our tree no 
longer a drop-in solution on anyone's system), but also by ffmpeg.c, 
with no developer noticing it to prevent such code being pushed.

So no, i don't support a simple "please, don't touch the shinny and 
enticing object in the table" notifications. If in the future this 
approach here is replaced, it needs to be by something better/cleaner 
that also keeps things local.

James Almer (12023-04-29):
> History has shown that these notifications have had no effect on users, and
> even on this same project's developers. A good example was ffserver.c, which
> accessed an unholy amount of lavf private fields (both exposed in public
> headers and even internal ones), and first_dts from AVStream, which was not
> only accessed by prominent library users (One of which refuses to do things
> right and forces distros to use a patch to expose said field on their ffmpeg
> packages for the sake of supporting their application, thus making a pure
> recompilation from our tree no longer a drop-in solution on anyone's
> system), but also by ffmpeg.c, with no developer noticing it to prevent such
> code being pushed.

Since we are discussing types for the fftools, not librairies, what
people outside the project might do is not an issue

As for abuse of fields private to a part of the code by another part of
the code, I think you are somewhat rewriting history here.

The reason the fftools used to access private fields of the libraries is
not that developers neglected the rules against using them, it is that
no such rule existed when the code was written, they came later.

But if you really think we need to enforce the rule, then there still
are better solutions than using a separate structure and littering the
code with casts.

For example we can apply __attribute__((unavailable)) to the private
fields except in the part of the code where they are allowed.

(The attribute might not be supported on all compilers, but it is plenty
enough that it is supported on the compilers used by the FATE instances
that test submitted patches and the compilers used by some of us.)

This is a MUCH better solution than what is proposed here.

Regards,