[FFmpeg-devel] [PATCH] ffprobe integration

[Stefano Sabatini]

On date Sunday 2010-02-07 21:15:24 +0100, Michael Niedermayer encoded:
> On Sun, Feb 07, 2010 at 03:55:24PM +0100, Stefano Sabatini wrote:
> [...]
> > +const Unit measure_units[UNIT_NB] = {
> > +    [UNIT_NONE]            = { UNIT_NONE           , ""       },
> > +    [UNIT_SECOND]          = { UNIT_SECOND         , "s"      },
> > +    [UNIT_HERTZ]           = { UNIT_HERTZ          , "Hz"     },
> > +    [UNIT_BIT]             = { UNIT_BIT            , "bit"    },
> > +    [UNIT_BYTE]            = { UNIT_BYTE           , "byte"   },
> > +    [UNIT_BYTE_PER_SECOND] = { UNIT_BYTE_PER_SECOND, "byte/s" },
> > +    [UNIT_BIT_PER_SECOND]  = { UNIT_BIT_PER_SECOND , "bit/s"  }
> > +};
> 
> as said this indirection over the table is silly
> please use "byte" "bit" "Hz" ... dont invent a complicated system to
> obfuscate constant strings

Passing a constant to a function rather than a string is both faster
(no need to strcmp) and more robust (less likely to introduce a typo).

> > +
> > +/** Print unit of measure. */
> > +#define VALUE_STRING_USE_UNIT                      0x0001
> > +
> > +/** Print SI (binary or decimal) prefixes. */
> > +#define VALUE_STRING_USE_PREFIX                    0x0002
> > +
> > +/** Use binary type prefixes, implies the use of prefixes */
> > +#define VALUE_STRING_USE_BINARY_PREFIX             0x0004
> > +
> > +/** Force the use of binary prefixes with bytes measure units. */
> > +#define VALUE_STRING_USE_BYTE_BINARY_PREFIX        0x0008
> > +
> > +/* Use sexagesimal time format HH:MM:SS.MICROSECONDS. */
> > +#define VALUE_STRING_USE_SEXAGESIMAL_TIME_FORMAT   0x0010
> > +
> > +struct flag_descriptor value_string_flag_descriptors[] = {
> > +    { "unit"              , VALUE_STRING_USE_UNIT                    },
> > +    { "prefix"            , VALUE_STRING_USE_PREFIX                  },
> > +    { "byte_binary_prefix", VALUE_STRING_USE_BYTE_BINARY_PREFIX      },
> > +    { "sexagesimal"       , VALUE_STRING_USE_SEXAGESIMAL_TIME_FORMAT },
> > +    { "pretty"            , VALUE_STRING_USE_UNIT                    |
> > +                            VALUE_STRING_USE_PREFIX                  |
> > +                            VALUE_STRING_USE_BYTE_BINARY_PREFIX      |
> > +                            VALUE_STRING_USE_SEXAGESIMAL_TIME_FORMAT },
> > +    { NULL }
> > +};
> > +
> > +static int opt_format(const char *opt, const char *arg)
> > +{
> > +    return get_flags(&value_string_flags, value_string_flag_descriptors, arg);
> > +}
> 
> please use
> static int print_unit;
> static int use_prefix;
> ...
> 
> this is MUCH simpler

Yes but also less flexible. 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).

> [...]
> 
> > +
> > +static void show_stream(AVFormatContext *fmt_ctx, int stream_idx)
> > +{
> > +    AVStream *stream = fmt_ctx->streams[stream_idx];
> > +    AVCodecContext *dec_ctx;
> > +    AVCodec *dec;
> > +    char val_str[128];
> > +    AVMetadataTag *tag;
> > +    char a, b, c, d;
> > +
> 
> > +    printf("[STREAM]\n");
> > +
> > +    printf("index=%d\n",        stream->index);
> 
> can be merged, and this applies to more printfs

I have no doubt, but this would lead to plenty unreadable:

printf("foo=%d\n"
       "bar=%d\n"
       "baz=%s\n"
       ...,
       foo, bar, baz, string_value(etc));

I'll do that if I find evidence that there is a significant speed gain
though.

Regards.
-- 
FFmpeg = Freak Freak Multimedia Plastic Elected Geek