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

Michael Niedermayer michaelni
Tue Dec 16 22:05:43 CET 2008


On Tue, Dec 16, 2008 at 09:29:02PM +0100, Stefano Sabatini wrote:
> On date Tuesday 2008-12-16 18:24:21 +0100, Michael Niedermayer encoded:
> > On Tue, Dec 16, 2008 at 09:14:06AM +0100, Stefano Sabatini wrote:
> [...]
> > > I think it is better to follow the path:
> > > * define the new function;
> > > * replace all the uses of the old function
> > > * deprecate + ifdef the old function
> > > 
> > > implemented by the patchset.
> > > 
> > > And I think that if you want to ifdef a function then you have also to
> > > deprecate it, so is nicer to first remove the old function use thus
> > > avoiding warnings.
> > > 
> > > But if you prefer the other way (define av_set_string3() *and*
> > > ifdef/deprecate av_set_string2() at once) I'm fine with it too.
> > 
> > i prefer to mark the old as deprecated when the new is added because
> > that is the point where it is deprecated, not once it isnt used anywhere
> > anymore.
> > the "deprecated" gives a nice reminder so its not forgotten, when its
> > done seperately it can easily be forgotten.
> 
> Sorry to bother again, I promise this is the last time (at least in
> this thread)...
> 
> So what about:
> 1) define the new function *and* deprecate the old one
> 2) replace all the old function uses
> 3) ifdef the old function
> 
> Rationale: deprecated functions shouldn't be iff'ed when they're
> still used in the project code.
> 
> Regards.
> -- 
> FFmpeg = Faboulous and Freak Marvellous Prodigious Educated Geisha

> Index: ffmpeg/libavcodec/opt.h
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.h	2008-12-16 19:46:41.000000000 +0100
> +++ ffmpeg/libavcodec/opt.h	2008-12-16 19:50:05.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()
> + */
> +attribute_deprecated const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc);
> +
> +/**
>   * 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[out] o_out if non-NULL put here a pointer to the AVOption
> + * found
>   * @param alloc when 1 then the old value will be av_freed() and the
>   *                     new av_strduped()
>   *              when 0 then no av_free() nor av_strdup() will be used
> + * @return 0 if the value has been set, an AVERROR* error code value
> + * if no matching option exists, or if the value \p val is not valid
>   */
> -const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc);
> +int av_set_string3(void *obj, const char *name, const char *val, int alloc, const AVOption **o_out);
>  
>  const AVOption *av_set_double(void *obj, const char *name, double n);
>  const AVOption *av_set_q(void *obj, const char *name, AVRational n);
> Index: ffmpeg/libavcodec/opt.c
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.c	2008-12-16 19:46:41.000000000 +0100
> +++ ffmpeg/libavcodec/opt.c	2008-12-16 19:46:47.000000000 +0100
> @@ -107,10 +107,16 @@
>      return -1;
>  }
>  
> -const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc){
> +int av_set_string3(void *obj, const char *name, const char *val, int alloc, const AVOption **o_out){
> +    int ret;
>      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
> -    if(!o || !val || o->offset<=0)
> -        return NULL;
> +    if (o_out)
> +        *o_out = o;
> +    if(!o)
> +        return AVERROR(ENOENT);
> +    if(!val || o->offset<=0)
> +        return AVERROR(EINVAL);
> +
>      if(o->type == FF_OPT_TYPE_BINARY){
>          uint8_t **dst = (uint8_t **)(((uint8_t*)obj) + o->offset);
>          int *lendst = (int *)(dst + 1);
> @@ -118,7 +124,7 @@
>          int len = strlen(val);
>          av_freep(dst);
>          *lendst = 0;
> -        if (len & 1) return NULL;
> +        if (len & 1) return AVERROR(EINVAL);
>          len /= 2;
>          ptr = bin = av_malloc(len);
>          while (*val) {
> @@ -126,13 +132,13 @@
>              int b = hexchar2int(*val++);
>              if (a < 0 || b < 0) {
>                  av_free(bin);
> -                return NULL;
> +                return AVERROR(EINVAL);
>              }
>              *ptr++ = (a << 4) | b;
>          }
>          *dst = bin;
>          *lendst = len;
> -        return o;
> +        return 0;
>      }
>      if(o->type != FF_OPT_TYPE_STRING){
>          int notfirst=0;
> @@ -163,7 +169,7 @@
>                  else {
>                      if (error)
>                          av_log(NULL, AV_LOG_ERROR, "Unable to parse option value \"%s\": %s\n", val, error);
> -                    return NULL;
> +                    return AVERROR(EINVAL);
>                  }
>              }
>              if(o->type == FF_OPT_TYPE_FLAGS){
> @@ -174,14 +180,14 @@
>                  else if(cmd=='-') d= notfirst*av_get_double(obj, name, NULL) - d;
>              }
>  
> -            if (!av_set_number(obj, name, d, 1, 1))
> -                return NULL;
> +            if ((ret = av_set_number2(obj, name, d, 1, 1, o_out)) < 0)
> +                return ret;
>              val+= i;
>              if(!*val)
> -                return o;
> +                return 0;
>              notfirst=1;
>          }
> -        return NULL;
> +        return AVERROR(EINVAL);
>      }
>  
>      if(alloc){
> @@ -190,6 +196,13 @@
>      }
>  
>      memcpy(((uint8_t*)obj) + o->offset, &val, sizeof(val));
> +    return 0;
> +}
> +
> +const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc){
> +    const AVOption *o;
> +    if (av_set_string3(obj, name, val, alloc, &o) < 0)
> +        return NULL;
>      return o;
>  }
>  

