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

[wm4]

On Sun, 18 Dec 2016 19:32:16 +0100
Nicolas George <george at nsup.org> wrote:

> By "actual internal structs", I suspect you mean something similar to
> this:
> 
> typedef struct AVFormatContext {
>     ...
>     AVFormatInternal *internal;
>     ...
> };
> 
> Introducing these structures was a big mistake. For the reasons, see the
> recent discussion about making filter_frame() non-recursive (short of
> it: it makes the actual code unreadable), plus another discussion I did
> not take part about using options on these structure (short of it: a lot
> of work if even possible).
> 
> I do not intend to extend that mistake in libavfilter. If possible, I
> would rather like to kick it out, but fortunately the current uses are
> very limited.

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.

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.

(The former approach, internal structs, is used and preferred for
AVCodecContext etc. because it has no impact on the API.)

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

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. For example, we could make AVCodecContext
opaque, and provide instantiation functions that take specialized
structs (such as AVCodecParameters) to open the decoder. Making
creation and use of an API would be a good step into improving the API
IMHO. I found myself confused over what fields are always immutable in
an AVHWFramesContext, and which are mutable until "init", and that's an
example of the more classic mixed init/use API concept in ffmpeg.