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

[Michael Niedermayer]

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)


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130415/475955dd/attachment.asc>