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

[Michael Niedermayer]

On Sat, May 24, 2008 at 07:56:04PM +0200, Stefano Sabatini wrote:
> On date Saturday 2008-05-24 17:53:55 +0200, Michael Niedermayer encoded:
> > 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:
> [...]
> > > 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 ...
> 
> That was quite a pathological case, anyway who knows?
> 
> And the user could for example want to specify the more mundane
> command:
> ffmpeg -bt -1+default
> 
> then discover with surprise that it doesn't work, while the command
> ffmpeg -bt default-1
> 
> logically equivalent from her point of view works just fine.
> 
> Secondarily: when you're documenting such things, you and the reader
> prefer to read something like this:
> 
> * takes as an argument a string representing a sum of addends, they're
>   added and the result of this sum is set into the value if it
>   respects the value constraints.
> 
> rather than:
> 
> * takes as an argument a string representing a sum of addends, each
>   partial sum is set in the option if it respects the value
>   constraints (so the command will fail even if the total sum is valid
>   but a subtotal cannot be validated against the value constraints,
>   nonetheless the value in the context will result set to the previous
>   valid subtotal).
> 
> The first behaviour is straightforward, the second leads to plenty
> corner cases which lead to obscure bugs/glitches, when it isn't
> documented it is even worse.

Takes an numeric scalar optionally with a SI postfix or a named scalar.
behaviour with +- infix operators is undefined.

Takes an numeric scalar or a named flag. prefixing a flag with + causes
it to be set without affecting the other flags, - similarly unsets a flag.

No corner cases, not messy, not even hard to document.
My question is simply where the extra fearures (which mostly but not
completely work ATM) are usefull.
We have to weight the awnser to this question against the complexity
of a imlpementation of it.

That is of course if you can implement it with no extra complexity then
it would be fine without any explanations why its good for. But so far, 
the suggestions of hash tables and and and ... really are quite a heavy
thing compared to the _complete_ lack of a single realistic use case.
Maybe it is usefull in some cases, but so far noone pointed to any ...



[...]
> > 
> > 
> > >              }
> > >  
> > > -            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
> 
> No one is perfect, indeed I tested but too superficially, I'm going to
> write on my scratch buffer:
> "test thoroughly before to post a patch"
> "test thoroughly before to post a patch"
> "test thoroughly before to post a patch"
> ...
> 
> maybe it will work for the next times ;-)...
> 

> Anyway regression test passed and maybe we should have also a test for
> opt.c/eval.c (tell me and I'll try to hack it).

Well opt.c is used in the current regression tests.
eval.c is not really changed that often and when it is there is code under
#ifdef TEST in there ...


> 
> Regards.
> -- 
> FFmpeg = Fantastic Fast MultiPurpose Exciting Gem

> 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=='-') res-= d;
> +                else              res+= d;
>              }
>  
> -            if (!av_set_number(obj, name, d, 1, 1))
> +            if(o->type == FF_OPT_TYPE_FLAGS && !av_set_number(obj, name, d, 1, 1))
>                  return NULL;
>              val+= i;
>              if(!*val)
> -                return o;
> -            notfirst=1;
> +                return o->type == FF_OPT_TYPE_FLAGS? o : av_set_number(obj, name, res, 1, 1);

This is ugly
all should be set at the same place

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

Democracy is the form of government in which you can choose your dictator
-------------- 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/9755253b/attachment.pgp>