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

[James Darnley]

Updated patch attached for review.

2009/10/21 Justin Ruggles <justin.ruggles at gmail.com>:
> Although the VorbisComment reading function should probably go into
> vorbiscomment.c eventually (or now?), which might simplify the includes
> and the Makefile.
>
That was brought up when I was discussing this on IRC.  I plan to do
it but I had to get the writing working and finished first.

>> I do have a question or two.
>> Is it possible to determine the length of the audio from the
>> AVFormatContext struct? ?I tried a couple of "duration" values but
>> they always seemed to be 0.
>
>
> I don't think this is possible at the muxer level.
>
>> If that is not possible, is there a value in AVFormatContext which
>> could be interpreted as a header padding value, a value which might
>> request N bytes of padding?
>
>
> Not currently.
>
>> I ask because there is also the possibility of writing a
>> METADATA_BLOCK_PADDING to allow extra metadata or possibly a seek
>> table to be added later without the need to rewrite the whole file.
>
> An imperfect alternative might be to reserve space for a fixed number of
> seek table entries and choose the interval at the end based on the final
> duration. ?But that can be a separate patch.
>
Thanks anyway, I made a note of this in a comment and took your
suggestion of using flac's default 8192 bytes.

>> Thanks for any comments.
>
>
> Please update the regression tests.
>
I will look into doing that.

>> Index: libavformat/oggenc.c
>> ===================================================================
>> --- libavformat/oggenc.c ? ? ?(revision 20332)
>> +++ libavformat/oggenc.c ? ? ?(working copy)
>> @@ -25,6 +25,7 @@
>> ?#include "libavcodec/flac.h"
>> ?#include "avformat.h"
>> ?#include "internal.h"
>> +#include "vorbiscomment.h"
>>
>> ?typedef struct {
>> ? ? ?int64_t duration;
>> @@ -357,4 +358,5 @@
>> ? ? ?ogg_write_packet,
>> ? ? ?ogg_write_trailer,
>> ? ? ?.interleave_packet = ogg_interleave_per_granule,
>> + ? ?.metadata_conv = ff_vorbiscomment_metadata_conv,
>> ?};
>
>
> This change would only be needed if VorbisComment writing is added to
> the ogg muxer. ?Which leads to the question... Why is that not done?
>
I was looking into that but saw that it wouldn't as simple as I first
thought.  But now that I write into a buffer instead of an
ByteIOContext, it should be easier.  Another thing I noticed is that I
think the ogg_build_flac_headers() function here is duplicating things
done in flacenc.c.  But that is a subject for another patch

>> Index: libavformat/oggdec.c
>> ===================================================================
>> --- libavformat/oggdec.c ? ? ?(revision 20332)
>> +++ libavformat/oggdec.c ? ? ?(working copy)
>> @@ -33,6 +33,7 @@
>> ?#include <stdio.h>
>> ?#include "oggdec.h"
>> ?#include "avformat.h"
>> +#include "vorbiscomment.h"
>>
>> ?#define MAX_PAGE_SIZE 65307
>> ?#define DECODER_BUFFER_SIZE MAX_PAGE_SIZE
>> Index: libavformat/oggparsevorbis.c
>> ===================================================================
>> --- libavformat/oggparsevorbis.c ? ? ?(revision 20332)
>> +++ libavformat/oggparsevorbis.c ? ? ?(working copy)
>> @@ -29,18 +29,13 @@
>> ?#include "libavcodec/bytestream.h"
>> ?#include "avformat.h"
>> ?#include "oggdec.h"
>> +#include "vorbiscomment.h"
>
>
> why is this #include needed?
>
I thought the include was needed for the AVMetadataConv struct to be
used.  I have also moved the comment as instructed and changed the
keys back to uppercase.

> unneeded parentheses around last_block
>
Fixed the three instances of this.

>> + ? ?bytestream_put_be24(&p, len);
>> + ? ?ff_vorbis_comment_write(&p, m, vendor);
>> +
>> + ? ?put_buffer(pb, p0, len+4);
>> + ? ?//av_freep(p0);
>
>
> is this supposed to be commented-out?
>
I didn't think so but I traced a segfault back to that free.  But with
some more testing it just seems to be cygwin messing with me.
Building native code works fine.  I will test on a "good" OS when I
can.

>> Index: libavformat/vorbiscomment.c
>> ===================================================================
>> --- libavformat/vorbiscomment.c ? ? ? (revision 0)
>> +++ libavformat/vorbiscomment.c ? ? ? (revision 0)
>> @@ -0,0 +1,48 @@
>> +#include "avformat.h"
>> +#include "metadata.h"
>> +#include "vorbiscomment.h"
>> +#include "libavcodec/bytestream.h"
>> +
>> +const AVMetadataConv ff_vorbiscomment_metadata_conv[] = {
>> + ? ?{ "artist" ? ? , "author" },
>> + ? ?{ "date" ? ? ? , "year" ? },
>> + ? ?{ "tracknumber", "track" ?},
>> + ? ?{ 0 }
>> +};
>
>
> as mentioned above, move the documentation for this too. ?I think it
> could probably just be put in the header. ?And don't change the case of
> the key names if you're just moving the table; copy it as-is.
>
Mentioned above.  This raises another question.  How would one
instruct the metadata conversion to be case insensitive for vorbis
comments?  Perhaps it already is.

>> +
>> +int ff_vorbis_comment_length(AVMetadata *m, const char *vendor_string)
>
>
> document this.
>

>> +
>> +int ff_vorbis_comment_write(uint8_t **p, AVMetadata *m, const char *vendor_string)
>
>
> you should probably document this. especially the part about the amount
> of data required for p.
>
Done, please say if you think it is satisfactory or not.


>> +{
>> + ? ?unsigned int len1, len2;
>> + ? ?int i;
>> + ? ?bytestream_put_le32(p, strlen(vendor_string));
>> + ? ?bytestream_put_buffer(p, vendor_string, strlen(vendor_string));
>> + ? ?if (m)
>> + ? ?{
>> + ? ? ? ?bytestream_put_le32(p, m->count);
>> + ? ? ? ?for (i=0; i<m->count; i++)
>> + ? ? ? ?{
>> + ? ? ? ? ? ?len1 = strlen(m->elems[i].key);
>> + ? ? ? ? ? ?len2 = strlen(m->elems[i].value);
>
>
> len1 and len2 can be declared inside the loop.
>
Moved

>> Index: libavformat/vorbiscomment.h
>> ===================================================================
>> --- libavformat/vorbiscomment.h ? ? ? (revision 0)
>> +++ libavformat/vorbiscomment.h ? ? ? (revision 0)
>> @@ -0,0 +1,7 @@
>> +#include "avformat.h"
>> +#include "metadata.h"
>> +
>> +int ff_vorbis_comment_length(AVMetadata *m, const char *vendor_string);
>> +int ff_vorbis_comment_write(uint8_t **p, AVMetadata *m, const char *vendor_string);
>> +
>> +extern const AVMetadataConv ff_vorbiscomment_metadata_conv[];
>
>
> as Diego mentioned, license header and inclusion guards.
>
> -Justin
Done, I think.  I copied the header and #ifdef in flacenc.h and
changed the appropriate sections
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg_flac-tags_11_r20337M.diff
Type: application/octet-stream
Size: 11945 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091021/5f6a1db7/attachment.obj>