[FFmpeg-devel] [PATCH] ffprobe: generalize writer subsection nesting model

[Stefano Sabatini]

On date Saturday 2012-09-22 01:01:03 +0200, Clément Bœsch encoded:
> On Wed, Sep 19, 2012 at 10:23:14AM +0200, Stefano Sabatini wrote:
> > On date Tuesday 2012-09-18 23:25:40 +0200, Clément Bœsch encoded:
> > > 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

Fixed.

> > > 
> > > > +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;
> > >     ...
> > 
> > I may need to check on the identity of a section (if parent_section ==
> > fruits_section), I could still do it embedding the type in the section
> > type, but looks more verbose, so I see no need for it.
> > 
> 
> Or maybe you could just store the sections as integer id instead of const
> struct pointers, leading to a "if (parent_section_id ==
> SECTION_TYPE_FRUITS)" instead.
> 
> The main point is to group all the section declarations into one blob
> (instead of a somehow stray list), which would likely look less hacky and
> more obvious, IMO.

Changed that way (having all the sections in an iterable collection
simplify things later).

> [...]
> > > > -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?
> > 
> > Yes it is, but what do you gain from it?
> > 
> 

> Special cases would be handled because of explicit section properties
> (like let's say a with a "if (section_settings[parent_sid].is_multiple)"
> or any field better worded).
> 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ffprobe-generalize-writer-subsection-nesting-model.patch
Type: text/x-diff
Size: 50622 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120925/fdf5ee4d/attachment.bin>