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

[Nicolas George]

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

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

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

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

Regards,

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