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

Michael Niedermayer michaelni
Sun Dec 7 14:35:58 CET 2008


On Sun, Dec 07, 2008 at 02:45:16AM +0100, Stefano Sabatini wrote:
> On date Saturday 2008-12-06 17:06:57 +0100, Michael Niedermayer encoded:
> > 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 ...
> 
> I just see the cases "value set", "option found but invalid value",
> "option not found", also I'm not sure how to map these errors
> (AVERRR_INVALIDDATA, AVERROR_NOENT?).

i see plenty more, like value ok but outside allowed range ...
anyway, just see the descriptions from POSIX iam sure you will find
something that matches approximately


> 
> As for the index in the options array, I found its use quite awkward,
> but anyway if you like it I can implement this:
> 
> int av_set_string3(void *obj, const char *name, const char *val);

iam not insisting on this, what i insist on is that we will not have
20 return -1 and parse an error message (or check if its NULL)  to find
out which kind of error it was.


> 
> which returns an AVERROR_* in case of error, or an index i in case of
> success, which can be used to access the obj->options array like this:
> 
> const AVOption* o= obj->options[i];
> 
> Another possibility could be to implement a function which implements
> the setting:
> int av_set_option(AVOption* option, const char *val, int alloc);
> 
> which returns 0 or a corresponding AVERROR_* code. 

this again is extreemly inconvenient

a program MUST be able to
av_set_blah(context, name, value);

not some kind of

opt=av_find_the_option(context, name);
if(opt)
    av_set_blah(opt, value);

the things you suggest make the code really awkward to use for its main
use cases.

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

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20081207/92be0f0f/attachment.pgp>



More information about the ffmpeg-devel mailing list