[FFmpeg-devel] [PATCH 2/2] ffprobe: add basic JSON print format.

Alexander Strasser eclipse7 at gmx.net
Sat Sep 3 15:23:51 CEST 2011


Hi

Clément Bœsch wrote:
> On Fri, Sep 02, 2011 at 01:41:44AM +0200, Alexander Strasser wrote:
> > Clément Bœsch wrote:
> > > +static char *json_escape_str(const char *s)
> > > +{
> > > +    char *ret, *p;
> > > +    int i, len = 0;
> > > +
> > > +    for (i = 0; s[i]; i++) {
> > > +        if (strchr(JSON_ESCAPE, s[i]))     len += 2;
> > > +        else if ((unsigned char)s[i] < 32) len += 6;
> > > +        else                               len += 1;
> > > +    }
> > > +
> > > +    p = ret = av_malloc(len + 1);
> > > +    for (i = 0; s[i]; i++) {
> > > +        char *q = strchr(JSON_ESCAPE, s[i]);
> > > +        if (q) {
> > > +            *p++ = '\\';
> > > +            *p++ = JSON_SUBST[q - JSON_ESCAPE];
> > > +        } else if ((unsigned char)s[i] < 32) {
> > > +            snprintf(p, 7, "\\u00%02x", s[i] & 0xff);
> > > +            p += 6;
> > > +        } else {
> > > +            *p++ = s[i];
> > > +        }
> > > +    }
> > > +    *p = 0;
> > > +    return ret;
> > > +}
> > 
> >   I don't really like it, but it is at least short. Maybe someone
> > comes up with a simpler idea. Unfortunately I don't have any ideas
> > right now. This is really not meant to discourage you, it is just my
> > personal feeling when I read the code. I think it is a bit fragile
> > and may be expressed more robust/safely.
> > 
> 
> Feel free to propose something else :)

  Should work for now. It is rather isolated, so sticking with it now
should be fine because we can replace it fairly easy later on.

  The only thing still nagging me is if a (browser) client that receives
the JSON output could be fooled if it assumes a wrong character encoding.
It might not be possible but I didn't yet fully grasp that scenario and
under which circumstances it would arise.

[...]

> New patch attached.
[...]
> From c4c1c8da165e88cd61cb4d607d6ae24d04700115 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:44 +0200
> Subject: [PATCH] ffprobe: add basic JSON print format.
> 
> ---
>  ffprobe.c |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 140 insertions(+), 4 deletions(-)
> 
> diff --git a/ffprobe.c b/ffprobe.c
> index 2748224..6396842 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
[...]
> @@ -138,6 +140,107 @@ struct writer {
>  };
>  
>  
> +/* JSON output */
> +
> +static void json_print_header(const char *section)
> +{
> +    printf("{\n");
> +}
> +
> +static const char *json_escape = "\"\\\b\f\n\r\t";
> +static const char *json_subst  = "\"\\bfnrt";
> +
> +static char *json_escape_str(const char *s)
> +{
> +    char *ret, *p;
> +    int i, len = 0;
> +
> +    for (i = 0; s[i]; i++) {
> +        if (strchr(json_escape, s[i]))     len += 2;
> +        else if ((unsigned char)s[i] < 32) len += 6;
> +        else                               len += 1;
> +    }
> +
> +    p = ret = av_malloc(len + 1);
> +    if (!p)
> +        return NULL;
> +    for (i = 0; s[i]; i++) {
> +        char *q = strchr(json_escape, s[i]);
> +        if (q) {
> +            *p++ = '\\';
> +            *p++ = json_subst[q - json_escape];
> +        } else if ((unsigned char)s[i] < 32) {
> +            snprintf(p, 7, "\\u00%02x", s[i] & 0xff);
> +            p += 6;
> +        } else {
> +            *p++ = s[i];
> +        }
> +    }
> +    *p = 0;
> +    return ret;
> +}

  What do you think of implementing it like

static char *json_escape_str(const char *s)
{
    static const char json_escape[] = {'"', '\\', '\b', '\f', '\n', '\r', '\t'};
    static const char json_subst[]  = {'"', '\\',  'b',  'f',  'n',  'r',  't'};

that?

  Would get rid of the pointer-to-char-array indirection and the
trailing ASCII NUL that terminates the C strings.

  Other advantages are:
  - arrays are private to the only function that uses them
  - the array initializers emphasize the symmetry of the two arrays

> +static void json_print_fmt(const char *key, const char *fmt, ...)
> +{
> +    int len;
> +    char *raw_str = NULL, *value_esc = NULL, *key_esc;

  NIT: Why is value_esc inited to NULL but key_esc isn't? Or vice versa.

> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    len = vsnprintf(NULL, 0, fmt, ap);
> +    va_end(ap);
> +    if (len < 0)
> +        goto end;
> +
> +    raw_str = av_malloc(len + 1);
> +    if (!raw_str)
> +        goto end;
> +
> +    va_start(ap, fmt);
> +    len = vsnprintf(raw_str, len + 1, fmt, ap);
> +    va_end(ap);
> +    if (len < 0)
> +        goto end;
> +
> +    value_esc = json_escape_str(raw_str);
> +
> +end:
> +    key_esc = json_escape_str(key);
> +    printf("    \"%s\": \"%s\"",
> +           key_esc   ? key_esc   : "",
> +           value_esc ? value_esc : "");
> +    av_free(raw_str);
> +    av_free(key_esc);
> +    av_free(value_esc);
> +}
> +
> +static void json_print_int(const char *key, int value)
> +{
> +    char *key_esc = json_escape_str(key);
> +    printf("    \"%s\": %d", key_esc ? key_esc : "", value);
> +    av_free(key_esc);
> +}
> +
> +static void json_print_footer(const char *section)
> +{
> +    printf("\n  }");
> +}
> +
> +static void json_print_section_start(const char *section, int multiple_entries)
> +{
> +    char *section_esc = json_escape_str(section);
> +    printf("\n  \"%s\":%s", section_esc ? section_esc : "",
> +           multiple_entries ? " [" : " ");
> +    av_free(section_esc);
> +}

  Looks good to me now. The remaining parts should be OK according to
the previous patch of this series.

[...]

  Alexander


More information about the ffmpeg-devel mailing list