[FFmpeg-devel] [PACTH] Fix another problem in av_set_string()

[Michael Niedermayer]

On Tue, May 13, 2008 at 11:11:29PM +0200, Stefano Sabatini wrote:
> On date Tuesday 2008-05-13 13:16:25 +0200, Michael Niedermayer encoded:
> > On Tue, May 13, 2008 at 11:38:34AM +0200, Stefano Sabatini wrote:
> > > On date Tuesday 2008-05-13 10:18:47 +0200, Michael Niedermayer encoded:
> > > > On Tue, May 13, 2008 at 09:38:58AM +0200, Stefano Sabatini wrote:
> > > > > Hi all,
> > > > > 
> > > > > since evaluation occurs on the buffer buf:
> > > > > d = ff_eval2(buf, const_values, const_names, NULL, NULL, NULL, NULL, NULL, &error);
> > > > > 
> > > > > then avlog should report it when telling there was a parsing error here:
> > > > > if (error)
> > > > >    av_log(NULL, AV_LOG_ERROR, "Unable to parse option value \"%s\": %s\n", val, error);
> > > > >                                                                            ^^^
> > > > > while the val contains the remaining part of the argument string still
> > > > > to be evaluated.
> > > > > 
> > > > > This pach does s/val/buf/ in the abovementioned line.
> > > > 
> > > > ive rejected this patch already
> > > > 
> > > > the bug is that val+=i is done too early.
> > > 
> > > mmh.. got it, please check this one.
> > 
> > rejected
> 
> I think you rejected the patch because it didn't follow strictly
> your suggestion (to move the position of val+=i), so I'm attaching a
> patch corresponding to what I suppose is your preferred solution (or
> maybe you're just annoyed by my profusion of trivial patches with
> corresponding trivial errors, who knows?).
> 
> Nonetheless I think that my solution issues clearer output (the whole
> argument value is output, as opposed to just the misbehaving chop

The user knows the whole argment, after all he wrote it likely a second
ago, what he does not know is where parsing failed.

Also patches are suposed to be minimal and split in seperate parts
a bugfix and a change completely changing what is output definitly
do not belong in the same patch.
[...]

> Index: libavcodec/opt.c
> ===================================================================
> --- libavcodec/opt.c	(revision 13150)
> +++ libavcodec/opt.c	(working copy)
> @@ -159,7 +159,6 @@
>              for(i=0; i<sizeof(buf)-1 && val[i] && val[i]!='+' && val[i]!='-'; i++)
>                  buf[i]= val[i];
>              buf[i]=0;
> -            val+= i;
>  
>              d = ff_eval2(buf, const_values, const_names, NULL, NULL, NULL, NULL, NULL, &error);
>              if(isnan(d)) {
> @@ -184,6 +183,7 @@
>                  d= -d;
>  
>              av_set_number(obj, name, d, 1, 1);
> +            val+= i;
>              if(!*val)
>                  return o;
>          }

ok

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

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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/20080514/c02c37a8/attachment.pgp>