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

Justin Ruggles justin.ruggles
Fri Oct 23 00:25:46 CEST 2009


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.

> 2009/10/22 Justin Ruggles <justin.ruggles at gmail.com>:
>>>  static int flac_write_header(struct AVFormatContext *s)
>>>  {
>>> -    return ff_flac_write_header(s->pb, s->streams[0]->codec);
>>> +    int ret;
>>> +    AVCodecContext *codec = s->streams[0]->codec;
>>> +    /* unsigned int dur = 0; /* see comment below
>>> +    av_log(NULL, AV_LOG_ERROR, "[flac tags] duration=%d\n", dur);*/
>>
>> the 2 lines are still there...
>>
> *hurr*  Definitely fixed now
> 
>> 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.

> +/**
> + * 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.

-Justin




More information about the ffmpeg-devel mailing list