[FFmpeg-devel] [PATCH 1/4] avutil/opt: check min/max/default at ASSERT_LEVEL >= 2

[Stefano Sabatini]

On date Monday 2013-04-15 23:55:26 +0200, Michael Niedermayer encoded:
> On Mon, Apr 15, 2013 at 09:44:26PM +0200, Stefano Sabatini wrote:
> > On date Saturday 2013-04-13 20:37:12 +0200, Michael Niedermayer encoded:
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > ---
> > >  libavutil/opt.c |   19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/libavutil/opt.c b/libavutil/opt.c
> > > index 0e7a3a8..b4d1d67 100644
> > > --- a/libavutil/opt.c
> > > +++ b/libavutil/opt.c
> > > @@ -25,6 +25,7 @@
> > >   * @author Michael Niedermayer <michaelni at gmx.at>
> > >   */
> > >  
> > > +#include "avassert.h"
> > >  #include "avutil.h"
> > >  #include "avstring.h"
> > >  #include "common.h"
> > > @@ -63,6 +64,24 @@ const AVOption *av_next_option(void *obj, const AVOption *last)
> > >  const AVOption *av_opt_next(void *obj, const AVOption *last)
> > >  {
> > >      AVClass *class = *(AVClass**)obj;
> > 
> 
> > > +#if defined(HAVE_AV_CONFIG_H) && defined(ASSERT_LEVEL) && ASSERT_LEVEL >= 2
> > 
> > Don't know why HAVE_AV_CONFIG_H is useful.
> 
> the HAVE_AV_CONFIG_H might be usefull if a user application defines
> its own ASSERT_LEVEL (it lacks a prefix)
> 
> 
> > 
> > > +    AVOption *o = last && last[1].name ? last + 1 : class->option;
> > > +    if(o) {
> > 
> > nit: if_(o)
> > 
> > > +        int faulty = 0;
> > > +        if (o->type == AV_OPT_TYPE_DOUBLE || o->type == AV_OPT_TYPE_FLOAT) {
> > > +            faulty |= (o->min > o->default_val.dbl || o->default_val.dbl > o->max);
> > > +        } else if (o->type == AV_OPT_TYPE_INT || o->type == AV_OPT_TYPE_INT64) {
> > > +            faulty |= (o->min > o->default_val.i64 || o->default_val.i64 > o->max);
> > > +        } else if (o->type == AV_OPT_TYPE_RATIONAL) {
> > > +            faulty |= (o->min > av_q2d(o->default_val.q) || av_q2d(o->default_val.q) > o->max);
> > > +        }
> > > +        faulty |= o->min > o->max;
> > > +        if (faulty) {
> > > +            av_log(obj, AV_LOG_FATAL, "Invalid min/default/max combination in %s\n", o->name);
> > > +            av_assert2(0);
> > > +        }
> > > +    }
> > > +#endif
> > >      if (!last && class->option && class->option[0].name)
> > >          return class->option;
> > >      if (last && last[1].name)
> > 
> > Not against and could be definitively useful, although maybe the
> > checks should be moved to set_defaults().
> 
> that doesnt work, because triggering the set defaults for all structs
> doesnt seem that easy
> while we iterate over most/all (in printing the help for example)

LGTM.

BTW what assert failures does this patch trigger?
-- 
FFmpeg = Formidable and Fundamental Mega Portable Evanescent Gigant