[FFmpeg-devel] [PATCH] avcodec: Add AVClass to AVCodecParameters

[Paul B Mahol]

On 5/10/16, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Tue, May 10, 2016 at 2:35 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> On Tue, May 10, 2016 at 02:02:52PM +0200, Hendrik Leppkes wrote:
>>> On Tue, May 10, 2016 at 1:43 PM, Michael Niedermayer
>>> <michael at niedermayer.cc> wrote:
>>> > On Tue, May 10, 2016 at 11:17:12AM +0100, Derek Buitenhuis wrote:
>>> >> On 5/10/2016 7:24 AM, Hendrik Leppkes wrote:
>>> >> > I don't like this, the struct is pretty cleanly defined and unlikely
>>> >> > to be extended much over time.
>>> >> > Most other structs have AVOptions so the CLI can interact with it,
>>> >> > but
>>> >> > this struct is not meant to be modified by users, its just a direct
>>> >> > line of communication between demuxer->decoder or encoder->muxer.
>>> >>
>>> >> To me this mostly still feels like trying to make a demuxing library
>>> >> something it isn't. It demuxes and muxes.
>>> >>
>>> >> You can argue that it should be easier than decoding a frame to get
>>> >> this
>>> >> info (which I don't personally think is much trouble at all). However,
>>> >> jamming it into libavformat for reasons like "we've always done this"
>>> >> and
>>> >> "there's no better place" is not good design IMO.
>>> >>
>>> >> It sounds like what you really want is a higher level "easy" API.
>>> >> Don't
>>> >> jam it in to existing decoupled APIs because it is convenient, IMO.
>>> >> That's
>>> >> how horrible things like having ffmpeg.c functionality in libavformat
>>> >> came
>>> >> to exist, and a decade of misery followed.
>>> >
>>> > This patch adds an AVClass field to AVCodecParameters like we use in
>>> > other public structures.
>>> > i fail to see how this patch is related to your reply
>>> >
>>> > What the patch does, is it makes the API more consistent and easy to
>>> > use. Users call the AVOption functions to set fields in the main
>>> > public structures, if they do that for AVCodecParameters it would
>>> > crash.
>>> > Even an empty AVClass that doesnt list options would reduce that
>>> > misery by at least not crashing but giving a clear error message.
>>> >
>>>
>>> We should not design our API around people not caring to read our API,
>>> because thats not a fight we can ever win.
>>
>> APIs should be designed with the "Principle of least astonishment"
>>
>> its quite surprising that AVOption APIs work with most public API
>> structures but crash with some
>> (AVPacket is another and i am of the oppinon and stated that previously
>>  IIRC that it too should have a AVClass as its first element, this is
>>  just hard to add as it requires ABI bumping everything which is why
>>  i didnt push much for that being done, it needs to be done at a
>>  time we bump all stuff for more important reasons)
>>
>
> The number of structs that don't have it probably greatly outnumber
> those that do.
> From the top of my head:
>
> AVFrame and AVSubtitle
> AVPacket
> AVStream
> AVCodecDescriptor
> AVCodecParameters, of course
> Any and all structs used for side-data (AVCPBProperties, AVPanScan, etc)
>
> In fact the only that do have it are those where it was originally
> used to access private options of the underlying components.
> AVFilterContext / AVFilterGraph
> AVCodecContext
> AVFormatContext
>
> So I'm not sure I can follow your argument of "everything else has it".

Everything should have it, it just takes few bytes ;-)