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

[Michael Niedermayer]

On Tue, May 10, 2016 at 10:40:50PM +0200, wm4 wrote:
> 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.

ill post a patch that just adds the AVClass

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160517/9402474f/attachment.sig>