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

[Clément Bœsch]

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.

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

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.

-- 
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/20110911/2f5fe0d8/attachment.asc>