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

[Clément Bœsch]

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
> > 
> > > +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.

[...]
> > > -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).
[...]

-- 
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/20120922/702d12e2/attachment.asc>