[FFmpeg-devel] [PATCH] ffprobe integration

[Stefano Sabatini]

On date Friday 2010-02-12 04:02:55 +0100, Michael Niedermayer encoded:
> 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 ...

Dropped opt.c use, flags and enum for units.

Docs updated as well.

Regards.
-- 
FFmpeg = Foolish Faithless Mythic Pitiless Exuberant Gigant
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-ffprobe.patch
Type: text/x-diff
Size: 17157 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100214/da947b3c/attachment.patch>