[FFmpeg-devel] [PATCH] Enhance ffmpeg.c:opt_default()

[Stefano Sabatini]

On date Sunday 2008-11-30 19:54:13 +0100, Michael Niedermayer encoded:
> On Sat, Nov 29, 2008 at 07:24:08PM +0100, Stefano Sabatini wrote:
> > On date Friday 2008-08-22 19:33:09 +0200, Stefano Sabatini encoded:
> > [...]
> > > This should eventually fix the weird error reporting for errors of the
> > > kind:
> > > stefano at geppetto ~> ffmpeg -bt foo
> > > FFmpeg version SVN-r14865, Copyright (c) 2000-2008 Fabrice Bellard, et al.
> > >   configuration: --prefix=/home/stefano --disable-shared --enable-libschroedinger --enable-libx264 --enable-libxvid --enable-pthreads --enable-gpl --enable-debug=3 --enable-libtheora --enable-libvorbis --enable-avfilter --enable-libamr-nb --enable-libamr-wb --enable-nonfree --enable-libfaad --enable-libfaac --enable-x11grab --enable-libmp3lame --disable-optimizations --disable-mmx
> > >   libavutil     49.10. 0 / 49.10. 0
> > >   libavcodec    51.68. 0 / 51.68. 0
> > >   libavformat   52.20. 0 / 52.20. 0
> > >   libavdevice   52. 1. 0 / 52. 1. 0
> > >   libavfilter    0. 1. 0 /  0. 1. 0
> > >   built on Aug 20 2008 17:53:40, gcc: 4.2.3 20071014 (prerelease) (Debian 4.2.2-3)
> > > Unable to parse option value "foo": undefined constant or missing (
> > > Unable to parse option value "foo": undefined constant or missing (
> > > Unable to parse option value "foo": undefined constant or missing (
> > > ffmpeg: unrecognized option '-bt'
> > 
> > New try. Now the error messages will look like:
> > stefano at geppetto ~/s/ffmpeg> ffmpeg -bt -1
> > [...]
> > Invalid value '-1' for option 'bt': Value -1.000000 for parameter 'bt' out of range
> > 
> > stefano at geppetto ~/s/ffmpeg> ffmpeg -bt foo
> > [...]
> > Invalid value 'foo' for option 'bt': Value foo for parameter 'bt' non-parsable: undefined constant or missing (
> 
> [...]
> error->eval_error
> ok
> 
> 
> > Index: ffmpeg/libavcodec/opt.c
> > ===================================================================
> > --- ffmpeg.orig/libavcodec/opt.c	2008-11-29 18:03:00.000000000 +0100
> > +++ ffmpeg/libavcodec/opt.c	2008-11-29 18:42:49.000000000 +0100
> > @@ -28,6 +28,7 @@
> >  #include "avcodec.h"
> >  #include "opt.h"
> >  #include "eval.h"
> > +#include "libavutil/avstring.h"
> >  
> >  //FIXME order them and do a bin search
> >  const AVOption *av_find_opt(void *v, const char *name, const char *unit, int mask, int flags){
> 
> > @@ -47,15 +48,16 @@
> >      else                     return (*(AVClass**)obj)->option;
> >  }
> >  
> > -static const AVOption *av_set_number(void *obj, const char *name, double num, int den, int64_t intnum){
> > +static const AVOption *av_set_number2(void *obj, const char *name, double num, int den, int64_t intnum, char *error, unsigned int error_size){
> >      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
> >      void *dst;
> > +    *error = 0;
> >      if(!o || o->offset<=0)
> >          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_strlcatf(error, error_size, "Value %lf for parameter '%s' out of range.", num, name);
> 
> > -        return NULL;
> > +        return o;
> 
> why?
> 
> 
> [...]
> > @@ -184,9 +185,10 @@
> >                  else if(!strcmp(buf, "none"   )) d= 0;
> >                  else if(!strcmp(buf, "all"    )) d= ~0;
> >                  else {
> > -                    if (eval_error)
> > -                        av_log(NULL, AV_LOG_ERROR, "Unable to parse option value \"%s\": %s\n", val, eval_error);
> > +                    if (eval_error) {
> > +                        av_strlcatf(error, error_size, "Value %s for parameter '%s' non-parsable: %s", val, name, eval_error);
> 
> > -                    return NULL;
> > +                        return o;
> 
> same, why?

The current semantics of av_set_string2() is to return NULL if the
option hasn't been found *or* if the value wasn't valid, so there is
currently no way to distinguish the two cases (which is the reason of
the patchset).

I think that if the function *finds* the option but can't set the
value because the value isn't valid, it is still a good idea to return
the option found, at least it seems better because this way we're
providing more information to the invoker.

The alternative is between the two usages:

AVOption *o = av_set_string3(..., error, error_size);
if (!o)
    fprintf(stderr, "Option not found!\n");
if (*error)
    fprintf(stderr, "Invalid value!\n");

versus:

AVOption *o = av_set_string3(..., error, error_size);
if (!o && !*error)
    fprintf(stderr, "Option not found!\n");
    return -1;
if (!o && *error)
    fprintf(stderr, "Invalid value!\n");

I think the former is more natural.

Regards.
-- 
FFmpeg = Freak and Fantastic Magic Pitiless Eretic Gymnast