ok


> Index: ffmpeg/cmdutils.c
> ===================================================================
> --- ffmpeg.orig/cmdutils.c	2008-12-16 19:46:39.000000000 +0100
> +++ ffmpeg/cmdutils.c	2008-12-16 21:19:16.000000000 +0100
> @@ -176,25 +176,33 @@
>  
>  int opt_default(const char *opt, const char *arg){
>      int type;
> +    int ret= 0;
>      const AVOption *o= NULL;
>      int opt_types[]={AV_OPT_FLAG_VIDEO_PARAM, AV_OPT_FLAG_AUDIO_PARAM, 0, AV_OPT_FLAG_SUBTITLE_PARAM, 0};
>  
> -    for(type=0; type<CODEC_TYPE_NB; type++){
> +    for(type=0; type<CODEC_TYPE_NB && ret>= 0; type++){
>          const AVOption *o2 = av_find_opt(avctx_opts[0], opt, NULL, opt_types[type], opt_types[type]);
>          if(o2)
> -            o = av_set_string2(avctx_opts[type], opt, arg, 1);
> +            ret = av_set_string3(avctx_opts[type], opt, arg, 1, &o);
>      }

> +    if(o && ret < 0)
> +        goto invalid_arg;

do these 2 lines hav any effect?

>      if(!o)
> -        o = av_set_string2(avformat_opts, opt, arg, 1);
> +        ret = av_set_string3(avformat_opts, opt, arg, 1, &o);
>      if(!o)
> -        o = av_set_string2(sws_opts, opt, arg, 1);
> +        ret = av_set_string3(sws_opts, opt, arg, 1, &o);
>      if(!o){
>          if(opt[0] == 'a')
> -            o = av_set_string2(avctx_opts[CODEC_TYPE_AUDIO], opt+1, arg, 1);
> +            ret = av_set_string3(avctx_opts[CODEC_TYPE_AUDIO], opt+1, arg, 1, &o);
>          else if(opt[0] == 'v')
> -            o = av_set_string2(avctx_opts[CODEC_TYPE_VIDEO], opt+1, arg, 1);
> +            ret = av_set_string3(avctx_opts[CODEC_TYPE_VIDEO], opt+1, arg, 1, &o);
>          else if(opt[0] == 's')
> -            o = av_set_string2(avctx_opts[CODEC_TYPE_SUBTITLE], opt+1, arg, 1);
> +            ret = av_set_string3(avctx_opts[CODEC_TYPE_SUBTITLE], opt+1, arg, 1, &o);
> +    }
> +invalid_arg:
> +    if (o && ret < 0) {
> +        fprintf(stderr, "Invalid value '%s' for option '%s'\n", arg, opt);
> +        exit(1);
>      }
>      if(!o)
>          return -1;

> Index: ffmpeg/cmdutils.c
> ===================================================================
> --- ffmpeg.orig/cmdutils.c	2008-12-16 01:16:47.000000000 +0100
> +++ ffmpeg/cmdutils.c	2008-12-16 01:16:50.000000000 +0100
> @@ -227,7 +227,7 @@
>          const char *str= av_get_string(opts_ctx, opt_names[i], &opt, buf, sizeof(buf));
>          /* if an option with name opt_names[i] is present in opts_ctx then str is non-NULL */
>          if(str && ((opt->flags & flags) == flags))
> -            av_set_string2(ctx, opt_names[i], str, 1);
> +            av_set_string3(ctx, opt_names[i], str, 1, NULL);
>      }
>  }
>  

