[FFmpeg-devel] [PATCH 2/3] ffprobe: replace fmt callback with str callback.

[Clément Bœsch]

On Tue, Sep 13, 2011 at 07:43:40PM +0200, Stefano Sabatini wrote:
[...]
> > 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...
> 

It's mainly to avoid the confusion with the macro with the same name; I
can rename them to the long version instead if you want (print_string,
print_integer, ...).

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

It simplifies the declaration in the main show_* functions, and show_tags
implementations. There are already two extra lines dedicated to the print
buffering (struct print_buf ... and av_free() call), it's better to avoid
more redondant code imo.

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

If the alloc fail, something certainly went wrong. It is not likely to
happen, so the performance reason isn't important here imo. Also, I think
it's better to clean up properly in case of error.

[...]
> Looks fine otherwise, thanks.

I'll push it soon, thanks for the review.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/0b8e950c/attachment.asc>