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

[James Darnley]

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?

>> +/**
>> + * 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg_flac-tags_14_r20357M.diff
Type: application/octet-stream
Size: 13842 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091023/5886058d/attachment.obj>