[FFmpeg-devel] [PATCH 1/2] ffprobe: introduce output format writers.

[Stefano Sabatini]

On date Saturday 2011-08-27 20:34:41 +0200, Clément Bœsch encoded:
> On Sat, Aug 27, 2011 at 04:30:05PM +0200, Stefano Sabatini wrote:
[...]
> From f70a5c918f81f30427de6970d9abd093212e5487 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
> Date: Sat, 27 Aug 2011 20:19:19 +0200
> Subject: [PATCH 1/2] ffprobe: introduce output format writers.
> 
> ---
>  ffprobe.c |  293 +++++++++++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 209 insertions(+), 84 deletions(-)
> 
> diff --git a/ffprobe.c b/ffprobe.c
> index a314abe..f566ec5 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -41,6 +41,8 @@ static int use_value_prefix             = 0;
>  static int use_byte_value_binary_prefix = 0;
>  static int use_value_sexagesimal_format = 0;
>  
> +static char *print_format;
> +
>  /* globals */
>  static const OptionDef options[];
>  
> @@ -121,141 +123,219 @@ static const char *media_type_string(enum AVMediaType media_type)
>      return s ? s : "unknown";
>  }
>  
> -static void show_packet(AVFormatContext *fmt_ctx, AVPacket *pkt)
> +
> +struct writer {
> +    const char *name;
> +    const char *item_sep, *items_sep, *section_sep;

what's exactly the difference between item_sep and items_sep? Doxies are
welcome here.

> +    const char *header, *footer;
> +    void (*print_header)(const char *);
> +    void (*print_footer)(const char *);
> +    void (*print_fmt_f)(const char *, const char *, va_list ap);
> +    void (*print_int_f)(const char *, int);
> +    void (*print_str_f)(const char *, const char *);
> +    void (*show_tags)(struct writer *w, AVDictionary *dict);
> +};
[...]
> +static void show_packet(struct writer *w, AVFormatContext *fmt_ctx, AVPacket *pkt, int packet_idx)
>  {
>      char val_str[128];
>      AVStream *st = fmt_ctx->streams[pkt->stream_index];
>  
> -    printf("[PACKET]\n");
> -    printf("codec_type=%s\n"   , media_type_string(st->codec->codec_type));
> -    printf("stream_index=%d\n" , pkt->stream_index);
> -    printf("pts=%s\n"          , ts_value_string  (val_str, sizeof(val_str), pkt->pts));
> -    printf("pts_time=%s\n"     , time_value_string(val_str, sizeof(val_str), pkt->pts, &st->time_base));
> -    printf("dts=%s\n"          , ts_value_string  (val_str, sizeof(val_str), pkt->dts));
> -    printf("dts_time=%s\n"     , time_value_string(val_str, sizeof(val_str), pkt->dts, &st->time_base));
> -    printf("duration=%s\n"     , ts_value_string  (val_str, sizeof(val_str), pkt->duration));
> -    printf("duration_time=%s\n", time_value_string(val_str, sizeof(val_str), pkt->duration, &st->time_base));
> -    printf("size=%s\n"         , value_string     (val_str, sizeof(val_str), pkt->size, unit_byte_str));
> -    printf("pos=%"PRId64"\n"   , pkt->pos);
> -    printf("flags=%c\n"        , pkt->flags & AV_PKT_FLAG_KEY ? 'K' : '_');
> -    printf("[/PACKET]\n");
> +    if (packet_idx)
> +        printf("%s", w->items_sep);
> +    w->print_header("PACKET");
> +    print_str0("codec_type",    media_type_string(st->codec->codec_type));
> +    print_int("stream_index",   pkt->stream_index);
> +    print_str("pts",            ts_value_string  (val_str, sizeof(val_str), pkt->pts));
> +    print_str("pts_time",       time_value_string(val_str, sizeof(val_str), pkt->pts, &st->time_base));
> +    print_str("dts",            ts_value_string  (val_str, sizeof(val_str), pkt->dts));
> +    print_str("dts_time",       time_value_string(val_str, sizeof(val_str), pkt->dts, &st->time_base));
> +    print_str("duration",       ts_value_string  (val_str, sizeof(val_str), pkt->duration));
> +    print_str("duration_time",  time_value_string(val_str, sizeof(val_str), pkt->duration, &st->time_base));
> +    print_str("size",           value_string     (val_str, sizeof(val_str), pkt->size, unit_byte_str));

> +    print_fmt("pos", "%"PRId64, pkt->pos);
> +    print_fmt("flags",    "%c", pkt->flags & AV_PKT_FLAG_KEY ? 'K' : '_');

Nit++: this may be vertically aligned

> +    w->print_footer("PACKET");
>      fflush(stdout);
>  }
[...]  

> +#define SECTION_PRINT(name, multiple_entries, left) do {      \
> +    if (do_show_ ## name) {                                   \
> +        show_ ## name (w, fmt_ctx);                           \
> +        if (left)                                             \
> +            printf("%s", w->section_sep);                     \
> +    }                                                         \
> +} while (0)

what's the use of multiple_entries?

> +
>  static int probe_file(const char *filename)
>  {
>      AVFormatContext *fmt_ctx;
> -    int ret, i;
> +    int ret, writer_id;
> +    struct writer *w;
> +
> +    writer_id = get_writer(print_format);
> +    if (writer_id < 0) {
> +        fprintf(stderr, "Invalid output format '%s'\n", print_format);
> +        return AVERROR(EINVAL);
> +    }
> +    w = &writers[writer_id];
>  
>      if ((ret = open_input_file(&fmt_ctx, filename)))
>          return ret;
>  
> -    if (do_show_packets)
> -        show_packets(fmt_ctx);
> +    if (w->header)
> +        printf("%s", w->header);
>  
> -    if (do_show_streams)
> -        for (i = 0; i < fmt_ctx->nb_streams; i++)
> -            show_stream(fmt_ctx, i);
> +    SECTION_PRINT(packets, 1, do_show_streams || do_show_format);
> +    SECTION_PRINT(streams, 1, do_show_format);
> +    SECTION_PRINT(format,  0, 0);

Looks fine to me, although I wonder if there is a simpler way to
handle this without the use of macros.
-- 
FFmpeg = Fundamental and Fiendish Multipurpose Ponderous Extended Goblin