[FFmpeg-devel] [PATCH 1/6] lavfi/buffersink: add accessors for the stream properties.

[wm4]

On Mon, 19 Dec 2016 09:40:41 +0100
Nicolas George <george at nsup.org> wrote:

> L'octidi 28 frimaire, an CCXXV, wm4 a écrit :
> > For buffersink, you could simply return a struct with the parameters.
> > As a value type, it'd be a copy and wouldn't need accessors.  
> 
> You mean a single structure returned by a single accessor with all the
> stream's properties instead of individual accessors for individual
> properties? Well, the naive methods of returning a structure to the
> caller would make the size of the structure part of the API, but there
> are ways around it.
> 
> I do not dislike the idea, if that is what people prefer.
> 
> (AVCodecParameter would have been an obvious candidate for that, but it
> lacks a few necessary fields. And it has a lot of extra fields, which
> goes against what you write below.)
> 
> 
> > Since you moved the discussion to general API issues...
> > 
> > Using public fields and such "internal" structs is functionally
> > equivalent to having an opaque struct with trivial accessors. It's
> > basically a style issue.  
> 
> This is true, but "functionally equivalent" is a very myopic criterion.
> It misses all the impact of the design on the code readability. Just
> look at all the "->inputs[0]" that this patch allows to eliminate (and
> that James missed, I think).

In general, replacing public fields with macro-generated accessors is
really just a rename. Admittedly, it removes a confusing indirection
(though ->inputs[0]) in this case, but in general the improvements are
very minor. What does it matter if whether there are 100 fields or 100
set/get functions? (Didn't check whether AVCodecContext really has 100
fields, but it wouldn't surprise me.)

> > The difference between them is essentially syntax only. Both of them
> > have multiple disadvantages:
> > - too much to deal with at once (whether it's fields or
> >   setters/getters), no logical separation of functionality that is
> >   lesser needed or doesn't make sense in some contexts. (Consider all
> >   these AVCodecContext fields for tuning encoders - what do they do for
> >   decoding? What do fields, which are audio-only, do if video is
> >   encoded or decoded?)  
> 
> It feels more a mater of documentation and auto-documentation than
> anything else. "There is a crf field, does it apply to the x262 encder?"
> and "There is a crf option, does it apply to the x262 encoder?" are
> exactly the same question, the only difference being that options are
> auto-documenting and answer the question automatically. But we could
> have found a way for fields as well.

AVOptions are unfortunately a good example how to create
encoder-specific options without "polluting" the common API. I don't
quite like AVOptions though, because they're too "fuzzy" for a C API
(av_opt essentially emulates dynamic/weak typing to some degree), and
they are typically even less well documented and defined like the C
API.

I don't really have a good idea how options specific to single codecs
or a small number of codecs should be handled. In some cases, side-data
is a good mechanism to move overly specific use-cases out of the common
API, especially for decoding.

If you look how Microsoft handles this, you'll see that they put
additional encoder controls into separate COM interfaces, which can be
not present in simpler encoders.

> > - it's unclear when these fields can be set or not. (Why _can_ you set
> >   a field if the graph is already "configured"? What happens then? How
> >   is the user supposed to know when it's ok to set them?)
> > - even if you argue that the previous point can be fixed by having the
> >   setters check the state and return an error value on misuse, it's
> >   complex for both API implementer and user  
> 
> All considered, the complexity is in the task: video encoding is an
> incredibly complex subject. API can only do so much to ease it.
> 
> > - many uses of this hide internal fields from the public API user,
> >   which is good. But at the same time, this moves away from the
> >   (probably worthy) goal of allowing outside implementation of
> >   codecs, (de)muxers, filters.  
> 
> This calls to my recent answer to Michael about making AVFilterLink and
> AVFilterPad opaque: allowing foreign modules requires stability for APIs
> that are currently internal, and make development that much harder.

Well, I agree that neither filters nor codecs are in a state where we
could easily allow foreign modules. This will require some work.

> > So I would suggest that instead of changing the whole API to use
> > accessors, we should make the API more "transactional", and force
> > correct use by API design.  
> 
> Unfortunately, if doing that was simple, we would already be doing it.

I'm just saying we're not doing it even where we could. I would claim
that the right mindset is just not there.