[FFmpeg-devel] [PATCH 2/3] ffprobe: replace fmt callback with str callback.
Stefano Sabatini
stefasab at gmail.com
Tue Sep 13 19:43:40 CEST 2011
On date Monday 2011-09-12 22:44:22 +0200, Clément Bœsch encoded:
> On Sun, Sep 11, 2011 at 11:21:50PM +0200, Clément Bœsch wrote:
> [...]
> > > Could you benchmark it? allocing/freeing a buffer for every single
> > > print looks quite expensive.
> > >
> >
> > With a 250M media input:
> >
> > av_asprintf/av_free: ~1.5sec
> > 4096B stack buffer: ~1.1sec
> >
> > > Alternatively: you pass a buffer which is reallocated if and only when
> > > needed, this would need some custom code.
> > >
>
> 1.2sec with the "fast asprintf" version attached. I'll commit this in a
> few days if no objection.
>
> --
> Clément B.
> From 978d84f3346f7470674a8c7730d7cbc44da872da Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
> Date: Mon, 12 Sep 2011 19:22:53 +0200
> Subject: [PATCH] ffprobe: replace fmt callback with str callback.
>
> Having a string callback is much more simpler than a variable args
> one for writers to deal with, especially when dealing with escaping.
>
> This patch also introduces a local fast_asprintf() function which is
> similar to av_asprintf() but avoids reallocating at each print (leading
> to a performance issue).
> ---
> ffprobe.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/ffprobe.c b/ffprobe.c
> index 538120f..75df26c 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -137,8 +137,8 @@ struct writer {
> const char *header, *footer;
> void (*print_header)(const char *);
> void (*print_footer)(const char *);
> - void (*print_fmt_f)(const char *, const char *, ...);
> void (*print_int_f)(const char *, int);
> + void (*print_str_f)(const char *, const char *);
nit++: I wonder if it is a good idea to keep the _f prefix, indeed
print_header and print_str_f are quite inconsistent...
> void (*show_tags)(struct writer *w, AVDictionary *dict);
> };
>
> @@ -150,13 +150,9 @@ static void default_print_header(const char *section)
> printf("[%s]\n", section);
> }
>
> -static void default_print_fmt(const char *key, const char *fmt, ...)
> +static void default_print_str(const char *key, const char *value)
> {
> - va_list ap;
> - va_start(ap, fmt);
> - printf("%s=", key);
> - vprintf(fmt, ap);
> - va_end(ap);
> + printf("%s=%s", key, value);
> }
>
> static void default_print_int(const char *key, int value)
> @@ -169,14 +165,54 @@ static void default_print_footer(const char *section)
> printf("\n[/%s]", section);
> }
>
> +struct print_buf {
> + char *s;
> + int len;
> +};
uhm, I'd prefer to avoid a struct for this, that obfuscates the
code...
> +
> +static char *fast_asprintf(struct print_buf *pbuf, const char *fmt, ...)
> +{
> + va_list va;
> + int len;
> +
> + va_start(va, fmt);
> + len = vsnprintf(NULL, 0, fmt, va);
> + va_end(va);
> + if (len < 0)
> + goto fail;
> +
> + if (pbuf->len < len) {
> + char *p = av_realloc(pbuf->s, len + 1);
> + if (!p)
> + goto fail;
> + pbuf->s = p;
> + pbuf->len = len;
> + }
> +
> + va_start(va, fmt);
> + len = vsnprintf(pbuf->s, len + 1, fmt, va);
> + va_end(va);
> + if (len < 0)
> + goto fail;
> + return pbuf->s;
> +
> +fail:
> + av_freep(&pbuf->s);
> + pbuf->len = 0;
> + return NULL;
> +}
OK, but I wonder if it would be better to reuse the old buffer in case
or reallocation failure, I don't think this should block the patch
though.
> +
>
> /* Print helpers */
>
> -#define print_fmt0(k, f, ...) w->print_fmt_f(k, f, __VA_ARGS__)
> +#define print_fmt0(k, f, ...) do { \
> + if (fast_asprintf(&pbuf, f, __VA_ARGS__)) \
> + w->print_str_f(k, pbuf.s); \
> +} while (0)
> #define print_fmt( k, f, ...) do { \
> if (w->item_sep) \
> printf("%s", w->item_sep); \
> - w->print_fmt_f(k, f, __VA_ARGS__); \
> + print_fmt0(k, f, __VA_ARGS__); \
> } while (0)
>
> #define print_int0(k, v) w->print_int_f(k, v)
> @@ -194,6 +230,7 @@ static void show_packet(struct writer *w, AVFormatContext *fmt_ctx, AVPacket *pk
> {
> char val_str[128];
> AVStream *st = fmt_ctx->streams[pkt->stream_index];
> + struct print_buf pbuf = {.s = NULL};
>
> if (packet_idx)
> printf("%s", w->items_sep);
> @@ -210,6 +247,7 @@ static void show_packet(struct writer *w, AVFormatContext *fmt_ctx, AVPacket *pk
> print_fmt("pos", "%"PRId64, pkt->pos);
> print_fmt("flags", "%c", pkt->flags & AV_PKT_FLAG_KEY ? 'K' : '_');
> w->print_footer("PACKET");
> + av_free(pbuf.s);
> fflush(stdout);
> }
>
> @@ -227,10 +265,12 @@ static void show_packets(struct writer *w, AVFormatContext *fmt_ctx)
> static void default_show_tags(struct writer *w, AVDictionary *dict)
> {
> AVDictionaryEntry *tag = NULL;
> + struct print_buf pbuf = {.s = NULL};
> while ((tag = av_dict_get(dict, "", tag, AV_DICT_IGNORE_SUFFIX))) {
> printf("\nTAG:");
> print_str0(tag->key, tag->value);
> }
> + av_free(pbuf.s);
> }
>
> static void show_stream(struct writer *w, AVFormatContext *fmt_ctx, int stream_idx)
> @@ -240,6 +280,7 @@ static void show_stream(struct writer *w, AVFormatContext *fmt_ctx, int stream_i
> AVCodec *dec;
> char val_str[128];
> AVRational display_aspect_ratio;
> + struct print_buf pbuf = {.s = NULL};
>
> if (stream_idx)
> printf("%s", w->items_sep);
> @@ -307,6 +348,7 @@ static void show_stream(struct writer *w, AVFormatContext *fmt_ctx, int stream_i
> w->show_tags(w, stream->metadata);
>
> w->print_footer("STREAM");
> + av_free(pbuf.s);
> fflush(stdout);
> }
>
> @@ -320,6 +362,7 @@ static void show_streams(struct writer *w, AVFormatContext *fmt_ctx)
> static void show_format(struct writer *w, AVFormatContext *fmt_ctx)
> {
> char val_str[128];
> + struct print_buf pbuf = {.s = NULL};
>
> w->print_header("FORMAT");
> print_str0("filename", fmt_ctx->filename);
> @@ -332,6 +375,7 @@ static void show_format(struct writer *w, AVFormatContext *fmt_ctx)
> print_str("bit_rate", value_string(val_str, sizeof(val_str), fmt_ctx->bit_rate, unit_bit_per_second_str));
> w->show_tags(w, fmt_ctx->metadata);
> w->print_footer("FORMAT");
> + av_free(pbuf.s);
> fflush(stdout);
> }
>
> @@ -380,8 +424,8 @@ static int open_input_file(AVFormatContext **fmt_ctx_ptr, const char *filename)
> #define WRITER_FUNC(func) \
> .print_header = func ## _print_header, \
> .print_footer = func ## _print_footer, \
> - .print_fmt_f = func ## _print_fmt, \
> .print_int_f = func ## _print_int, \
> + .print_str_f = func ## _print_str, \
> .show_tags = func ## _show_tags
Looks fine otherwise, thanks.
--
FFmpeg = Fancy and Frenzy Murdering Pitiless Experimenting Geek
More information about the ffmpeg-devel
mailing list