[FFmpeg-devel] [PATCH] ffprobe: add compact and compactnk writers

[Stefano Sabatini]

On date Wednesday 2011-09-28 08:06:27 +0200, Clément Bœsch encoded:
> On Wed, Sep 28, 2011 at 01:57:19AM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2011-09-28 01:19:06 +0200, Alexander Strasser encoded:
> > > Hi
> > > 
> > > Stefano Sabatini wrote:
> > > > On date Sunday 2011-09-25 16:26:55 +0200, Alexander Strasser encoded:
> > > > > Stefano Sabatini wrote:
> > > [...]
> > > > > > I added escaping (which may be used e.g. for CSV output), and change
> > > > > 
> > > > >   I think for CSV output it would be better to adhere to RFC 4180.
> > > > > Probably with the exception of "Each line should contain the same
> > > > > number of fields throughout the file.", but that is "should" anyway.
> > > > 
> > > > MS-escaping (as described in RFC4180) is mal-designed and thus I'm not
> > > > eager to make of this the default escaping algo, I suppose the best
> > > > option is to make the escaping algorithm configurable, which requires
> > > > some major redesign.
> > > 
> > >   If it should be default or not I cannot judge, but it would make sense
> > > because it is what people usually expect when talking about CSV (if that
> > > can at all be said about such a adhoc/no-real-spec file format).
> > > 
> > >   But as you say, being able to choose the escaping algo would probably
> > > be nice to have.
> > 
> > Well, actually *this* is my idea, maybe overkill, possibly extensible
> > to other objects in libav* (e.g. *showinfo filters).
> > 
> 
> Yeah, a bit overkill, but I like the idea of the writer context.
> 
> [...]
> > From 49101b8269c1d0df6b1300023c11f6f48a6ca603 Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefasab at gmail.com>
> > Date: Tue, 27 Sep 2011 20:07:51 +0200
> > Subject: [PATCH] ffprobe: move writers to separate files, and use them
> > 
> > Generalize writers API, and move them to separate dedicated files.
> > ---
> >  Makefile  |    6 +-
> >  ffprobe.c |  335 +++++++++----------------------------------------------------
> >  writers.c |  312 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  writers.h |  132 ++++++++++++++++++++++++
> >  4 files changed, 493 insertions(+), 292 deletions(-)
> >  create mode 100644 writers.c
> >  create mode 100644 writers.h
> 
> The item separator isn't honored correctly here in JSON output after this
> change:
> 
>     "streams": [{...}{...}...]
>                     ^^
>                  missing sep

fixed

> 
> Also, the JSON show tags is broken:
> 
>     "tags": {
>     ,
>     ,
>     "filename": "BAARS___.TTF",
>     "mimetype": "application/x-truetype-font"
>     }

fixed

> [...]
> > -#define SECTION_PRINT(name, multiple_entries, left) do {      \
> > -    if (do_show_ ## name) {                                   \
> > -        if (w->print_section_start)                           \
> > -            w->print_section_start(#name, multiple_entries);  \
> > -        show_ ## name (w, fmt_ctx);                           \
> > -        if (w->print_section_end)                             \
> > -            w->print_section_end(#name, multiple_entries);    \
> > -        if (left)                                             \
> > -            printf("%s", w->section_sep);                     \
> > -    }                                                         \
> 
> > +#define PRINT_CHAPTER(name) do {                                        \
> > +    if (do_show_ ## name) {                                             \
> > +        av_writer_print_chapter_header(wctx, #name);                    \
> > +        show_ ## name (wctx, fmt_ctx);                                  \
> > +        av_writer_print_chapter_footer(wctx, #name);                    \
> > +    }                                                                   \
> 

> What happened to the section separator which have to be displayed only if
> another section is following?

Not required, this is handled internally by the writer, by writing a
chapter separator when the next chapter starts (if required).
 
> [...]
> > +AVWriter default_writer = {
> > +    .name         = "default",
> > +    .print_footer = default_print_footer,
> > +    WRITER_FUNC(default),
> > +};
> > +
> > +/* JSON output */
> > +
> > +typedef struct {
> > +    int multiple_entries; ///< tells if the given chapter requires multiple entries
> > +} JSONContext;
> 
> Why do you think this should be handled in the writer context?  Maybe
> other writers might need this information as well.

Right now only JSON needs it, so I prefer to keep this private.
 
> [...]
> 
> > +struct print_buf {
> > +    char *s;
> > +    int len;
> > +};
> > +
> 
> Can't this be put in the AVWriterContext?

How, why?

...

Updated, note this is just a work in progress, indeed I didn't much
effort to cleanse all the possible bugs, just wanted to see if such a
refactoring was possible and how hard it was. As for the final shape
I'm not yet sure (e.g.: moving writers to lavu -> ABI problems in case
of changes, keep them in the top dir, directly include them in
ffprobe.c or link them *only* to it), but it doesn't look that bad and
in general I'd prefer to keep writers complexity away from ffprobe.c
(and possibly use them in other parts of the library when/if
senseful).
-- 
FFmpeg = Friendly & Fast Merciful Poor Easy Guide
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ffprobe-move-writers-to-separate-files-and-use-them.patch
Type: text/x-diff
Size: 29585 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110928/4be24027/attachment.bin>