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

Fri Aug 22 19:33:09 CEST 2008

On date Wednesday 2008-05-28 02:17:18 +0200, Stefano Sabatini encoded:
> 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.

Patch updated against latest SVN.

This should eventually fix the weird error reporting for errors of the
kind:
stefano at geppetto ~> ffmpeg -bt foo
FFmpeg version SVN-r14865, Copyright (c) 2000-2008 Fabrice Bellard, et al.
  configuration: --prefix=/home/stefano --disable-shared --enable-libschroedinger --enable-libx264 --enable-libxvid --enable-pthreads --enable-gpl --enable-debug=3 --enable-libtheora --enable-libvorbis --enable-avfilter --enable-libamr-nb --enable-libamr-wb --enable-nonfree --enable-libfaad --enable-libfaac --enable-x11grab --enable-libmp3lame --disable-optimizations --disable-mmx
  libavutil     49.10. 0 / 49.10. 0
  libavcodec    51.68. 0 / 51.68. 0
  libavformat   52.20. 0 / 52.20. 0
  libavdevice   52. 1. 0 / 52. 1. 0
  libavfilter    0. 1. 0 /  0. 1. 0
  built on Aug 20 2008 17:53:40, gcc: 4.2.3 20071014 (prerelease) (Debian 4.2.2-3)
Unable to parse option value "foo": undefined constant or missing (
Unable to parse option value "foo": undefined constant or missing (
Unable to parse option value "foo": undefined constant or missing (
ffmpeg: unrecognized option '-bt'

Regards.
-- 
FFmpeg = Freak and Frightening Minimal Portable Elitarian God
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-av-set-string3-00.patch
Type: text/x-diff
Size: 5413 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080822/c32de0e2/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-opt-default-00.patch
Type: text/x-diff
Size: 1806 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080822/c32de0e2/attachment-0001.patch>