[FFmpeg-devel] Document AVOption struct

[Michael Niedermayer]

On Sat, Jun 14, 2008 at 01:34:04AM +0200, Stefano Sabatini wrote:
> On date Sunday 2008-06-08 16:00:44 +0200, Michael Niedermayer encoded:
[...]
> > > +     */
> > >      const char *name;
> > >  
> > >      /**
> > > @@ -52,12 +70,24 @@
> > >       * @todo What about other languages?
> > >       */
> > >      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<br>
> > 
> > it should end in a dot not <br> !
> > <br> says line break, . says end of short explanation at least it was that
> > way last time i RTFM, i dunno if <br> works as well nowadays
> 
> Fixed.
> 
> 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


[...]
> > >      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.


[...]
> 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


>  
>  #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 ...)


>  
>      /**
> -     * 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.


>      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 :)


>      double default_val;

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

ok


>  
>      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

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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20080614/ee04e780/attachment.pgp>