[FFmpeg-devel] [PATCH] ffprobe integration

Stefano Sabatini stefano.sabatini-lala
Fri Feb 12 01:01:28 CET 2010


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.

> [...]
> > +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, also I'd like to have the functions which use them as general
as possible, 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.

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?

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

> [...]
> 
> > +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
...
?

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

static char *value_string(char *buf, int buf_size, double val,
                          const char *unit, int flags)
{
    int index;
    const char *prefix_string;
    const char *unit_string;

    if (!strcmp(unit, UNIT_BYTE_STR) && flags & FORMAT_USE_BYTE_BINARY_PREFIX)
        flags |= FORMAT_USE_BINARY_PREFIX;

    if (!strcmp(unit, UNIT_SECOND_STR) && flags & FORMAT_USE_SEXAGESIMAL_TIME_FORMAT) {

With an enum these are simply enum comparisons.

Regards.
-- 
FFmpeg = Foolish Fierce Monstrous Pitiless Egregious Gorilla



More information about the ffmpeg-devel mailing list