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

[James Almer]

On 11/13/2016 6:57 PM, Nicolas George wrote:
> Le tridi 23 brumaire, an CCXXV, James Almer a écrit :
>> I don't really like this method. This kind of ifdeffery is very
>> uncommon and feels dirty. Just look at avutil/common.h to see how bad
>> a public header can get this way.
>> I'd prefer if we can stick with the usual internal opaque structs
>> instead.
> 
> What is the exact solution you would propose instead? Please give a
> thought to the drawbacks of any alternative solution compared to this
> one, including indirection overhead.

The same method used almost everywhere else.

public header:

typedef struct AVSomethingInternal AVSomethingInternal;

typedef struct AVSomething {
    int public_field1;
    int public_field2;
    AVSomethingInternal *internal;
} AVSomething;

AVSomething *av_something_alloc();

private header:

struct AVSomethingInternal {
    int private_field1;
    int private_field2;
};

AVSomething *av_something_alloc() {
    AVSomething *something = malloc(sizeof(*something));
    something->internal    = malloc(sizeof(*something->internal));
    return something;
}

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.
As i said, what I'm mostly looking for is consistency across the codebase
and to reduce ifdeffery.

> 
>> The plan was to remove as many internal-but-not-really fields across
>> the codebase as possible on the next major bump. Ideally, we'd be
>> consistent with whatever method we choose to achieve this, and opaque
>> structs are already being used everywhere, including avfilter.h
> 
> Almost everything in AVFilterLink is internal, but it is still needed
> there, it can not be removed.

That's the other method currently used for internal fields. Left in the
public struct, with a huge message saying they are internal and shouldn't
be accessed.
That has proven to be problematic before, between merges adding fields at
the wrong offset by accident, or downstream projects ignoring the big
warnings and accessing them anyway.

They are the ones planned to be made actually internal in the next bump.

> 
> I think you are confusing with per-codec fields: fields that serve only
> for a few codecs are moved in private structures. But fields that are
> common to most codecs stay in the context.

AVCodecInternal is explicitly not codec-specific.

> 
> To hide private fields from the public API is a different issue
> altogether. There are several ways of doing that, the one I used was the
> one I found that minimizes first overhead, then brittleness, then
> ugliness. But I am open to other suggestions.
> 
>> In any case, this is missing the usual header guards.
> 
> This is intentional: this is not a normal header, it should not be
> included the normal way, and in particular never twice. The absence of
> guards is meant as a hint in that direction.

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

> 
>> This shouldn't happen. Guess it's because of the missing guards?
> 
> I do not mind adding the guards to avoid that, if that it what people
> prefer.

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