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

[Alexander Strasser]

Hi Clément,

  here comes a very-fast-before-going-to-sleep review of your
updated patch. I am sure Stefano will have a closer look at it.

Clément Bœsch wrote:
[...]
> From 1a956e0b4dcf0016b19e0132c79adf1a6ac9fb76 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 |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 129 insertions(+), 4 deletions(-)
> 
> diff --git a/ffprobe.c b/ffprobe.c
> index 2748224..ba66bdd 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -130,6 +130,8 @@ struct writer {
>      const char *items_sep;          ///< separator between sets of key/value couples
>      const char *section_sep;        ///< separator between sections (streams, packets, ...)
>      const char *header, *footer;
> +    void (*print_section_start)(const char *, int);
> +    void (*print_section_end)  (const char *, int);
>      void (*print_header)(const char *);
>      void (*print_footer)(const char *);
>      void (*print_fmt_f)(const char *, const char *, ...);
> @@ -138,6 +140,96 @@ struct writer {
>  };
>  
>  
> +/* JSON output */
> +
> +static void json_print_header(const char *section)
> +{
> +    printf("{\n");
> +}
> +
> +#define JSON_ESCAPE "\"\\\b\f\n\r\t"
> +#define JSON_SUBST  "\"\\bfnr"

  This looks wrong. If I understood the code below correctly
than you do a pointer subtraction of 2 pointers with the 1st
pointing inside the first string literal and the 2nd pointing
at the start of it. Afterwards you use that offset to index the
second literal. 

  So if we assume a hypothetical memory with

the addresses  3456789
               AB..X..
and the        1st,2nd strings (modified and shortened for demo)

  then

strchr returns  pointer to the 1st position:  3 - 3 = 0
strchf retruns  pointer to the 2nd position:  4 - 3 = 1

  if we use the result of the last calculation as index
to the second string we would get the zero terminator
(ASCII control NUL) which is probably not what we wanted.

  Making the long comment short I think the second Literal
misses a `t' character at the end.

  Besides the above, I don't think the C standard guarantees
that the literals after being substituted by the preprocessor
actually must be at the same storage location. IIRC at least
the first C standard would actually demand the opposite behaviour
as string literals were meant to be writable.

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

> +
> +static void json_print_fmt(const char *key, const char *fmt, ...)
> +{
> +    int len;
> +    char *raw_s = NULL, *escaped_s = NULL;
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    len = vsnprintf(NULL, 0, fmt, ap);
> +    va_end(ap);
> +    if (len < 0)
> +        goto end;
> +
> +    raw_s = av_malloc(len + 1);
> +    if (!raw_s)
> +        goto end;
> +
> +    va_start(ap, fmt);
> +    len = vsnprintf(raw_s, len + 1, fmt, ap);
> +    va_end(ap);
> +    if (len < 0)
> +        goto end;
> +
> +    escaped_s = json_escape_str(raw_s);
> +
> +end:
> +    printf("    \"%s\": \"%s\"", key, escaped_s ? escaped_s : "");
> +    av_free(raw_s);
> +    av_free(escaped_s);
> +}
> +
> +static void json_print_int(const char *key, int value)
> +{
> +    printf("    \"%s\": %d", key, value);
> +}

  The key params of those to routines are used as the name of the
object members. Those are defined as strings by the JSON RFC and
should IMHO get escaped as well.

> +
> +static void json_print_footer(const char *section)
> +{
> +    printf("\n  }");
> +}
> +
> +static void json_print_section_start(const char *section, int multiple_entries)
> +{
> +    printf("\n  \"%s\":%s", section, multiple_entries ? " [" : " ");
> +}

  If I didn't get confused this is also the name of an object member and
therefore escaping should apply here too.

  I understand the contents for some cases for which I demand escaping may
be more tightly under control by the developers, but applying escaping
consequently makes it less easy for developers to break things while making
changes in the future. A future change may be motivated by something entirely
different and it would be easy for the developer to forget to check all output
writer implementations.

  Also I am a lazy developer, that didn't yet check if one of the object
member names could be fed with content not under our control. So if it
gets decided against escaping the object member names, at least this
should be double checked.

[remainder skipped as it is getting late]

Thank you for keeping on working on the JSON output,
  Alexander