[FFmpeg-devel] [PATCH] Add writing of vorbis comments to flac files

[Justin Ruggles]

James Darnley wrote:

> 2009/10/23 Justin Ruggles <justin.ruggles at gmail.com>:
>> James Darnley wrote:
>>
>>> Okay, updated patch attached.  Writing comments to ogg files will be
>>> another patch because while it seems simple to write tags for flac and
>>> speex, I still need to test them.  And if you use vorbis or libvorbis
>>> then it seems the comments are written at libavcodec/vorbis_enc.c:553
>>> and libvorbis.c:99 respecively.
>>
>> The vorbis encoders do not have access to AVMetadata, so they only write
>> an empty comment header with the vendor string.  The Ogg muxer can
>> choose not to use the codec extradata for the comment header and instead
>> substitute its own.
>>
> Yeah, after looking at libavcodec I see that, but thanks for
> confirming.  I will have to look at adapting what the encoder does
> with vorbis to what muxer must do for flac and speex.
> 
>>>> you need to use doxygen comments for the documentation, including
>>>> documentation of each parameter.
>>>> mention that m can be NULL.
>>>> mention that vendor_string cannot be NULL but it can be an empty string.
>>>>
>>> Done, I hope this is the correct format.  I copied from some of the
>>> comments in avformat.h
>>
>> One general note, I think all the comment text should consistently use
>> the term VorbisComment, as that is what Xiph calls the format.
>>
> Okay, I changed these bits to VorbisComment.  I also changed the title
> of the file from "vorbis comment writer" to "VorbisComment writer" to
> be consistent.  Should I change the function names to have
> "VorbisComment" or "vorbiscomment" or leave them as they are?

no mixed case function names. either vorbis_comment or vorbiscomment are
equally ok i think.

>>> +/**
>>> + * Calculates the length in bytes of a vorbis comment.  This is the minimum
>>> + * size required by ff_vorbis_comment_write().
>>> + * @param m The metadata structure you want to be used. For no metadata,
>>> + * set to NULL.
>>> + * @param vendor_string The vendor string you want to be used.  For no string,
>>> + * set to an empty string.
>>> + * @return The length in bytes
>>> + */
>>> +int ff_vorbis_comment_length(AVMetadata *m, const char *vendor_string);
>>> +
>>> +/**
>>> + * Writes the vorbis comment into the buffer pointed to by *p.  The metadata
>>> + * struct and vendor_strings should no longer than those used in
>>> + * ff_vorbis_comment_length()
>>
>> I would suggest something a little clearer. Maybe something like, "The
>> buffer, p, must have enough data to hold the entire VorbisComment
>> header.  You can obtain the minimum size required by passing the same
>> AVMetadata and vendor_string to ff_vorbis_comment_length()."
>>
>> Also, using "want to be used" doesn't really fit well and is not
>> consistent with how we document other function parameters in FFmpeg.
>>
> Yes, I think that poor piece of English resulted from editing it too
> many times while trying to remain concise.  I looked at some other
> documented parameters and tried to copy their style.  If you have some
> specific text you would prefer to be used, please say so.

Your revised text looks fine to me.

> +int ff_vorbis_comment_write(uint8_t **p, AVMetadata *m,
> +                            const char *vendor_string)
> +{
> +    int i;
> +    bytestream_put_le32(p, strlen(vendor_string));
> +    av_log(NULL, AV_LOG_ERROR, "[VorbisComment] %d %s\n", strlen(vendor_string), vendor_string);

looks like this line slipped in by accident...

> +/**
> + * Writes a VorbisComment into a buffer. The buffer, p, must have enough
> + * data to hold the whole VorbisComment. The minimum size required can be
> + * obtained by passing the same AVMetadata and vendor_string to
> + * ff_vorbis_comment_length()
> + *
> + * @param p The buffer in which to write.
> + * @param m The metadata struct you want to write.
> + * @param vendor_string The vendor string you want to write.
> + */


This function documentation needs some similar editing to what you did
to the other.

-Justin