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

Stefano Sabatini stefano.sabatini-lala
Sat Dec 6 14:43:25 CET 2008


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");
> }
> 
> logging the error message with av_log(), which is also consistent with
> the rest of libav*.

Hi, the attached patchset implements this solution, regression test passed.

Now the output will look like:
stefano at geppetto ~/s/ffmpeg> ffmpeg -bt foo
[...]
Unable to parse option value "foo": undefined constant or missing (
Invalid value 'foo' for option 'bt'

stefano at geppetto ~/s/ffmpeg> ffmpeg -bt -1
[...]
Value -1.000000 for parameter 'bt' out of range
Invalid value '-1' for option 'bt'

Regards.
-- 
FFmpeg = Forgiving Fiendish Mind-dumbing Portable Encoding/decoding Governor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-unconsistent-ending-period.patch
Type: text/x-diff
Size: 558 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081206/67e300e5/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-set-number2.patch
Type: text/x-diff
Size: 1508 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081206/67e300e5/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-av-set-string3.patch
Type: text/x-diff
Size: 5950 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081206/67e300e5/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-opt-default.patch
Type: text/x-diff
Size: 1881 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081206/67e300e5/attachment-0003.patch>



More information about the ffmpeg-devel mailing list