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

[Stefano Sabatini]

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

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

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. 

Regards.
-- 
FFmpeg = Faboulous & Faboulous Mastering Pacific Ecletic Gem