[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