[FFmpeg-devel] [PATCH] ffprobe: generalize writer subsection nesting model
Clément Bœsch
ubitux at gmail.com
Tue Sep 18 23:25:40 CEST 2012
On Tue, Sep 18, 2012 at 12:38:39AM +0200, Stefano Sabatini wrote:
> 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, slightly simplify the code.
> ---
> ffprobe.c | 515 +++++++++++++++++++++++++++++--------------------------------
> 1 files changed, 242 insertions(+), 273 deletions(-)
>
Good idea :)
> diff --git a/ffprobe.c b/ffprobe.c
> index a0aee83..d198f90 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -28,6 +28,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"
> @@ -66,6 +67,40 @@ static int show_private_data = 1;
>
> static char *print_format;
>
> +/* section structure definition */
> +
> +struct section {
> + const char *name;
> + int flags;
> +};
> +
> +#define SECTION_FLAG_IS_WRAPPER 1 ///< the section only contains other sections, but has no internal data
> +#define SECTION_FLAG_IS_ARRAY 2 ///< the section contains an array of elements of the same type
> +
> +struct section root_section = { "root", 1 };
> +
1 can be replaced with the flag
> +struct section packets_and_frames_section = { "packets_and_frames", SECTION_FLAG_IS_ARRAY };
> +
> +struct section frames_section = { "frames", SECTION_FLAG_IS_ARRAY };
> +struct section frame_section = { "frame", 0 };
> +struct section frame_tags_section = { "tags", SECTION_FLAG_IS_ARRAY };
> +
> +struct section packets_section = { "packets", SECTION_FLAG_IS_ARRAY };
> +struct section packet_section = { "packet", 0 };
> +
> +struct section error_section = { "error", 0 };
> +
> +struct section format_section = { "format", 0 };
> +struct section format_tags_section = { "tags", SECTION_FLAG_IS_ARRAY };
> +
> +struct section library_versions_section = { "library_versions", SECTION_FLAG_IS_ARRAY };
> +struct section library_version_section = { "library_version", 0 };
> +struct section program_version_section = { "program_version", 0 };
> +
> +struct section streams_section = { "streams", SECTION_FLAG_IS_ARRAY };
> +struct section stream_section = { "stream", 0 };
> +struct section stream_tags_section = { "tags", SECTION_FLAG_IS_ARRAY };
> +
Isn't it possible to do something like:
static const struct section {
const char *name;
int flags;
} section_settings[] = {
[SECTION_TYPE_ROOT] = {"root", SECTION_FLAG_...},
[SECTION_TYPE_FRAMES] = {"frames", SECTION_FLAG_...},
...
};
Also, instead of flags, you might want to use bit fields:
const char *name;
int wrapper:1;
int array:1;
...
> static const OptionDef *options;
>
> /* FFprobe context */
> @@ -160,13 +195,8 @@ typedef struct Writer {
> int (*init) (WriterContext *wctx, const char *args, void *opaque);
> 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 *);
> @@ -174,19 +204,24 @@ typedef struct Writer {
> int flags; ///< a combination or WRITER_FLAG_*
> } Writer;
>
> +#define MAX_SECTION_LEVEL 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
> +
> 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 at 0
> + unsigned int nb_item[MAX_SECTION_LEVEL];
> + const struct section *sections[MAX_SECTION_LEVEL];
> +
> int is_fmt_chapter; ///< tells if the current chapter is "format", required by the print_format_entry option
> int is_packets_and_frames; ///< tells if the current section is "packets_and_frames"
> };
> @@ -234,6 +269,7 @@ static int writer_open(WriterContext **wctx, const Writer *writer,
>
> (*wctx)->class = &writer_class;
> (*wctx)->writer = writer;
> + (*wctx)->level = -1;
>
> if (writer->priv_class) {
> void *priv_ctx = (*wctx)->priv;
> @@ -256,64 +292,47 @@ fail:
> return ret;
> }
>
> -static inline void writer_print_header(WriterContext *wctx)
> +static inline void writer_print_section_header(WriterContext *wctx,
> + const struct section *section)
> {
> - if (wctx->writer->print_header)
> - wctx->writer->print_header(wctx);
> - wctx->nb_chapter = 0;
> -}
> + const struct section *parent_section;
>
> -static inline void writer_print_footer(WriterContext *wctx)
> -{
> - if (wctx->writer->print_footer)
> - wctx->writer->print_footer(wctx);
> -}
> + wctx->level++;
> + av_assert0(wctx->level <= MAX_SECTION_LEVEL);
> + parent_section = wctx->level ? wctx->sections[wctx->level-1] : NULL;
>
> -static inline void writer_print_chapter_header(WriterContext *wctx,
> - const char *chapter)
> -{
> - wctx->nb_section =
> - wctx->nb_section_packet = wctx->nb_section_frame =
> - wctx->nb_section_packet_frame = 0;
> - wctx->is_packets_and_frames = !strcmp(chapter, "packets_and_frames");
> - wctx->multiple_sections = !strcmp(chapter, "packets") || !strcmp(chapter, "frames" ) ||
> - wctx->is_packets_and_frames ||
> - !strcmp(chapter, "streams") || !strcmp(chapter, "library_versions");
> - wctx->is_fmt_chapter = !strcmp(chapter, "format");
> + wctx->nb_item[wctx->level] = 0;
> + wctx->sections[wctx->level] = section;
According to the assert above, wctx->level can reach MAX_SECTION_LEVEL; an
overflow looks possible.
>
> - if (wctx->writer->print_chapter_header)
> - wctx->writer->print_chapter_header(wctx, chapter);
> -}
> + if (section == &packets_and_frames_section) {
> + wctx->nb_section_packet = wctx->nb_section_frame =
> + wctx->nb_section_packet_frame = 0;
> + } else if (parent_section == &packets_and_frames_section) {
> + wctx->nb_section_packet_frame = section == &packet_section ?
> + wctx->nb_section_packet : wctx->nb_section_frame;
> + }
>
> -static inline void writer_print_chapter_footer(WriterContext *wctx,
> - const char *chapter)
> -{
> - if (wctx->writer->print_chapter_footer)
> - wctx->writer->print_chapter_footer(wctx, chapter);
> - wctx->nb_chapter++;
> -}
> + wctx->is_fmt_chapter = !strcmp(section->name, "format");
>
> -static inline void writer_print_section_header(WriterContext *wctx,
> - const char *section)
> -{
> - if (wctx->is_packets_and_frames)
> - wctx->nb_section_packet_frame = !strcmp(section, "packet") ? wctx->nb_section_packet
> - : wctx->nb_section_frame;
> if (wctx->writer->print_section_header)
> - wctx->writer->print_section_header(wctx, section);
> - wctx->nb_item = 0;
> + wctx->writer->print_section_header(wctx);
> }
>
> -static inline void writer_print_section_footer(WriterContext *wctx,
> - const char *section)
> +static inline void writer_print_section_footer(WriterContext *wctx)
> {
> - if (wctx->writer->print_section_footer)
> - wctx->writer->print_section_footer(wctx, section);
> - if (wctx->is_packets_and_frames) {
> - if (!strcmp(section, "packet")) wctx->nb_section_packet++;
> - else wctx->nb_section_frame++;
> + const struct section *section = wctx->sections[wctx->level];
> + const struct section *parent_section = wctx->level ?
> + wctx->sections[wctx->level-1] : NULL;
> +
> + if (parent_section)
> + wctx->nb_item[wctx->level-1]++;
> + if (parent_section == &packets_and_frames_section) {
Isn't it possible to use a section->flag for this?
[...]
I'll make a deeper review in the next patches according to what you decide
based on the comments I made on top of the mail.
--
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/20120918/5167867e/attachment.asc>
More information about the ffmpeg-devel
mailing list