On Tue, 10 May 2016 22:21:12 +0200 (CEST) Marton Balint <c...@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 <mich...@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 > >>>> <mich...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
