[FFmpeg-devel] Document AVOption struct

[Stefano Sabatini]

On date Sunday 2008-06-08 16:00:44 +0200, Michael Niedermayer encoded:
> On Sat, Jun 07, 2008 at 10:49:02AM +0200, Stefano Sabatini wrote:
> [...]
> > Since I was at it I did also other modifications, namely:
> > * reworded the description of the return for av_next_option()
> > * reworded the main description of av_get_*, now it sounds like:
> > 
> > Looks for and option in \p ctx and gets its value
> > 
> > rather than:
> > 
> > Gets an option value looked up in \p ctx
> > (the use of the verb "to look up" was wrong).
> > 
> > Also I changed all the return definitions for av_get_*, which were
> > *wrong* before as I wrote them (they don't return negative values, but
> > null values in case of error or if no option has been found), I'm very
> > sorry for this continuos modifications ballet, I think now it should
> > be quite enough correct and accurate.

[...]
> > Index: libavcodec/opt.h
> > ===================================================================
> > --- libavcodec/opt.h	(revision 13684)
> > +++ libavcodec/opt.h	(working copy)
> [...]
> > @@ -45,6 +57,12 @@
> >   * AVOption
> >   */
> >  typedef struct AVOption {
> > +    /**
> > +     * the name of the option<br>
> 
> The name of the option. (The dot is significant syntax wise)

I checked again the Javadoc comments writing guidelines:
http://java.sun.com/j2se/javadoc/writingdoccomments/

effectively they use Capitalization and "." at the end also for
incomplete sentence, so I'm assuming that is the correct style.

> > +     * It should be unique amongst the options contained in an AVClass
> > +     * object. 
> 
> 
> > +     * If the option contains a named value, then this field
> > +     * contains the name of it.
> 
> redundant

Fixed.

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

> > +     * It should be 0 if the option is used to contain a named value.
> 
> named constant

Fixed.

> 
> > +     */
> > +    int offset;
> >      enum AVOptionType type;
> >  
> > +    /**
> > +     * the default value<br>
> > +     * 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.
> > +     */
> >      double default_val;
> 
> > -    double min;
> > -    double max;
> > +    double min;                 ///< minimum acceptable value for the option if the option does not contain a constant
> > +    double max;                 ///< maximum acceptable value for the option if the option does not contain a constant
> 
> valid not acceptable
> and id drop the "if the option does not contain a constant"

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


> 
> 
> >  } AVOption;
> >  
> > +/**
> > + * Looks for an option in \p obj. Looks only for the options which
> 
> > + * have the flags masked with \p mask equal to \p flags (that is for
> > + * which is true: opt->flags & mask = flags).
> 
> urhm ....
>
> have the flags set as specified in mask and flags.
> 
> 
> > + *
> > + * @param[in] obj a pointer to an AVClass object
> > + * @param[in] name the name of the option to look for
> 
> > + * @param[in] unit the unit of the option, if non-NULL only looks for
> > + * options belonging to this unit
> 
> the unit of the option to look for or any if NULL
> where is the wanderer when one needs him :(
> Your documentations are sooooooooooooooooooo confused
> they really could benefit from some heavy reformulations and i hate
> reformulating docs
> 
> 
> > + * @return a pointer to the first option found or NULL if no option
> > + * has been found
> 
> s/first //
> 
> please split the patch! Lets try to get the above svn ready before the
> rest below :)

I splitted the patch which now contains only the comments for the file
and for the AVOption struct, the other ones (updated with your
comments) will eventually belong to a future patch.

Thank you for your review, regards.
-- 
FFmpeg = Furious & Fiendish MultiPurpose ExchanGer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: document-avoption-00.patch
Type: text/x-diff
Size: 3177 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080614/52cd4633/attachment.patch>