[FFmpeg-devel] [PATCH] Fix opt_default()

[Michael Niedermayer]

On Mon, Dec 15, 2008 at 12:08:49AM +0100, Stefano Sabatini wrote:
> On date Sunday 2008-12-07 18:49:21 +0100, Michael Niedermayer encoded:
> > On Sun, Dec 07, 2008 at 05:10:11PM +0100, Stefano Sabatini wrote:
> > > On date Sunday 2008-12-07 14:35:58 +0100, Michael Niedermayer encoded:
> > > > On Sun, Dec 07, 2008 at 02:45:16AM +0100, Stefano Sabatini wrote:
> [...]
> > > > > As for the index in the options array, I found its use quite awkward,
> > > > > but anyway if you like it I can implement this:
> > > > > 
> > > > > int av_set_string3(void *obj, const char *name, const char *val);
> > > > 
> > > > iam not insisting on this, what i insist on is that we will not have
> > > > 20 return -1 and parse an error message (or check if its NULL)  to find
> > > > out which kind of error it was.
> > > 
> > > I continue to prefer:
> > > 
> > > 1) int av_set_string3(void *obj, const char *name, const char *val, const AVOption **o_out);
> > > (return an error code or 0 if the option has been set, put in o_out
> > > the option eventually found)
> > > 
> > > over:
> > > 
> > > 2) int av_set_string3(void *obj, const char *name, const char *val);
> > > (return an error code or the index in the obj->options array if the
> > > option is found and the value is valid)
> > > 
> > > but I won't insist if you prefer the other one, please just say which
> > > you prefer.
> > > 
> > > Advantage of 1) over 2) is that you know the option which has been
> > > found even in case of error, so no need to iterate through options to
> > > find it again, if you don't care about the option found you can simply
> > > set o_out to NULL.
> > 
> > i dont mind 1.
> 
> Patchset attached.
> 
> Regards.
> -- 
> FFmpeg = Fiendish and Fantastic Magic Power Energized Glue

> Index: ffmpeg/libavcodec/opt.c
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.c	2008-12-06 01:06:53.000000000 +0100
> +++ ffmpeg/libavcodec/opt.c	2008-12-06 01:13:34.000000000 +0100
> @@ -54,7 +54,7 @@
>          return NULL;
>  
>      if(o->max*den < num*intnum || o->min*den > num*intnum) {
> -        av_log(NULL, AV_LOG_ERROR, "Value %lf for parameter '%s' out of range.\n", num, name);
> +        av_log(NULL, AV_LOG_ERROR, "Value %lf for parameter '%s' out of range\n", num, name);
>          return NULL;
>      }
>  

ok


> Index: ffmpeg/libavcodec/opt.c
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.c	2008-12-14 22:42:21.000000000 +0100
> +++ ffmpeg/libavcodec/opt.c	2008-12-14 23:57:53.000000000 +0100
> @@ -47,15 +47,17 @@
>      else                     return (*(AVClass**)obj)->option;
>  }
>  
> -static const AVOption *av_set_number(void *obj, const char *name, double num, int den, int64_t intnum){
> +static int av_set_number2(void *obj, const char *name, double num, int den, int64_t intnum, const AVOption **o_out){
>      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
>      void *dst;
> +    if(o_out)
> +        *o_out= o;
>      if(!o || o->offset<=0)
> -        return NULL;
> +        return AVERROR(ENOENT);
>  
>      if(o->max*den < num*intnum || o->min*den > num*intnum) {
>          av_log(NULL, AV_LOG_ERROR, "Value %lf for parameter '%s' out of range\n", num, name);
> -        return NULL;
> +        return AVERROR(ERANGE);
>      }
>  
>      dst= ((uint8_t*)obj) + o->offset;
> @@ -71,9 +73,17 @@
>          else                *(AVRational*)dst= av_d2q(num*intnum/den, 1<<24);
>          break;
>      default:
> -        return NULL;
> +        return AVERROR(EINVAL);
>      }
> -    return o;
> +    return 0;
> +}
> +
> +static const AVOption *av_set_number(void *obj, const char *name, double num, int den, int64_t intnum){
> +    const AVOption *o = NULL;
> +    if (av_set_number2(obj, name, num, den, intnum, &o) < 0)
> +        return NULL;
> +    else
> +        return o;
>  }
>  
>  static const double const_values[]={

ok


> Index: ffmpeg/libavcodec/opt.h
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.h	2008-12-14 23:17:27.000000000 +0100
> +++ ffmpeg/libavcodec/opt.h	2008-12-14 23:21:18.000000000 +0100

> @@ -105,6 +105,14 @@
>  attribute_deprecated const AVOption *av_set_string(void *obj, const char *name, const char *val);
>  
>  /**
> + * @return a pointer to the AVOption corresponding to the field set or
> + * NULL if no matching AVOption exists, or if the value \p val is not
> + * valid
> + * @see av_set_string3()
> + */
> +const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc);
> +

this should be under #ifdefs to remove it with the next major bump


> +/**
>   * Sets the field of obj with the given name to value.
>   *
>   * @param[in] obj A struct whose first element is a pointer to an
> @@ -120,14 +128,15 @@
>   * scalars or named flags separated by '+' or '-'. Prefixing a flag
>   * with '+' causes it to be set without affecting the other flags;
>   * similarly, '-' unsets a flag.
> - * @return a pointer to the AVOption corresponding to the field set or
> - * NULL if no matching AVOption exists, or if the value \p val is not
> - * valid

> + * @param[in,out] o_out if non-NULL put here a pointer to the AVOption
> + * found

id say this is just [out]

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081215/8f9954a6/attachment.pgp>