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

[James Almer]

On 11/13/2016 7:54 PM, Nicolas George wrote:
> 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.

Why do you always only accept or consider replies and arguments from other
people when they exclusively fit the narrative you expect them to be? You're
literally telling me what i said is worth nothing because it wasn't, or you
assumed it wasn't, exactly what you wanted.

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

I very much rather stick with a clean, longstanding and proven to work method
that doesn't fill the public headers with ifdeffery, doesn't require extra care
to avoid including a new special kind of volatile header that should only be
used in a specific way, and that doesn't make future merges more a pain in the
ass than they already are.

One indirection is not going to make a difference, even if we catalog it as a
considerable disadvantage for the sake of this discussion. It's not enough of
an argument in favor or replacing it with your design when you weight it with
the above.

And "Aesthetics comes second" many times ends up in spaghetti and nigh
unreadable and maintainable code, so lets try to not go that route if possible.
Ask Ronald about his libswr simd refactor work if you want anecdotal evidence,
or refer to avutil/common.h

> 
>> 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,
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>