[FFmpeg-devel] [PATCH 3/3] ffprobe: add JSON output printing format.

[Stefano Sabatini]

On date Sunday 2011-09-11 23:31:59 +0200, Clément Bœsch encoded:
> On Tue, Sep 06, 2011 at 03:52:00PM +0200, Stefano Sabatini wrote:
> > On date Saturday 2011-09-03 20:12:56 +0200, Clément Bœsch encoded:
> > > ---
> > >  doc/ffprobe.texi |    4 ++
> > >  ffprobe.c        |  120 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 120 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/doc/ffprobe.texi b/doc/ffprobe.texi
> > > index 6f7e83b..b66c619 100644
> > > --- a/doc/ffprobe.texi
> > > +++ b/doc/ffprobe.texi
> > > @@ -87,6 +87,10 @@ Use sexagesimal format HH:MM:SS.MICROSECONDS for time values.
> > >  Prettify the format of the displayed values, it corresponds to the
> > >  options "-unit -prefix -byte_binary_prefix -sexagesimal".
> > >  
> > > + at item -print_format
> > 
> > @item -print_format @var{format}
> > 
> 
> Oups, fixed locally.
> 
> [...]
> > > +    for (i = 0; s[i]; i++) {
> > > +        if (strchr(json_escape, s[i]))     len += 2;
> > 
> > > +        else if ((unsigned char)s[i] < 32) len += 6;
> > 
> > maybe you can pass an unsigned char string, and avoid the obufscating
> > casts, a note on the line of // handle non-printable chars
> > may help.
> > 
> 
> I'd say it is a simple string while an unsigned char * might give the
> feeling it is a bytes stream. But I don't really care so changed to
> unsigned char locally.

whatever, do as you prefer, your considerations also are valid.

> 
> Comment added too.
> 
> > 
> > > +        else                               len += 1;
> > > +    }
> > 
> > maybe add a comment header: // compute the length of the escaped string
> > 
> > > +
> > > +    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);
> > 
> > why & 0xff?
> > 
> 
> Because the printf format tend to print more than expected since s[i] is a
> signed char.
> 
>   char c = 150;
>   printf("%02x", c);        // ffffff96

>   printf("%02x", c & 0xff); // 96

curious, so c&0xff prevents the conversion char -> int, which is quite
surprising to me (possibly unsafe?).

> It also hilight the fact it's exactly 1B.
> 
> [...]
> > >      if (w->footer)
> > >          printf("%s", w->footer);
> > > @@ -500,6 +611,7 @@ static const OptionDef options[] = {
> > >        "use sexagesimal format HOURS:MM:SS.MICROSECONDS for time units" },
> > >      { "pretty", 0, {(void*)&opt_pretty},
> > >        "prettify the format of displayed values, make it more human readable" },
> > 
> > > +    { "print_format", OPT_STRING | HAS_ARG, {(void*)&print_format}, "Set the output printing format. Available formats: default, json" },
> > 
> > Nit: for consistency just "set the output printing format (available formats are: default, json)
> > 
> 
> Fixed locally.
> 
> [...]
> > Looks fine otherwise.
> 
> I'll push when we'll find an agreement for the previous patch.

Fine.
-- 
FFmpeg = Fabulous Free Mortal Philosophical Elegant Guru