[FFmpeg-devel] [PATCH] Document libavcodec/opt.h

[Stefano Sabatini]

On date Friday 2008-06-06 11:23:37 +0200, Diego Biurrun encoded:
> On Fri, Jun 06, 2008 at 01:08:57AM +0200, Stefano Sabatini wrote:
> > 
> > some cosmetics and other minor improvements, also thanks to Michael it
> > includes now a more complete explanation of av_set_string().
> > 
> > In the case this will be applied, I think should be also a good idea
> > to update the opt.c definitions to match the parameter names I used in
> > this patch.
> 
> Just make a patch for this :)
> 
> > --- libavcodec/opt.h	(revision 13667)
> > +++ libavcodec/opt.h	(working copy)
> > @@ -45,19 +57,36 @@
> >  typedef struct AVOption {
> > +    /**
> > +     * the name of the option, it should be unique amongst the options
> > +     * contained in an AVClass object<br>
> 
> Capitalize, end in a period.

Diego, in this case we have a nominal followed by a complete sentence,
so I'm not really sure which rule to apply.
My solution is:

 * name the name of the option<br>
 * It should be unique amongst the options contained in an AVClass
 * object.

Hope you like it ;-).

[...]
> > +     * the offset to context structure where the option value should
> > +     * be stored, it is 0 if the structure is used to contain a named
> > +     * value
> 
> Capitalize, end in a period.
>
> There are more cases, please review your patch for these changes.

As above...
 
> > @@ -67,22 +96,172 @@
> >  #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 logic unit (for example named
> 
> capitalize, s/logic/logical/

fixed
 
> > +     * values for a flag or an int), if the option contains a named
> 
> Split this long sentence in two at the comma.

fixed
 
> > + * @return a pointer to the first option found or NULL if cannot find
> > + * any option matching the parameters
> 
> This sentence is lacking a subject and thus does not make sense.

I thought was clear that the omitted subject is "the function itself",
but evidently it isn't so clear, so I'm rewording it as:

 * @return a pointer to the first option found or NULL if no option
 * has been found

> > + * @param[in] name the name of the option to be looked for
> 
> to look for, same below

fixed here and below
 
> > + * optionally with a SI postfix or a named scalar. Behaviour with +-
> 
> BehaviOr, please use American English.

fixed
 
> > + * @param[in,out] o_out if non NULL puts at this address the pointer
> > + * to the first option found in \p ctx with name \p name
> 
> non-NULL
> 
> .. put the pointer .. at this address
> 
> > + * @param[in] buf the buffer used for returning non string values as
> 
> non-string

fixed all these

> > + * Shows an header telling the AVClass of \p obj then lists all
> 
> a
> 
> > + * @param[in]  av_log_ctx a pointer to a context which is used for logging the output
> 
> nit: odd spacing

fixed
 
> > + * Sets the default values for the options in \p ctx.  Default values
> 
> The default is one space after periods around here.

fixed

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.

Best regards.
-- 
FFmpeg = Free & Fucked MultiPurpose EntanGlement
-------------- next part --------------
A non-text attachment was scrubbed...
Name: document-libavcodec-opt-h-05.patch
Type: text/x-diff
Size: 10116 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080607/173dad40/attachment.patch>