[FFmpeg-devel] [PATCH] Fix opt_default()

[Michael Niedermayer]

On Sat, Dec 06, 2008 at 02:43:25PM +0100, Stefano Sabatini wrote:
> On date Wednesday 2008-12-03 19:59:20 +0100, Stefano Sabatini encoded:
> > On date Sunday 2008-11-30 23:27:50 +0100, Michael Niedermayer encoded:
> > > On Sun, Nov 30, 2008 at 10:27:01PM +0100, Stefano Sabatini wrote:
> > > > On date Sunday 2008-11-30 21:02:43 +0100, Michael Niedermayer encoded:
> > > > > On Sun, Nov 30, 2008 at 08:22:35PM +0100, Stefano Sabatini wrote:
> > > > [...]
> > > > > > 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
> > > > 
> > > > He can always do:
> > > > AVOption *o = av_set_string3(..., error, error_size);
> > > > if (o && !error)
> > > >    ...
> > > > 
> > > > (yes this way he's forced to pass a non-NULL error...).
> > > > 
> > > > On the other hand if he wants to know for whatever reason if the
> > > > option was found and doesn't care to know if the value has been set or
> > > > not he can do it in just one step.
> > > > 
> > > > But I won't insist on this if you prefer the other way.
> > > 
> > > what about just returning an index into the
> > > AVOption array and a negative value to indicate exactly which error happened?
> > > (thats just an idea, i dont know how this would look in code so maybe it
> > >  ends up ugly ...)
> > 
> > The main problem with this is that as I see it, you need to map each
> > index to an error string, *and* you can't provide specifics
> > information (e.g. "unvalid value: x must be > min and < max").
> > 
> > What about something like:
> > int av_set_string3(void *obj, const char *name, const char *val, const AVOption **o_out);
> > ?
> > 
> > We could use it like:
> > 
> > const AVOption *o;
> > if (av_set_string3(..,  &o) < 0) {
> >    if (!o)
> >       fprintf(stderr, "Option not found\n");
> >    else            
> >       fprintf(stderr, "Option found but invalid value\n");
> > }

id like to repeat that when an integer is returned that should be an error
code according to AVERROR(). i am not ok with this kind of ptr argument
checking to guess which of 2 cases out of 10 apply. Next we would have
to add a sscanf() of the inercepted av_log() output with this design ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- 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/20081206/55e74f5d/attachment.pgp>