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

[Hendrik Leppkes]

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

- Hendrik