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

[Michael Niedermayer]

On Sun, Nov 30, 2008 at 08:22:35PM +0100, Stefano Sabatini wrote:
> 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 problem iam having with this is when the "user" doesnt care about the
error message and thus passes NULL.
At that point he cant find out if the call succeeded

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20081130/c33be21e/attachment.pgp>