[FFmpeg-devel] [PATCH] Enhance ffmpeg.c:opt_default()

[Stefano Sabatini]

On date Monday 2008-05-19 03:26:22 +0200, Michael Niedermayer encoded:
> On Sun, May 18, 2008 at 09:42:19PM +0200, Stefano Sabatini wrote:
> > On date Sunday 2008-05-18 15:39:06 +0200, Michael Niedermayer encoded:
> > > On Sun, May 18, 2008 at 01:44:36AM +0200, Stefano Sabatini wrote:
> > > > On date Saturday 2008-05-17 19:57:43 +0200, Michael Niedermayer encoded:
> > [...]
> > > > > an optional 
> > > > > av_log(context, AV_LOG_ERROR, error, param, arg, opt.min, opt.max);
> > > > 
> > > > In this way you have to know how is made the returned string, boring
> > > > since you have to document it and not very flexible for example if you
> > > > want to change the wording of the message thus the ordering of the
> > > > parameters you break it, furthermore if you want to support more than
> > > > one message (for example: "The value for '%s' was '%f' which isn't an
> > > > integer", only two params) it can't work anymore.
> > > 
> > > Go read ISO C and POSIX, i could explain how printf() works but ffmpeg-dev
> > > really isnt the correct place for this, the docs are clear and
> > > understandable.
> > [...] 
> > > > > vs.
> > > > > 
> > > > > an optional 
> > > > > av_log(context, AV_LOG_ERROR, error);
> > > > > and a mandatory
> > > > > av_free(error);
> > > > > 
> > > > > Honestly i dont see how the first is worse, and of course if someone has a
> > > > > better idea than these 2 thats welcome as well.
> > > > 
> > > > Patches attached corresponding to the second approach, the first one
> > > > maybe should go in a separate thread.
> > > 
> > > patches rejected, we will not malloc error messages, the memleaks alone
> > > are reason enough why not.
> > 
> > At this point I'm defeated, I can't see any other ways to resolve the
> > problem but:
> > 
> > 1) use av_log to show the error occurred
> 
> That is an option

What about a:
const AVOption *av_set_string2(void *obj, const char *name, const char *val, int *error_ptr);

which prints the error message with av_log and set error_ptr to 1 in
case of errors when setting the option?

In this way we can still have contextual messages, e.g.:
Value '-10' for parameter 'foo' is not within 0 - inf
Value 'foo' for parameter 'bar' is unparsable: not a constant or missing (

Possibly it would also be better to *not* specify the option name,
this because the external name of the option (e.g. "sb") could not
correspond to the name of the option in a context ("b" in the subtitle
options context).

> > 2) allocate a string containing the error in av_set_strin2() and
> > mandate the user to free it
> 
> That is not an option
> 
> 
> > 
> > If someone can find a solution corresponding to:
> > 
> > 3) use a static parametric string containing enough parameter to show
> 
> I already suggested that you read the standards about printf()
> If you did and still have no clue ill explain how it can be done
> but i wont explain it if you are just to lazy to look it up ...
> 
> 
> > the various possible error messages:
> 
> > Value '%s' for parameter '%s' out of range
> > Value '%s' for parameter '%s' is not within %f - %f\n"
> 
> redundant
> 
> 
> > Value '%s' for parameter '%s' unparsable: %s\n"
> 
> Isnt there one %s too much in there ?
> 
> 
> > ... possibly others to be added in the future
> 
> About which we can think in the future ...
> 
> 
> > 
> > and a method to correctly fill them outside of av_set_string2() (where
> > for example the parsing error messages isn't accessible anymore) 
> 
> Here you return the parsing error message

I still continue to dislike this solution, since it is difficult to
extend it, also in this way it isn't possible to express the error
message returned by ff_eval (the best you can say is:
error = Value '%s' for parameter '%s' unparsable
av_log(NULL, AV_LOG_ERROR, "%s\n", name, val, o.min, o.max);

(Yes I know that in a printf format the list of actual parameters may
be longer than the list of the format parameter:
e.g. you can have:
printf("Value '%s' for parameter '%s' unparsable\n", val, opt, o.min, o.max);
)

So I'm proposing this other solution which makes av_set_string2 set a
pointer to a static string, solution which has the disadvantage that
it doesn't let to express a contextual error, e.g.:
"Value '-10' for parameter 'foo' is not within 0 - inf"

but only the generic error occurred (corresponding to a static
string), e.g.:
"out of range value"

On the other hand this is also symmetric with the behaviour of
ff_eval(), which only returns the generic error encountered and
doesn't tell *where* the parsing error occurred.

Also I don't think this is a real problem, for example we could extend
av_opt_show() to show the validity range of each value, or we could
even implement a sort of av_show_opt(const AVOption *opt,... ) which
prints a description of an option with all the set constraints (which
could be called when av_set_string2 fails with a setting error, or
when the val for av_set_string2() is "help").

Here it is a collection of error messages with these patches applied:
Invalid value '-mv4+obmc+qpel+foo-trell+none' for option 'flags': undefined constant or missing (
Invalid value '-+obmc+bar+qpel+foo-trell+none' for option 'flags': undefined constant or missing (
Invalid value 'foo+1' for option 'b': undefined constant or missing (
Invalid value '100*foo-1' for option 'b': undefined constant or missing (
Invalid value '-100' for option 'bt': out of range value
Invalid value 'foo' for option 'bt': undefined constant or missing (
Invalid value '1000+foo' for option 'bt': undefined constant or missing (

Sorry for the long mail, regards.
-- 
FFmpeg = Foolish Fantastic MultiPurpose EnGine
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-av-set-string2-03.patch
Type: text/x-diff
Size: 4231 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080528/c6b9cf40/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: enhance-opt-default-05.patch
Type: text/x-diff
Size: 1764 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080528/c6b9cf40/attachment-0001.patch>