ok


> Index: ffmpeg/ffserver.c
> ===================================================================
> --- ffmpeg.orig/ffserver.c	2008-12-16 01:14:28.000000000 +0100
> +++ ffmpeg/ffserver.c	2008-12-16 01:47:01.000000000 +0100

iam not ffserver maintainer


> Index: ffmpeg/libavcodec/opt.c
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.c	2008-12-16 01:17:38.000000000 +0100
> +++ ffmpeg/libavcodec/opt.c	2008-12-16 01:46:59.000000000 +0100
> @@ -207,7 +207,10 @@
>  }
>  
>  const AVOption *av_set_string(void *obj, const char *name, const char *val){
> -    return av_set_string2(obj, name, val, 0);
> +    const AVOption *o;
> +    if (av_set_string3(obj, name, val, 0, &o) < 0)
> +        return NULL;
> +    return o;
>  }
>  
>  const AVOption *av_set_double(void *obj, const char *name, double n){

ok


[...]

> Index: ffmpeg/libavcodec/opt.c
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.c	2008-12-16 19:50:54.000000000 +0100
> +++ ffmpeg/libavcodec/opt.c	2008-12-16 20:07:34.000000000 +0100
> @@ -199,6 +199,7 @@
>      return 0;
>  }
>  
> +#if LIBAVCODEC_VERSION_MAJOR < 53
>  const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc){
>      const AVOption *o;
>      if (av_set_string3(obj, name, val, alloc, &o) < 0)
> @@ -212,6 +213,7 @@
>          return NULL;
>      return o;
>  }
> +#endif
>  
>  const AVOption *av_set_double(void *obj, const char *name, double n){
>      return av_set_number(obj, name, n, 1, 1);
> Index: ffmpeg/libavcodec/opt.h
> ===================================================================
> --- ffmpeg.orig/libavcodec/opt.h	2008-12-16 19:50:39.000000000 +0100
> +++ ffmpeg/libavcodec/opt.h	2008-12-16 20:39:59.000000000 +0100
> @@ -99,6 +99,7 @@
>   */
>  const AVOption *av_find_opt(void *obj, const char *name, const char *unit, int mask, int flags);
>  
> +#if LIBAVCODEC_VERSION_MAJOR < 53
>  /**
>   * @see av_set_string2()
>   */
> @@ -111,6 +112,7 @@
>   * @see av_set_string3()
>   */
>  attribute_deprecated const AVOption *av_set_string2(void *obj, const char *name, const char *val, int alloc);
> +#endif
>  
>  /**
>   * Sets the field of obj with the given name to value.

ok

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20081216/e55be444/attachment.pgp>



More information about the ffmpeg-devel mailing list