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

[Stefano Sabatini]

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.

> > 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?

Hope that the above explanation clarifyies what was my intention in
this case, also I think my first mail explained sufficiently why I
wanted to change the current behaviour.

[...]

> > 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

fixed.

> 
> 
> >              }
> >  
> > -            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).

Regards.
-- 
FFmpeg = Fantastic Fast MultiPurpose Exciting Gem
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-av-set-string-01.patch
Type: text/x-diff
Size: 1238 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080524/8bcba90d/attachment.patch>