[FFmpeg-devel] [PATCH] ffprobe integration

Fri Feb 12 04:02:55 CET 2010

On Fri, Feb 12, 2010 at 01:01:28AM +0100, Stefano Sabatini wrote:
> On date Wednesday 2010-02-10 15:56:10 +0100, Michael Niedermayer encoded:
> > On Tue, Feb 09, 2010 at 02:08:47AM +0100, Stefano Sabatini wrote:
> > > On date Monday 2010-02-08 15:39:11 +0100, Michael Niedermayer encoded:
> > > > On Mon, Feb 08, 2010 at 01:01:59AM +0100, Stefano Sabatini wrote:
> > > [...]
> > > > > Also once given the get_flags function
> > > > > this requires also less code (just one line and the flags definitions)
> > > > > / options (just one compared to N).
> > > > 
> > > > huh?
> > > > you need 2 lines for what i suggest
> > > > static int print_unit;
> > > > and a single line in the cmdline parsing array that contains the string,
> > > > help text, default value, ...
> > > > 
> > > > your code does NOT work with less, you need at least the enum and
> > > > flag_descriptor
> > > > but its much messier, has no default, no help text
> > > 
> > > So what about to use opt.c? This has all the features you're asking
> > > for, and the flexibility I aim to.
> > 
> > All applications we have use cmdutils.c/h
> > You did not explain why this would be unflexible
> 
> We do have two different options system, I always toyed with the idea
> to start to use just one.
> 
> OptionDef is inflexible as it doesn't allow for example to set flags,
> for other aspects it is more flexible as it allows to call a function
> for setting a value.
> 
> Having two different option systems doesn't make much sense to me.

They serve different purposes, one is to parse command line options
for an application.
The other is to allow "high level" access including enumeration and all
that of structure fields.
Thats not the same thing, and iam not against considering patch propoals
that would drop one and make the other able to handle its case but
thes MUST decrease the # of lines because otherwise its not a simplification.
and it must be clean and not loose features,...
anyway until this happens you cant just use the "wrong" system as each is
lacking features for the others puprose.

> 
> > [...]
> > > +typedef struct {
> > > +    const AVClass *class;
> > > +    int show_flags;
> > > +    int format_flags;
> > > +} FFprobeContext;
> > > +
> > > +#define FORMAT_USE_UNIT                      0x0001 ///< Print unit of measure.
> > > +#define FORMAT_USE_PREFIX                    0x0002 ///< Print SI (binary or decimal) prefixes.
> > > +#define FORMAT_USE_BINARY_PREFIX             0x0004 ///< Use binary type prefixes, implies the use of prefixes.
> > > +#define FORMAT_USE_BYTE_BINARY_PREFIX        0x0008 ///< Force the use of binary prefixes with bytes measure units.
> > > +#define FORMAT_USE_SEXAGESIMAL_TIME_FORMAT   0x0010 ///< Use sexagesimal time format HH:MM:SS.MICROSECONDS.
> > 
> > please use seperate variables
> 
> All value formats are related, so it makes sense to set them just with
> a flag,

they should be int or if you insist uint8_t
flags are limited to 32, are a mess, more error prone, lead to bigger
code and much uglier on the command line.
Now please forget these flags i wont approve them.

> also I'd like to have the functions which use them as general
> as possible, 

port them to java

> I want to avoid to access to the global context directly
> so the obvious choice looks like storing all these flags in a common
> variable and pass that.

Why do you want to avoid that?
and they made structs for this (iff there is a real reason to go this
way but iam against it without good reason)

> 
> ffprobe -value_string_unit -value_string_binary_prefix -value_string_byte_binary_prefix ...
> or
> ffprobe -value_string unit+binary_prefix+byte_bynary_prefix
> 
> Which is more compact / clear?

ffprobe -binary_prefix
or
ffprobe -si_prefix
or
ffprobe -prefix si
ffprobe -si
...

> 
> Not to mention the trick you can do with flags, e.g. -value_string all-unit+pretty

or sin(pi*3)

> 
> > [...]
> > 
> > > +typedef enum UnitId {
> > > +    UNIT_NONE,
> > > +    UNIT_SECOND,
> > > +    UNIT_HERTZ,
> > > +    UNIT_BIT,
> > > +    UNIT_BYTE,
> > > +    UNIT_BIT_PER_SECOND,
> > > +    UNIT_BYTE_PER_SECOND,
> > > +    UNIT_NB,
> > > +} UnitId;
> > > +
> > > +const char *unit_strings[] = {
> > > +    [UNIT_NONE           ] = ""      ,
> > > +    [UNIT_SECOND         ] = "s"     ,
> > > +    [UNIT_HERTZ          ] = "Hz"    ,
> > > +    [UNIT_BIT            ] = "bit"   ,
> > > +    [UNIT_BYTE           ] = "byte"  ,
> > > +    [UNIT_BIT_PER_SECOND ] = "bit/s" ,
> > > +    [UNIT_BYTE_PER_SECOND] = "byte/s",
> > > +};
> > 
> > Maybe you should consider hosting ffprobe somewhere else?
> > ffmpeg code must be clean and simple
> > using enums & tables to obfuscate a "%d Hz" is not something i can approve
> 
> So would you suggest to remove the pixel formats and use instead
> PIX_FMT_YUV420P_STR
> PIX_FMT_MONOW_STR
> ...
> ?

no, we use the appropriate system for each case. pixel formats need to
be fast, we need switch/case and so on for them
none of this is needed in your case you just set them and print them

> 
> Anyway I don't mind removing the enum, but it will require a strcmp in
> a function used *much*:

much = 2 ?
besides we will see if you really need these 2 ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100212/0c523016/attachment.pgp>