[FFmpeg-devel] Document AVOption struct

[Stefano Sabatini]

On date Saturday 2008-06-14 22:47:33 +0200, Michael Niedermayer encoded:
> On Sat, Jun 14, 2008 at 01:34:04AM +0200, Stefano Sabatini wrote:
[...]
> > Anyway doxygen takes as the brief description the first string up to
> > the first dot *or* up to the first "<br>" encountered.
> 
> IMHO its cleaner if we dont fill C files with html tags

OK and fixed.
 
> [...]
> > > >      int flags;
> > > >  #define AV_OPT_FLAG_ENCODING_PARAM  1   ///< a generic parameter which can be set by the user for muxing or encoding
> > > > @@ -67,22 +97,174 @@
> > > >  #define AV_OPT_FLAG_VIDEO_PARAM     16
> > > >  #define AV_OPT_FLAG_SUBTITLE_PARAM  32
> > > >  //FIXME think about enc-audio, ... style flags
> > > 
> > > > +
> > > > +    /**
> > > > +     * Aggregates options into a single logical unit (for example
> > > > +     * named values for a flag or an int). If the option contains a
> > > > +     * named value then the named value is relative to the option with
> > > > +     * the name equal to \p unit.
> > > > +     */
> > > >      const char *unit;
> > > 
> > > ugh, just remove that doxy please! You can try again in a seperate patch
> > > maybe ...
> > 
> > I tried to improve it, I think now it's simpler and clearer:
> >     /**
> >      * Aggregates options into a single logical unit. Named constants
> >      * belonging to the same option share the same unit, which
> >      * corresponds to the name of that option.
> >      */
> 
> A function does something a variables is something.
> 
> A description of a variable should not be about how it is used but about
> what it is primarely. After one has defined what the variable is/contains
> one can, if needed explain what that is used for if thats is important.

Got it, please check if it is better now.
 
> [...]
> > Index: libavcodec/opt.h
> > ===================================================================
> > --- libavcodec/opt.h	(revision 13767)
> > +++ libavcodec/opt.h	(working copy)
> > @@ -24,7 +24,18 @@
> >  
> >  /**
> >   * @file opt.h
> > - * AVOptions
> > + * Defines the #AVOption system.
> > + * An #AVClass context is a structure which contains <em>as the first
> > + * field</em> a pointer to an #AVClass structure: for example
> > + * #AVCodecContext, #AVFormatContext, #SwsContext are all #AVClass
> > + * structures.<br>
> > + * An #AVClass object is an #AVClass structure \e or an #AVClass
> > + * context.<br>
> > + * Some functions defined here are supposed to act on #AVClass objects
> > + * (see av_find_opt(), av_opt_show() and av_next_option()), while the
> > + * av_set_*, av_get_* and av_opt_set_defaults* functions are supposed
> > + * to act only on #AVClass contexts, so they will not work with mere
> > + * #AVClass structures.
> >   */
> 
> unreadable spagethi
> first id suggest to drop all the html, not everyone uses doxygen to read C
> files

Removed HTML tags here and everywhere, removed the last sentence and
simplified the other ones. I left the definition of AVClass context
and object, because I think they're important in order to understand
how the following functions should be used.

> >  
> >  #include "libavutil/rational.h"
> 
> > @@ -45,28 +56,49 @@
> >   * AVOption
> >   */
> >  typedef struct AVOption {
> > +    /**
> > +     * The name of the option. It should be unique amongst the
> > +     * options contained in an #AVClass object.
> > +     */
> >      const char *name;
> 
> maybe following is better, iam not sure though
> It should be unique within each #AVClass object.
> Also iam not 100% sure but i think names may repeat as long as their
> unit differs. (needs RTFS ...)

Ouch, you're right!

> >      /**
> > -     * short English help text
> > +     * Short English help text.
> >       * @todo What about other languages?
> >       */
> 
> ok, feel free to commit this hunk (also feel free to commit anything else i
> marked as ok)
> 
> 
> >      const char *help;
> > -    int offset;             ///< offset to context structure where the parsed value should be stored
> > +
> 
> > +    /**
> > +     * The offset relative to the context structure where the option
> > +     * value should be stored. 
> 
> "should be stored" is not correct, storing something is not the only thing one
> can do.
> 
> 
> >         It should be 0 if the option is used
> > +     * to contain a named constant.
> > +     */
> > +    int offset;
> 
> Should be 0 for named constants.

OK, corrected here, please check it below.
  
> >      enum AVOptionType type;
> >  
> > +    /**
> > +     * The default value. It is settable only for non string
> > +     * values. If the option is a constant (for example is a named
> > +     * value) then it contains the value of the constant.
> > +     */
> 
> Default value for non constant and value for constant scalars.
> or maybe its "of" instead of "for" iam no native :)

Well, and I think "for" is more clear, I'm no native too so
suggestions are welcome (The Wanderer?).

> >      double default_val;
> 
> > -    double min;
> > -    double max;
> > +    double min;                 ///< Minimum valid value for the option.
> > +    double max;                 ///< Maximum valid value for the option.
> 
> ok

Yes but applied the Diego's rule (an incomplete sentence not followed by
other ones shouldn't be Capitalized and dot-terminated).

> >  
> >      int flags;
> > -#define AV_OPT_FLAG_ENCODING_PARAM  1   ///< a generic parameter which can be set by the user for muxing or encoding
> > -#define AV_OPT_FLAG_DECODING_PARAM  2   ///< a generic parameter which can be set by the user for demuxing or decoding
> > -#define AV_OPT_FLAG_METADATA        4   ///< some data extracted or inserted into the file like title, comment, ...
> > +#define AV_OPT_FLAG_ENCODING_PARAM  1   ///< A generic parameter which can be set by the user for muxing or encoding.
> > +#define AV_OPT_FLAG_DECODING_PARAM  2   ///< A generic parameter which can be set by the user for demuxing or decoding.
> > +#define AV_OPT_FLAG_METADATA        4   ///< Some data extracted or inserted into the file like title, comment, ...
> 
> ok

New patch attached, regards.
-- 
FFmpeg = Free & Frenzy MultiPurpose EntanGlement
-------------- next part --------------
A non-text attachment was scrubbed...
Name: document-avoption-01.patch
Type: text/x-diff
Size: 1985 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080615/3e9ca8ae/attachment.patch>