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

[wm4]

On Tue, 10 May 2016 22:21:12 +0200 (CEST)
Marton Balint <cus at passwd.hu> wrote:

> On Tue, 10 May 2016, Michael Niedermayer wrote:
> 
> > On Tue, May 10, 2016 at 03:53:31PM +0200, wm4 wrote:  
> >> On Tue, 10 May 2016 14:35:02 +0200
> >> 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)
> >>>
> >>> also let me steal the text from wikipedia about the
> >>> Principle_of_least_astonishment, as its IMO well written  
> >>
> >> Indeed, av_opt calls on structs which don't support it compile without
> >> warning, but mysteriously crash at runtime. That's a violation of this
> >> principle. And no, it doesn't mean that every struct should have an  
> >  
> >> AVClass, because that would be insane.  
> >
> > iam not seriously suggesting to add it to every public struct but
> > playing devils advocate here why is that insane ?
> > it would actually solve the problem and be consistent  
> 
> I also don't see why adding an AVClass for most public structs is so evil. 
> Apart from the crash issue above, having an av_opt support for struct 
> members is definitely good, only way to have that conveniently at the 
> moment is to use AVClass.
> 
> Specifying the class separately from the object for each 
> av_opt call would also be a bit ugly, so even I agree it is not 
> perfect, I don't see a better way, and we already have AVClasses for some 
> structs...
> 
> Does libav have any plans for AVClass?

Actually, this whole av_opt business if the evil part, not necessarily
the AVClass thing. Why should av_opt essentially duplicate the API? The
only real argument I can find is because ffmpeg.c needs it for command
line parsing (!).

FFmpeg is a set of C libraries. It's not a scripting wrapper. It
doesn't need to provide dynamic access to API that is already defined
by public fields.

Especially not for something like AVCodecParameters, which is
essentially a value type (plus an allocator function for ABI reasons).

AVOption usage might be slightly more understandable for cases where
options need to be dynamically passed down to sub-components, like
nested demuxers. Although that is crazy too.