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

[Michael Niedermayer]

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:
> > On Sat, May 17, 2008 at 06:49:11PM +0200, Stefano Sabatini wrote:
> > > On date Saturday 2008-05-17 17:04:02 +0200, Michael Niedermayer encoded:
> > > > On Sat, May 17, 2008 at 04:38:26PM +0200, Stefano Sabatini wrote:
> > > > > On date Saturday 2008-05-17 16:14:32 +0200, Michael Niedermayer encoded:
> > > > > > On Sat, May 17, 2008 at 12:24:58PM +0200, Stefano Sabatini wrote:
> > > > > > > On date Friday 2008-05-16 18:24:11 +0200, Michael Niedermayer encoded:
> > > > > > > > On Fri, May 16, 2008 at 01:57:09AM +0200, Stefano Sabatini wrote:
> > > > > [...]
> > > > > > > > > Index: libavcodec/opt.c
> > > > > [...]
> > > > > > > > > -static const AVOption *av_set_number(void *obj, const char *name, double num, int den, int64_t intnum){
> > > > > > > > > +static const AVOption *av_set_number2(void *obj, const char *name, double num, int den, int64_t intnum, const char **error_ptr){
> > > > > > > > >      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
> > > > > > > > > +    static char error[128];
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > rejected
> > > > > > > 
> > > > > > > Well I know it sucks like a solution, what about to allocate the error
> > > > > > > buffer and leave to the av_set_string2() user to free it?
> > > > > > 
> > > > > > RTFS eval.c
> > > > > 
> > > > > Yes, indeed I started from there, the difference is that while in
> > > > > eval.c the error string is set to a statically allocated string, I
> > > > > would like in this case to return an error message with a variable
> > > > > content, e.g.:
> > > > > 
> > > > > "The value for 'foo' was -1 which is not within 0 - inf\n";
> > > > 
> > > > "The value for '%s' was %f which is not within %f - %f\n"
> > > > 
> > > > still static const ...
> > > 
> > > Yes, but then you need to implement the logic to interpret the
> > > parametric string outside av_set_number2(), which seems pretty nasty
> > > to me.
> > 
> > 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.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- 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/20080518/dc245513/attachment.pgp>