[FFmpeg-devel] [PATCH] Improve documentation for libavutil/base64.h

Michael Niedermayer michaelni
Sun Jan 25 18:10:05 CET 2009


On Sun, Jan 25, 2009 at 02:04:52AM +0100, Stefano Sabatini wrote:
> On date Sunday 2009-01-25 00:27:23 +0100, Michael Niedermayer encoded:
> > On Sat, Jan 24, 2009 at 09:26:43PM +0100, Stefano Sabatini wrote:
> > > Hi,
> > > 
> > > in the patch I'm also changing the parameter names, I think they're
> > > more meaningful now, also I'm applying the distinction between size
> > > (size in bytes of a data buffer) and lenght, intended as the number of
> > > characters in a string *but* the last one, which looks consistent with
> > > the terminology of the C language (sizeof(...), strlen(...)).
> > > 
> > > Regards.
> > > -- 
> > > FFmpeg = Fast and Fantastic Minimal Philosofic Entertaining God
> > 
> > > Index: libavutil/base64.h
> > > ===================================================================
> > > --- libavutil/base64.h	(revision 16736)
> > > +++ libavutil/base64.h	(working copy)
> > > @@ -25,16 +25,26 @@
> > >  #include <stdint.h>
> > >  
> > >  /**
> > > - * decodes base64
> > > - * param order as strncpy()
> > > + * Decode the base-64 string in \p src and put the decoded data in \p
> > > + * dst.
> > 
> > av_base64_decode() decodes the base-64 string in ...
> > not
> > av_base64_decode() decode the base-64 string in ...
> 
> I agree, and this had been already discussed mmhh.. almost two years
> ago and we defined to stick to the Javadoc recommendations, which in
> this specific case recommends the use of the third person forms, 

> but
> most libav* codes uses the impersonal form.

svn blame && flame :)


> 
> Diego could you say which form do you prefer once and for all?  Also
> setting it in the patch rules may help to keep consistency and avoid
> boring discussions.
>   
> > > + *
> > > + * @param dst_size size in bytes of the \p dst buffer, it has to be at
> > > + * least 3/4 of the length of \p src
> > > + * @return the number of bytes written, or a negative value in case of
> > > + * error
> > >   */
> > > -int av_base64_decode(uint8_t * out, const char *in, int out_length);
> > > +int av_base64_decode(uint8_t *dst, const char *src, int dst_size);
> > >  
> > >  /**
> > > - * encodes base64
> > > - * @param src data, not a string
> > > - * @param buf output string
> > > + * Encode in base-64 the data in \p src and put the resulting string
> > > + * in \p dst.
> > > + *
> > 
> > > + * @param dst_len lenght of the \p dst string, it has to be at least
> > > + * \p src_size * 4 / 3 + 12.
> > 
> > From where do you have this number?
> > Is it due tp base64 or our implementation?
> 
> I read it in the implementation, and that limitation didn't made
> sense to me.
> 

> Can you confirm that the attached patch is correct? (At least I can
> confirm that it works with the test program.)

I can confirm that
* it could lead to a sechole if you missed something and i suspect you did
* its more complex
* you did not list any advantage of this change


> 
> > API and implementation are 2 seperate things. The API should
> > document exactly what is needed for both sides to interact, the implementation
> > details should not be documented in any way in the public header.
> 
> OK, just let me note that in this case the implementation details
> caused an unexpected failure.

so you silently change the API to enforce the limits of the implementation
until eternity


[...]
>  /**
> - * encodes base64
> - * @param src data, not a string
> - * @param buf output string
> + * Encodes in base-64 the data in \p src and puts the resulting string
> + * in \p dst.
> + *
> + * @param dst_len lenght of the \p dst string, it has to be at least
> + * 4/3 * N, where N is the smaller multiple of 3 greater than or equal
> + * to \p src_size.

this is even worse, you AGAIN document the current implementation limit
but now its much more complex and even exploitable

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

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20090125/42197cfd/attachment.pgp>



More information about the ffmpeg-devel mailing list