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

In general and not specific to AVCodecParameters

AVOptions allows an application to enumerate the available options,
their type, range and description for the actually linked libraries
and actually used decoder, encoder, demuxer, muxer.

An application that does encoding using libavcodec can present
the user with a list of options supported by the choosen encoder

it would otherwise have to hardcode what each encoder supports

The list of options can differ depening on the version of the libs and
the build options.

Some applications (i belive firefox is one) also have universal
binaries that are intended to work with a wide range of libavcodec
versions. AVOptions makes that alot easier than using the struct fields
directly

AVOptions also allows API changes like 32->64bit without breaking
applications using it. if an option and the fields after it are only
accessed through AVOptions
Examples where we needed 32->64 changes where durations and bitrates.
Similarly float->double or int->float could be done

With an application using only AVOptions. One only would have to
relink it to a newer libavcodec and that would make any new options
available to it without needing to rebuild the application

I would not call AVOption crazy nor useless

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

You can kill me, but you cannot change the truth.
-------------- 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/20160514/d67d4b3b/attachment.sig>