[FFmpeg-devel] [PATCH] ffprobe: generalize writer subsection nesting model
Clément Bœsch
ubitux at gmail.com
Tue Sep 25 21:39:00 CEST 2012
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.
BTW, what's the order logic?
> +} SectionType;
> +
> +#define SECTION_ENTRY(type, name, flags) \
> + [SECTION_TYPE_##type] = { SECTION_TYPE_##type, name, flags }
> +
> +struct section sections[] = {
static const?
> + 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
> };
> @@ -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?
> + 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
> + 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.
> - 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?
> + 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.
--
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/20120925/67cfd84e/attachment.asc>
More information about the ffmpeg-devel
mailing list