[FFmpeg-devel] [PATCH] Document opt.h:av_set_string functions

[Stefano Sabatini]

On date Wednesday 2008-07-16 18:42:18 +0200, Michael Niedermayer encoded:
> On Tue, Jul 15, 2008 at 11:10:14PM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2008-07-09 01:11:27 +0200, Michael Niedermayer encoded:
> > > On Tue, Jul 08, 2008 at 09:47:47PM +0200, Stefano Sabatini wrote:
> > > > On date Sunday 2008-07-06 11:00:34 +0200, Stefano Sabatini encoded:
> > > > > Hi all, as in subject.
> > > > > -- 
> > > > > FFmpeg = Funny and Fantastic Multimedia Portable Educated Game
> > > > 
> > > [...]
> > > > Please can you comment on this? Or I'll commit it on Saturday.
> > > 
> > > Would you accept a comment like
> > > all your doxy improvments are rejected?
> > > 
> > > It costs me more time fixing them up than writing them from scratch
> > 
> > Michael, I'll try to avoid to write doxy at least not related to my
> > own code (yes it's getting more and more a boring time waste), only
> > please let me end up just the doxyfication of this file which is in my
> > todo list since too much time and I don't want to waste all the effort
> > and comments spent since then.
> > 
> > New try attached.
> > 
> > Thanks for your patience, regards.
> > -- 
> > FFmpeg = Funny & Frenzy Multimedia Powered Experimenting Guru
> 
> > Index: libavcodec/opt.h
> > ===================================================================
> > --- libavcodec/opt.h	(revision 14247)
> > +++ libavcodec/opt.h	(working copy)
> > @@ -99,10 +99,28 @@
> >   */
> >  const AVOption *av_find_opt(void *obj, const char *name, const char *unit, int mask, int flags);
> >  
> > +/**
> > + * @see av_set_string2()
> > + */
> >  attribute_deprecated const AVOption *av_set_string(void *obj, const char *name, const char *val);
> 
> ok
> 
> >  
> >  /**
> > - * Sets the field of obj with the given name to value.
> > + * Sets a field of \p obj with the value in \p val.
> 
> not ok

I think as I wrote it is more correct/clear. See below.

> > + *
> > + * @param[in] obj a pointer to a struct whose first element is a
> > + * pointer to an #AVClass
> 
> a struct whose first element is a pointer to an #AVClass

But is it a pointer no? Also this is inconsistent with what we already
documented in av_find_opt().

> > + * @param[in] name the name of the option contained in \p obj
> > + * corresponding to the field to set.
> 
> the name of the field

I think it's more correct as I wrote, name corresponds to the name of
the AVOption, which can and often *is* different from the name of the
field to set:

Examples:

{"nr", "noise reduction", OFFSET(noise_reduction), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, V|E},
{"rc_init_occupancy", "number of bits which should be loaded into the rc buffer before decoding starts", OFFSET(rc_initial_buffer_occupancy), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, V|E},
{"error", NULL, OFFSET(error_rate), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, V|E},
{"antialias", "MP3 antialias algorithm", OFFSET(antialias_algo), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, 

and many others.

> > + * @param[in] val A string containing the value to parse and set
> > + * in \p obj.
> 
> the value. :)
> If you want, you could say in a seperate sentance that
> If the field is not of a string type, then the given string is parsed.

OK.

> > + * If the option is of numeric type, it has to be a numeric scalar
> > + * optionally with a SI postfix or a named scalar. 
> 
> "SI postfixes and some named scalars are supported"

OK, slightly extended and reformulated, please check it again.

> > Behavior with more
> > + * than one scalar and +- infix operators is undefined.
> 
> ok
> 
> 
> > + * If the option is of type flags, it has to be a numeric scalar or a
> > + * named flag. 
> 
> i agree with the content but not with how its formulated

Reformulated.
 
> > Prefixing a flag with + causes it to be set without
> > + * affecting the other flags,
> 
> OK
> 
> 
> > - similarly unsets a flag.
> 
> similarly '-' unsets a flag.

OK.

> > + * @return a pointer to the option set 
> 
> AVOption
> 
> 
> > or NULL if no option has been
> > + * found \e or the value \p val is not valid
> 
> or NULL if no matching AVOption exists, ...

OK. 

Thanks again for your review, just another little effort and we'll get
to the end, then I'll need a vacation period ;-).

Regards.
-- 
FFmpeg = Faboulous and Frenzy Magical Pitiless Empowered Game
-------------- next part --------------
A non-text attachment was scrubbed...
Name: document-av-set-string-01.patch
Type: text/x-diff
Size: 1613 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080717/125a4724/attachment.patch>