[FFmpeg-devel] [PATCH] ffprobe integration
Michael Niedermayer
michaelni
Sun Feb 14 23:31:11 CET 2010
On Sun, Feb 14, 2010 at 07:19:16PM +0100, Stefano Sabatini wrote:
> 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.
great now its 339 lines instead of 416
[...]
> +#define UNIT_NONE_STR ""
> +#define UNIT_SECOND_STR "s"
> +#define UNIT_HERTZ_STR "Hz"
> +#define UNIT_BIT_STR "bit"
> +#define UNIT_BYTE_STR "byte"
> +#define UNIT_BIT_PER_SECOND_STR "bit/s"
> +#define UNIT_BYTE_PER_SECOND_STR "byte/s"
static const char *unit_none_str=""
...
would allow you to compare for == and avoid the strcmp
[...]
> +static char *time_value_string(char *buf, int buf_size, int64_t val, AVRational *time_base)
> +{
> + if (val == AV_NOPTS_VALUE)
> + snprintf(buf, buf_size, "N/A");
> + else
{}
[...]
> + /* output avi tags */
> + a = isprint(a = dec_ctx->codec_tag & 0xff) ? a : '_';
> + b = isprint(b = dec_ctx->codec_tag>>8 & 0xff) ? b : '_';
> + c = isprint(c = dec_ctx->codec_tag>>16 & 0xff) ? c : '_';
> + d = isprint(d = dec_ctx->codec_tag>>24 & 0xff) ? d : '_';
obfuscated
> + printf("codec_tag_string=%c%c%c%c\n", a, b, c, d);
> + printf("codec_tag=0x%04x\n", dec_ctx->codec_tag);
> +
> + switch (dec_ctx->codec_type) {
> + case CODEC_TYPE_VIDEO:
> + printf("width=%d\n", dec_ctx->width);
> + printf("height=%d\n", dec_ctx->height);
> + printf("has_b_frames=%d\n", dec_ctx->has_b_frames);
> + printf("sample_aspect_ratio=%d:%d\n", dec_ctx->sample_aspect_ratio.num,
> + dec_ctx->sample_aspect_ratio.den);
> + printf("display_aspect_ratio=%d:%d\n", dec_ctx->sample_aspect_ratio.num,
> + dec_ctx->sample_aspect_ratio.den);
> + printf("pix_fmt=%s\n", av_pix_fmt_descriptors[dec_ctx->pix_fmt].name);
> + break;
> +
> + case CODEC_TYPE_AUDIO:
> + printf("sample_rate=%s\n", value_string(val_str, sizeof(val_str),
> + (double)dec_ctx->sample_rate,
useless cast
> + UNIT_HERTZ_STR));
> + printf("channels=%d\n", dec_ctx->channels);
> + printf("bits_per_sample=%d\n", av_get_bits_per_sample(dec_ctx->codec_id));
> + break;
> +
> + default:
> + break;
useless
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20100214/aefb37e8/attachment.pgp>
More information about the ffmpeg-devel
mailing list