[FFmpeg-devel] [PATCH] ffprobe integration

[Michael Niedermayer]

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


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


[...]

> +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

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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20100210/330c7091/attachment.pgp>