[FFmpeg-devel] [RFC] av_set_string() semantics

[Michael Niedermayer]

On Sat, May 24, 2008 at 05:33:26PM +0200, Stefano Sabatini wrote:
> On date Saturday 2008-05-24 15:00:40 +0200, Michael Niedermayer encoded:
> > On Sat, May 24, 2008 at 12:22:19PM +0200, Stefano Sabatini wrote:
> > > On date Saturday 2008-05-24 12:00:27 +0200, Stefano Sabatini encoded:
> > > > Hi all,
> > > > 
> > > > Setting a value for an option with av_set_string() for a number or a
> > > > flag option currently works like this:
> > > > 
> > > > * A '+' or '-' at the beginning of the value string cause the
> > > >   following value to be added to the *default* value for the option,
> > > 
> > > Amendment: s/default/current/ here and below.
> > 
> > fixed
> 
> And this fixes the second problem, with this now:
> ffmpeg -bt -1-2-3-4-5-6-7+100
> 
> doesn't issues an out of range value error.

and iam still curious who and why would write
-bt -1-2-3-4-5-6-7+100
in the first place ...


> 
> Also I continue to think that the logics that handle number and flags
> values in av_set_string() should be kept separated (e.g. they should
> be managed in two different blocks).
> 

> [...]
> > > To be able to modify the current value of an option seems anyway a
> > > good feature to keep, so we could use a new keyword "current", so that
> > > for example:
> > > 
> > > ffmpeg -foo current+1
> > 
> > read my previous mail please, that about thinking about "use cases" where
> > something might be usefull first
> 
> I'm not the maintaniner of anything, so I simply propose my ideas, then
> it's duty of the maintainer to decide what to take and what to reject.

yeah but it would be easier if you would either say "ive no clue what this
could be good for" or "It could be used for ..., for which iam not aware of
any reasonable simple way to do it currently"

Simply receiving these patches which no hint on what it could be good for
inevitably leads to "rejected" awnsers ...
I mean if you (who wrote a suggestion or patch) had no clue what something
could be good for, how should i?


> 
> Regards.
> -- 
> FFmpeg = Funny & Fast Majestic Programmable EniGma

> Index: libavcodec/opt.c
> ===================================================================
> --- libavcodec/opt.c	(revision 13281)
> +++ libavcodec/opt.c	(working copy)
> @@ -146,7 +146,7 @@
>          return o;
>      }
>      if(o->type != FF_OPT_TYPE_STRING){
> -        int notfirst=0;
> +        double res=0;
>          for(;;){
>              int i;
>              char buf[256];
> @@ -181,16 +181,15 @@
>                  if     (cmd=='+') d= av_get_int(obj, name, NULL) | (int64_t)d;
>                  else if(cmd=='-') d= av_get_int(obj, name, NULL) &~(int64_t)d;
>              }else{
> -                if     (cmd=='+') d= notfirst*av_get_double(obj, name, NULL) + d;
> -                else if(cmd=='-') d= notfirst*av_get_double(obj, name, NULL) - d;

> +                if     (cmd=='+' || cmd==0) res+= d;
> +                else if(cmd=='-')           res-= d;

that would be if(cmd=='-') and else


>              }
>  
> -            if (!av_set_number(obj, name, d, 1, 1))
> +            if (FF_OPT_TYPE_FLAGS && !(o = av_set_number(obj, name, d, 1, 1)))

?
FF_OPT_TYPE_FLAGS is a constant, what was it that you where trying to do here?


>                  return NULL;
>              val+= i;
>              if(!*val)
> -                return o;
> -            notfirst=1;
> +                return FF_OPT_TYPE_FLAGS? o : av_set_number(obj, name, res, 1, 1);

same issue

which brings me to the point of having to point out that code should be
tested before submitting

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20080524/aab4e920/attachment.pgp>