[FFmpeg-devel] [PATCH] ffprobe: generalize writer subsection nesting model
Stefano Sabatini
stefasab at gmail.com
Wed Sep 26 01:07:18 CEST 2012
On date Tuesday 2012-09-25 21:39:00 +0200, Clément Bœsch encoded:
> On Tue, Sep 25, 2012 at 11:56:55AM +0200, Stefano Sabatini wrote:
> [...]
> > > > Todo: yes the show_tags also should be dropped. I'll do it in a
> > > > following patch (or integrate in this one if I'll find the time).
> > > [...]
> >
> > This part of the code is definitively ugly. I could make the code more
> > generic marking the section with a flag (IS_MULTIPLE_TYPES_ARRAY or
> > something like that) and creating counters on the fly, based on the
> > section children. But I consider this to be best done as a separate
> > change.
> > --
> > FFmpeg = Frightening and Frenzy MultiPurpose Exploitable Geek
>
> > From 75de04c0864719bd6c92c92334ff7dcaae5901bd Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefasab at gmail.com>
> > Date: Mon, 17 Sep 2012 21:08:09 +0200
> > Subject: [PATCH] ffprobe: generalize writer subsection nesting model
> >
> > Discard unflexible structure based on the root/chapter/section structure
> > in favor of a generalized concept of section.
> >
> > This should allow to represent sections at generic level of nesting, and
> > allow subsections printing selection.
> >
> > Also, simplify the code.
> > ---
> > ffprobe.c | 784 ++++++++++++++++++++++++++++---------------------------------
> > 1 files changed, 357 insertions(+), 427 deletions(-)
> >
> > diff --git a/ffprobe.c b/ffprobe.c
> > index 6830f7d..71b5914 100644
> > --- a/ffprobe.c
> > +++ b/ffprobe.c
> > @@ -30,6 +30,7 @@
> >
> > #include "libavformat/avformat.h"
> > #include "libavcodec/avcodec.h"
> > +#include "libavutil/avassert.h"
> > #include "libavutil/avstring.h"
> > #include "libavutil/bprint.h"
> > #include "libavutil/opt.h"
> > @@ -69,6 +70,59 @@ static int show_private_data = 1;
> >
> > static char *print_format;
> >
> > +/* section structure definition */
> > +
> > +struct section {
> > + int type; ///< unique id indentifying a section
> > + const char *name;
> > +
> > +#define SECTION_FLAG_IS_WRAPPER 1 ///< the section only contains other sections, but has no data at its own level
> > +#define SECTION_FLAG_IS_ARRAY 2 ///< the section contains an array of elements of the same type
> > + int flags;
> > +};
> > +
> > +typedef enum {
> > + SECTION_TYPE_NONE = -1,
> > + SECTION_TYPE_ROOT,
> > + SECTION_TYPE_PACKETS_AND_FRAMES,
> > + SECTION_TYPE_FRAMES,
> > + SECTION_TYPE_FRAME,
> > + SECTION_TYPE_FRAME_TAGS,
> > + SECTION_TYPE_PACKETS,
> > + SECTION_TYPE_PACKET,
> > + SECTION_TYPE_ERROR,
> > + SECTION_TYPE_FORMAT,
> > + SECTION_TYPE_FORMAT_TAGS,
> > + SECTION_TYPE_LIBRARY_VERSIONS,
> > + SECTION_TYPE_LIBRARY_VERSION,
> > + SECTION_TYPE_PROGRAM_VERSION,
> > + SECTION_TYPE_STREAMS,
> > + SECTION_TYPE_STREAM,
> > + SECTION_TYPE_STREAM_TAGS
>
> nit: trailing comma for smaller diff next time we add one entry.
Fixed.
>
> BTW, what's the order logic?
Changed to alphabetical order.
> > +} SectionType;
> > +
> > +#define SECTION_ENTRY(type, name, flags) \
> > + [SECTION_TYPE_##type] = { SECTION_TYPE_##type, name, flags }
> > +
> > +struct section sections[] = {
>
> static const?
Changed.
> > + SECTION_ENTRY(ROOT, "root", SECTION_FLAG_IS_WRAPPER),
> > + SECTION_ENTRY(PACKETS_AND_FRAMES, "packets_and_frames", SECTION_FLAG_IS_ARRAY),
> > + SECTION_ENTRY(FRAMES, "frames", SECTION_FLAG_IS_ARRAY),
> > + SECTION_ENTRY(FRAME, "frame", 0),
> > + SECTION_ENTRY(FRAME_TAGS, "tags", 0),
> > + SECTION_ENTRY(PACKETS, "packets", SECTION_FLAG_IS_ARRAY),
> > + SECTION_ENTRY(PACKET, "packet", 0),
> > + SECTION_ENTRY(ERROR, "error", 0),
> > + SECTION_ENTRY(FORMAT, "format", 0),
> > + SECTION_ENTRY(FORMAT_TAGS, "tags", 0),
> > + SECTION_ENTRY(LIBRARY_VERSIONS, "library_versions", SECTION_FLAG_IS_ARRAY),
> > + SECTION_ENTRY(LIBRARY_VERSION, "library_version", 0),
> > + SECTION_ENTRY(PROGRAM_VERSION, "program_version", 0),
> > + SECTION_ENTRY(STREAMS, "streams", SECTION_FLAG_IS_ARRAY),
> > + SECTION_ENTRY(STREAM, "stream", 0),
> > + SECTION_ENTRY(STREAM_TAGS, "tags", 0),
> > +};
> > +
> > static const OptionDef *options;
> >
> > /* FFprobe context */
> > @@ -163,33 +217,37 @@ typedef struct Writer {
> > int (*init) (WriterContext *wctx, const char *args);
> > void (*uninit)(WriterContext *wctx);
> >
> > - void (*print_header)(WriterContext *ctx);
> > - void (*print_footer)(WriterContext *ctx);
> > -
> > - void (*print_chapter_header)(WriterContext *wctx, const char *);
> > - void (*print_chapter_footer)(WriterContext *wctx, const char *);
> > - void (*print_section_header)(WriterContext *wctx, const char *);
> > - void (*print_section_footer)(WriterContext *wctx, const char *);
> > + void (*print_section_header)(WriterContext *wctx);
> > + void (*print_section_footer)(WriterContext *wctx);
> > void (*print_integer) (WriterContext *wctx, const char *, long long int);
> > void (*print_rational) (WriterContext *wctx, AVRational *q, char *sep);
> > void (*print_string) (WriterContext *wctx, const char *, const char *);
> > - void (*show_tags) (WriterContext *wctx, AVDictionary *dict);
> > int flags; ///< a combination or WRITER_FLAG_*
> > } Writer;
> >
> > +#define SECTION_MAX_NB_LEVELS 10
> > +
> > struct WriterContext {
> > const AVClass *class; ///< class of the writer
> > const Writer *writer; ///< the Writer of which this is an instance
> > char *name; ///< name of this writer instance
> > void *priv; ///< private data for use by the filter
> > - unsigned int nb_item; ///< number of the item printed in the given section, starting at 0
> > - unsigned int nb_section; ///< number of the section printed in the given section sequence, starting at 0
> > +
> > + struct section *sections; ///< array of all sections
> > + int nb_sections; ///< number of sections
> > +
> > unsigned int nb_section_packet; ///< number of the packet section in case we are in "packets_and_frames" section
> > unsigned int nb_section_frame; ///< number of the frame section in case we are in "packets_and_frames" section
> > unsigned int nb_section_packet_frame; ///< nb_section_packet or nb_section_frame according if is_packets_and_frames
> > - unsigned int nb_chapter; ///< number of the chapter, starting at 0
> >
> > - int multiple_sections; ///< tells if the current chapter can contain multiple sections
> > + int level; ///< current level, starting from 0
> > +
> > + /** number of the item printed in the given section, starting from 0 */
> > + unsigned int nb_item[SECTION_MAX_NB_LEVELS];
> > +
> > + /** section per each level */
> > + struct section *section[SECTION_MAX_NB_LEVELS];
> > +
> > int is_fmt_chapter; ///< tells if the current chapter is "format", required by the print_format_entry option
>
> We don't have a concept of chapter anymore. Since the patch is already big
> maybe you want to delay its suppression. But since you're changing code
> around its usage anyway, I wonder if you couldn't replace it with the
> appropriate section->type == SECTION_TYPE_CHAPTER.
>
> > int is_packets_and_frames; ///< tells if the current section is "packets_and_frames"
>
> Ditto with SECTION_TYPE_PACKETS_AND_FRAMES
Both removed.
> > };
> > @@ -220,7 +278,8 @@ static void writer_close(WriterContext **wctx)
> > av_freep(wctx);
> > }
> >
> [...]
> >
> > -static void flat_print_chapter_header(WriterContext *wctx, const char *chapter)
> > +static void flat_print_section_header(WriterContext *wctx)
> > {
> > FlatContext *flat = wctx->priv;
> > - flat->chapter = chapter;
> > + AVBPrint *buf = &flat->section_header[wctx->level];
> > + int i;
> > +
> > + /* build section header */
> > + av_bprint_init(buf, 1, AV_BPRINT_SIZE_UNLIMITED);
>
> This is destroyed at some point, right?
>
> Also, would it make sense (from a performance PoV) to init/finalize all
> the buffer in the init/close writer callbacks, and just clear the buffer
> here?
Yes, changed that way.
> > + for (i = 1; i <= wctx->level; i++) {
> > + if (flat->hierarchical ||
> > + !(wctx->section[i]->flags & (SECTION_FLAG_IS_ARRAY|SECTION_FLAG_IS_WRAPPER)))
> > + av_bprintf(buf, "%s%s", wctx->section[i]->name, flat->sep_str);
> > + }
> > }
> >
> > -static void flat_print_section_header(WriterContext *wctx, const char *section)
> > +static void flat_print_key_prefix(WriterContext *wctx)
> > {
> > FlatContext *flat = wctx->priv;
> > - flat->section = section;
> > + struct section *parent_section = wctx->section[wctx->level-1];
> > +
>
> looks like it could be const
Changed.
> > + printf("%s", flat->section_header[wctx->level].str);
> > +
> > + if (parent_section->flags & SECTION_FLAG_IS_ARRAY) {
> > + int n = parent_section->type == SECTION_TYPE_PACKETS_AND_FRAMES ?
> > + wctx->nb_section_packet_frame : wctx->nb_item[wctx->level-1];
> > + printf("%d%s", n, flat->sep_str);
> > + }
> > }
> [...]
> > -static void ini_print_chapter_header(WriterContext *wctx, const char *chapter)
> > +static void ini_print_section_header(WriterContext *wctx)
> > {
> > INIContext *ini = wctx->priv;
> > + AVBPrint buf;
> > + int i;
> > + struct section *section = wctx->section[wctx->level];
> > + struct section *parent_section = wctx->level ?
> > + wctx->section[wctx->level-1] : NULL;
> >
>
> ditto; maybe for the section pointer as well from a quick look.
Fixed.
> > - av_bprint_clear(&ini->chapter_name);
> > - av_bprintf(&ini->chapter_name, "%s", chapter);
> > + av_bprint_init(&buf, 1, AV_BPRINT_SIZE_UNLIMITED);
> > + if (wctx->level == 0) {
> > + printf("# ffprobe output\n\n");
> > + return;
> > + }
> >
> [...]
> > static void xml_print_str(WriterContext *wctx, const char *key, const char *value)
> > {
> > AVBPrint buf;
> > + XMLContext *xml = wctx->priv;
> > + const struct section *section = wctx->section[wctx->level];
> >
> > - if (wctx->nb_item)
> > - printf(" ");
> > av_bprint_init(&buf, 1, AV_BPRINT_SIZE_UNLIMITED);
> > - printf("%s=\"%s\"", key, xml_escape_str(&buf, value, wctx));
> > +
> > + if (!strcmp(section->name, "tags")) {
>
> I know you're going to do some work to improve the tags thing, but
> couldn't you just factorize all these strcmp with a tags section->flags
> set for the few tags section types?
I'm not sure this is a good idea, since I'm willing to keep the
writers as much ffprobe-agnostic as possible, and possibly reuse it
later in other contexts. This needs some thought, so I'd keep it for
the moment.
>
> > + XML_INDENT();
> > + printf("<tag key=\"%s\"", xml_escape_str(&buf, key, wctx));
> > + av_bprint_clear(&buf);
> > + printf(" value=\"%s\"/>\n", xml_escape_str(&buf, value, wctx));
> > + } else {
> > + if (wctx->nb_item[wctx->level])
> > + printf(" ");
> > + av_bprint_init(&buf, 1, AV_BPRINT_SIZE_UNLIMITED);
> > + printf("%s=\"%s\"", key, xml_escape_str(&buf, value, wctx));
> > + }
> > +
> > av_bprint_finalize(&buf, NULL);
> > }
> [...]
>
> OK, that's kind of a huge patch. I'll hardly be able to make a deeper
> review. Feel free to drop any of my comments if you plan to fix them in a
> later patch, and if passes FATE.
>
> Just an extra check before you push, I'm always a bit worried about stuff
> like a missing trailing comma in the json output in some corner cases,
> like when you have metadata but empty, or no metadata at all (and so the
> section is not printed). Also when you mixed try various different
> combinations of the -show_* options.
>
> Anyway, good work so far, thanks.
I'll do more tests, and push when I feel the patch can be considered
ready.
--
FFmpeg = Funny and Friendly Mind-dumbing Problematic Embarassing Glue
More information about the ffmpeg-devel
mailing list