[FFmpeg-devel] [PATCH 2/2] lavfi: make filter_frame non-recursive.

[Nicolas George]

Le tridi 23 brumaire, an CCXXV, James Almer a écrit :
>     AVSomethingInternal *internal;

> No technical advantages i can think of compared to your method, but it's
> thoroughly tested, clean, and above all ubiquitous. See for example
> AVFilterInternal and AVCodecInternal.

It has technical DISadvantages, and therefore needs a good reason to
choose. I would really appreciate that objections acknowledge and
discuss them, however minimally. In other words, unless your message
contains a sense that looks like that:

	Compared to yours, my solution has the drawback of <fill in
	here>, but I still think it is better because <fill in here>.

... I consider that the advice was given without having thought about
all the implications and therefore cannot be really valuable.

> As i said, what I'm mostly looking for is consistency across the codebase
> and to reduce ifdeffery.

For consistency, I would prefer removing the indirections where they
already are and replace them with invisible fields.

And I consider this kind of ifdeffery, short and isolated, to be a
non-issue.

And most of all, efficiency should be the primary concern. Aesthetics
comes only second.

> That has proven to be problematic before, between merges adding fields at
> the wrong offset by accident,

IIRC, you are mistaken, we only had that kind of issue about public
fields.

>				or downstream projects ignoring the big
> warnings and accessing them anyway.

Indeed. But what I propose prevents that too.

> AVCodecInternal is explicitly not codec-specific.

I forgot this one.

> Every header is meant to be included once. That's why guards are put in
> place.

No, you have it the wrong way. The guards are there to allow the headers
to be included several times. Without the guards, the compiler would
issue errors for duplicated definitions.

Of course, including the same header twice explicitly would be rather
stupid. But that is forgetting indirect inclusions. A lot of the core
headers end up being included many times like that. The guards are
completely necessary for them.

Furthermore, including useless headers has usually no worse consequences
than a few milliseconds on the build time.

This header is different, though: it cannot, must not, be included
indirectly, and including it uselessly could have minor negative
consequences.

> If this ends up being implemented this way, then yes, please include them.

If you insist about guards, they should be negative, i.e.:

#ifdef AV_PRIVATE_FIELDS_H
# error "private_fields.h must not be included indirectly."
#endif
#define AV_PRIVATE_FIELDS_H

Positive guards would be useless, and useless code is harmful.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161113/e5fdf2c8/attachment.sig>