[FFmpeg-devel] [PATCH] Move stream_options to avformat

Sun Jan 25 18:22:02 CET 2015

On Sun, Jan 25, 2015 at 05:24:18PM +0100, wm4 wrote:
> On Sun, 25 Jan 2015 16:18:53 +0000
> Paul B Mahol <onemda at gmail.com> wrote:
> 
> > On 1/25/15, wm4 <nfxjfg at googlemail.com> wrote:
> > > On Sun, 25 Jan 2015 13:39:10 +0100
> > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > >
> > >> On Sun, Jan 25, 2015 at 01:18:31PM +0100, Michael Niedermayer wrote:
> > >> > On Sun, Jan 25, 2015 at 12:15:40PM +0100, Reimar Doeffinger wrote:
> > >> > > On 25.01.2015, at 03:08, Michael Niedermayer <michaelni at gmx.at> wrote:
> > >> > > > On Sun, Jan 25, 2015 at 02:31:33AM +0100, wm4 wrote:
> > >> > > >>
> > >> > > >>>>
> > >> > > >>>> As an experienced API user, I don't have the slightest clue what
> > >> > > >>>> I'd do
> > >> > > >>>> with this API, or where to find information about it.
> > >> > > >>>
> > >> > > >>> the primary goal is to remove duplicated disposition type tables,
> > >> > > >>> which needs one of the tables to be public first
> > >> > > >>>
> > >> > > >>> [...]
> > >> > > >>
> > >> > > >> And this is the most awkward way you could find to do this?
> > >> > > >
> > >> > > > No, i could certainly find a more akward way, if people prefer
> > >> > > >
> > >> > > > this is just the way that would be a big step towards consistent
> > >> > > > and simple access to the structs
> > >> > > > All public structs use AVClass and AVOptions to allow applications
> > >> > > > to extract/enumerate fields except a few like AVStream
> > >> > > > this patch would add these AVClass & AVOption for AVStream, its
> > >> > > > indeed not populated for all fields and AVStream doesnt have a
> > >> > > > AVClass as its first field due to ABI. But its a step toward it
> > >> > > >
> > >> > > > Would people prefer that each field in AVStream has a custom and
> > >> > > > different way to access it, as long as it looks simpler when looked
> > >> > > > at in isolation ?
> > >> > >
> > >> > > Sorry if it's useless of me to only state some obvious questions, but:
> > >> > > I think it's clear we all want a simple, obvious and consistent API :)
> > >> > > If it's a bit messy, might there be a point in holding off a bit so we
> > >> > > aren't stuck with something complicated?
> > >> > > Could possibly another approach after a major bump be nicer?
> > >> > > Or maybe better documentation/examples?
> > >> >
> > >> > > I think this started with a valid complaint/concern but unfortunately
> > >> > > no better alternative, could we stick to considering that instead of
> > >> > > going over to agressive rhethoric?
> > >> >
> > >> > absolutley
> > >> > i would strongly prefer if others could take this over, my interrest
> > >> > was just in the technical side and i wanted to move AVStream to
> > >> > the same system we use for all other structs. As well as fixing the
> > >> > quite valid issue nicolas had raised with the duplicated tables.
> > >> > I am quite surprised that others dont see this as a clear and
> > >> > uncontroversal step, there really are just
> > >> > 1. If we want AVStream to be consistent with other structs, that means
> > >> >    AVOption & AVClass. And this patch is a step toward it, one could
> > >> >    make a bigger or smaller step but its then either more or less
> > >> >    code not different code.
> > >> > 2. There could be a different system be used for this field or for
> > >> >    AVStream, this would be inconsistent
> > >> > 3. We can implement both a system based on AVOptions/AVClass and a
> > >> >    system without them, why would this field that noone cared about
> > >> >    until now need this, iam not sure though
> > >> > 4. We can leave the triplicated tables as is and hope not to forget
> > >> >    updating them in sync
> > >> >
> > >> > To me the best choice is clear, move toward the same system we use
> > >> > elsewhere. Change that system everywhere if it could be improved
> > >> > I see nothing controversal on this patch but others do apparently.
> > >> > As i dont see what issue people have with this, i certainly cannot
> > >> > help fixing the patch. But iam happy to review & approve the solution
> > >> > that people do prefer
> > >>
> > >> About the documentation & example side, i dont think this should yet
> > >> be used from outside, its only a partial implementation of AVOption
> > >> for AVStream, a full implementation needs a ABI bump due to the
> > >> first field needing to be a AVClass
> > >>
> > >> [...]
> > >>
> > >
> > > How is it even consistent with "other structs"?

The only public change the patch makes is adding
avstream_get_class()

git grep '_get_class' */*.h
libavcodec/avcodec.h:const AVClass *avcodec_get_class(void);
libavcodec/avdct.h:const AVClass *avcodec_dct_get_class(void);
libavfilter/avfilter.h:const AVClass *avfilter_get_class(void);
libavformat/avformat.h: * from avformat_get_class()). Private (format-specific) options are provided by
libavformat/avformat.h:const AVClass *avformat_get_class(void);
libavresample/avresample.h:const AVClass *avresample_get_class(void);
libswresample/swresample.h:const AVClass *swr_get_class(void);
libswscale/swscale.h:const AVClass *sws_get_class(void);

thats just what is there for other AVOption/AVClass structs as well

> > > Doesn't it just resolve
> > > flags?

It gives you the AVClass describing the struct
that includes its name, instance name, AVOption array with types
names, help text, default, range, ...
ATM only the disposition field is in the table, nothing prevents
more fields from being added to the table

> > > Resolving flags with a complicated AVOption contraption (which
> > > every user has to understand and duplicate) doesn't seem like a good
> > > choice to me at all. I hear about API users fighting with the basics of
> > > the FFmpeg API because it's so weird and complicated; seeing patches
> > > like this just feel like a bad joke in contrast.
> > >
> > > What's wrong with:
> > >
> > >     int av_parse_disposition_flags(const char *s);
> > >
> > > ?
> > 
> > How than one can know which flags are available?
> 
> Well yes, C doesn't have reflection, but I doubt AVOption is a good
> replacement for that.

> But you still could fall back to awkward things
> like messing with AVClasses if you really need.

that would require the patch that started this thread because it
adds exactly that AVClass

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150125/1c09bf24/attachment.asc>