[FFmpeg-devel] [PATCH] Document opt.h:av_find_opt()

[Michael Niedermayer]

On Thu, Jul 03, 2008 at 03:06:31PM +0200, Stefano Sabatini wrote:
> On date Wednesday 2008-07-02 18:02:03 +0200, Stefano Sabatini encoded:
> > On date Wednesday 2008-07-02 16:56:36 +0200, Michael Niedermayer encoded:
> > > On Wed, Jul 02, 2008 at 11:31:46AM +0200, Stefano Sabatini wrote:
> > > > On date Tuesday 2008-07-01 16:48:47 +0200, Stefano Sabatini encoded:
> > > > > On date Tuesday 2008-07-01 02:55:13 +0200, Michael Niedermayer encoded:
> > > > > > On Tue, Jul 01, 2008 at 01:32:10AM +0200, Stefano Sabatini wrote:
> > > > > > > On date Monday 2008-06-30 19:14:24 -0400, The Wanderer encoded:
> > > > > > > > Stefano Sabatini wrote:
> > > > > > > > 
> > > > > > > > > Hi all,
> > > > > > > > > continues the opt.h documentation saga.
> > > > > > > > > 
> > > > > > > > > I took account in this patch to all the previous remarks done by
> > > > > > > > > Diego, Michael, The Wanderer as much as my mind made it possible ;-).
> > > > > > > > 
> > > > > > > > Not bad, but I see a couple of possible improvements. (This is almost on 
> > > > > > > > the level of nitpicking, but I points 'em out as I sees 'em...)
> > > > > > > > 
> > > > > > > > > + * Looks for an option in \p obj. Looks only for the options which
> > > > > > > > > + * have the flags set as specified in \p mask and \p flags (that is,
> > > > > > > > > + * for which is: opt->flags & mask == flags).
> > > > > > > > 
> > > > > > > > I would probably say something more like "for which it is the case that"
> > > > > > > > (or, less precisely but more simply, just "for which") and drop the
> > > > > > > > colon.
> > > > > > > > 
> > > > > > > > > + * @param[in] obj a pointer to an #AVClass struct or to an #AVClass
> > > > > > > > > + * context struct
> > > > > > > > > + * @param[in] name the name of the option to look for
> > > > > > > > > + * @param[in[ unit the unit of the option to look for or any if NULL
> > > > > > > > > + * @return a pointer to the option found or NULL if no option
> > > > > > > > > + * has been found
> > > > > > > > 
> > > > > > > > I would add a comma before the "or" on both of these last two.
> > > > > > > 
> > > > > > > Mmh... this still continues to seem strange to me, nonetheless I
> > > > > > > applied all your suggestions.
> > > > > > > 
> > > > > > > > Other than that, looks fairly good to me.
> > > > > > > 
> > > > > > > Thanks for the review, new patch attached.
> > > > > > > -- 
> > > > > > > FFmpeg = Fantastic Fostering Maxi Pitiless Exploitable Guru
> > > > > > 
> > > > > > > Index: libavcodec/opt.h
> > > > > > > ===================================================================
> > > > > > > --- libavcodec/opt.h	(revision 14043)
> > > > > > > +++ libavcodec/opt.h	(working copy)
> > > > > > > @@ -85,6 +85,18 @@
> > > > > > >  } AVOption;
> > > > > > >  
> > > > > > >  
> > > > > > > +/**
> > > > > > > + * Looks for an option in \p obj. Looks only for the options which
> > > > > > > + * have the flags set as specified in \p mask and \p flags (that is,
> > > > > > > + * for which it is the case that opt->flags & mask == flags).
> > > > > > > + *
> > > > > > 
> > > > > > > + * @param[in] obj a pointer to an #AVClass struct or to an #AVClass
> > > > > > > + * context struct
> > > > > > 
> > > > > > Whats the difference between a "#AVClass struct" and a 
> > > > > > "#AVClass context struct" ?
> > > > > 
> > > > > The AVClass context struct(ure) is defined in libavutil/log.h as
> > > > > follows:
> > > > > /**
> > > > >  * Describes the class of an AVClass context structure, that is an
> > > > >  * arbitrary struct of which the first field is a pointer to an
> > > > >  * AVClass struct (e.g. AVCodecContext, AVFormatContext etc.).
> > > > >  */
> > > > > typedef struct AVCLASS AVClass;
> > > > > 
> > > > > (yes I know, this definition is somehow circular...)
> > > > 
> > > > If no one has further comments on this I'll apply after three days.
> > > 
> > > I object to anything that matches "AVClass.*struct"
> > 
> > What about s/AVClass.*struct/AVClass/?
> > 
> > /**
> >  * Describes the class of an AVClass context, that is an
> >  * arbitrary struct of which the first field is a pointer to an
> >  * AVClass (e.g. AVCodecContext, AVFormatContext etc.).
> >  */
> > 
> >  * @param[in] obj a pointer to an #AVClass or to an #AVClass
> >  * context
> 
> If that's OK (no explicit objections or further comments) I'll commit
> both modifications (log.h and opt.h) after three days.

I object
for exampe to "or" in the description, there are no 2 different things.
I also object to AVClass.*context
I do not know what all these AVClass structs contextes and classes are
supposed to be and if I as a developer working with the code am confused
by the terminology iam sure a person not used to the stuff will be as
well.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20080703/88085b27/attachment.pgp>