[FFmpeg-devel] [PATCH] ffprobe: add INI writer

[Stefano Sabatini]

On date Sunday 2012-05-27 10:14:47 +0200, Clément Bœsch encoded:
> On Sun, May 27, 2012 at 01:37:13AM +0200, Stefano Sabatini wrote:
> > Liberally based on the work of Luca Barbato <lu_zero at gentoo.org>, done
> > for libav/avprobe.
> > ---
> >  doc/ffprobe.texi |   24 +++++++++
> >  ffprobe.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 167 insertions(+), 1 deletions(-)
> > 
> > diff --git a/doc/ffprobe.texi b/doc/ffprobe.texi
> > index ed96575..b6af5df 100644
> > --- a/doc/ffprobe.texi
> > +++ b/doc/ffprobe.texi
> > @@ -269,6 +269,30 @@ CSV format.
> >  This writer is equivalent to
> >  @code{compact=item_sep=,:nokey=1:escape=csv}.
> >  
> > + at section ini
> > +INI format output.
> > +
> > +Print output in an INI based format.
> > +
> > +The following convenctions are adopted:
> > +
> 
> conventions?

Fixed.

> 
> > + at itemize
> > + at item
> > +all key and values are UTF8
> 
> UTF-8

Fixed.

> > + at item
> > +'.' is the subgroup separator
> > + at item
> > +newline, tab, form feed, and back space characters are escaped
> 
> I'd explicit the \n, \t, ...

Done.

> > + at item
> > +'\' is the escape character
> > + at item
> > +'#' is the comment indicator
> > + at item
> > +'=' is the key/value separators
> 
> separator?

Fixed.

> > + at item
> > +':' is not used but usually parsed as key/value separator
> > + at end itemize
> > +
> >  @section json
> >  JSON based format.
> >  
> > diff --git a/ffprobe.c b/ffprobe.c
> > index 1375f14..54ab9cd 100644
> > --- a/ffprobe.c
> > +++ b/ffprobe.c
> > @@ -715,6 +715,147 @@ static const Writer csv_writer = {
> >      .flags = WRITER_FLAG_DISPLAY_OPTIONAL_FIELDS,
> >  };
> >  
> > +/*
> > + * INI format output
> > + *
> > + * - all key and values are UTF8
> > + * - '.' is the subgroup separator
> > + * - newlines and the following characters are escaped
> > + * - '\' is the escape character
> > + * - '#' is the comment
> > + * - '=' is the key/value separators
> > + * - ':' is not used but usually parsed as key/value separator
> > + */
> > +
> 
> This is already in the documentation with the same typo errors, not sure
> it's worth duplicating them.

Removed docs in ffprobe.c.

> 
> > +typedef struct IniContext {
> 
> Note: do we really need to name these struct?
> 
> > +    AVBPrint section_name;
> > +    int print_packets_and_frames;
> > +    int nb_frame;
> > +    int nb_packet;
> > +} IniContext;
> > +
> 
> nit: INIContext?

I don't mind: changed.

> 
> > +static av_cold int ini_init(WriterContext *wctx, const char *args, void *opaque)
> > +{
> > +    IniContext *ini = wctx->priv;
> > +    av_bprint_init(&ini->section_name, 1, AV_BPRINT_SIZE_UNLIMITED);
> > +    ini->nb_frame = ini->nb_packet = 0;
> > +
> > +    if (args) {
> > +        av_log(wctx, AV_LOG_ERROR, "No options supported by the ini format writer: '%s'\n", args);
> 
> nit: "INI"? (we should change the strcmp to strcasecmp in the writers
> selection).

Changed since now the writer takes an option.
 
> > +    while (c = src[i++]) {
> > +        switch (c) {
> > +        case '\b': av_bprintf(dst, "%s", "\\b"); break;
> > +        case '\f': av_bprintf(dst, "%s", "\\f"); break;
> > +        case '\n': av_bprintf(dst, "%s", "\\n"); break;
> > +        case '\r': av_bprintf(dst, "%s", "\\r"); break;
> > +        case '\t': av_bprintf(dst, "%s", "\\t"); break;
> > +        case '\\':
> > +        case '#' :
> > +        case '=' :
> > +        case ':' : av_bprint_chars(dst, '\\', 1);
> > +        default:
> > +            if ((unsigned char)c < 32)
> > +                av_bprintf(dst, "\\x00%02x", c & 0xff);
> > +            else
> > +                av_bprint_chars(dst, c, 1);
> > +        break;
> 
> nit: align breaker

Fixed.

> > +        }
> > +    }
> > +    return dst->str;
> > +}
> > +
> > +static void ini_print_chapter_header(WriterContext *wctx, const char *chapter)
> > +{
> > +    IniContext *ini = wctx->priv;
> > +    if (wctx->nb_chapter)
> > +        printf("\n");
> > +    ini->print_packets_and_frames = !strcmp("packets_and_frames", chapter);
> > +}
> > +
> > +static void ini_print_section_header(WriterContext *wctx, const char *section)
> > +{
> > +    IniContext *ini = wctx->priv;
> > +    int n;
> > +    if (wctx->nb_section)
> > +        printf("\n");
> > +    av_bprint_clear(&ini->section_name);
> > +    av_bprintf(&ini->section_name, "%s", section);
> > +
> > +   if (ini->print_packets_and_frames)
> 
> align

Fixed.
 
> > +        n = !strcmp(section, "packet") ? ini->nb_packet++ : ini->nb_frame++;
> > +    else
> > +        n = wctx->nb_section;
> > +    if (wctx->multiple_sections)
> > +        av_bprintf(&ini->section_name, ".%d", n);
> > +    printf("[%s]\n", ini->section_name.str);
> > +}
> > +
> > +static void ini_print_str(WriterContext *wctx, const char *key, const char *value)
> > +{
> > +    AVBPrint buf;
> > +    av_bprint_init(&buf, 1, AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +    printf("%s=", ini_escape_str(&buf, key));
> > +    av_bprint_clear(&buf);
> > +    printf("%s\n", ini_escape_str(&buf, value));
> 
> I know this is meant to change, but using AV_BPRINT_SIZE_UNLIMITED needs a
> finalize to avoid leaks in some cases.

Fixed.

> Beside that, the rest LGTM, thanks.

Thanks for the review, patch updated.

I'll apply in a few days if there are no more comments.
-- 
FFmpeg = Frightening Fascinating Most Practical Easy Gem
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ffprobe-add-INI-writer.patch
Type: text/x-diff
Size: 7996 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120529/efac2b46/attachment.bin>