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

[James Darnley]

Updated patch attached.

2009/10/21 Diego Biurrun <diego at biurrun.de>:
>> --- libavformat/oggdec.c      (revision 20337)
>> +++ libavformat/oggdec.c      (working copy)
>> @@ -33,6 +33,7 @@
>>  #include "avformat.h"
>> +#include "vorbiscomment.h"
>
> Why is this necessary?
>
So one can build this file without errors.  But when looking again, I
can't see where this file picked up the old definition in
oggparsevorbis.c

> Spaces around operators would make this more readable.
>
> same here
>
> unnecessarily long line
>
> nit: Add an empty line before the #includes.
>
> Spaces around operators would make this more readable.
>
> see above, add empty line
>
Cosmetics done


2009/10/22 Justin Ruggles <justin.ruggles at gmail.com>:
>>>> 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
>
> It does look like it would be pretty simple with the way you have it
> implemented now.
>
> As for ogg_build_flac_headers(), if duplication of similar code can be
> avoided, that is great, but I don't know that it can be done cleanly in
> this case.
>
Would you prefer to do one commit which adds the writing to ogg and
flac files, or would you prefer them to be separate?  I have removed
the (currently) useless changes to oggenc.c

> Does oggparsevorbis.c use AVMetadataConv after this patch?
>
Haha, no it doesn't, include removed.

>> +int flac_write_block_comment(ByteIOContext *pb, AVMetadata *m, int last_block, int bitexact)
>> +{
>> + ? ?const char *vendor = bitexact ? "ffmpeg" : LIBAVFORMAT_IDENT;
>> + ? ?unsigned int len;
>> + ? ?uint8_t *p, *p0;
>> +
>> + ? ?len = ff_vorbis_comment_length(m, vendor);
>> + ? ?p0 = av_mallocz(len+4);
>> + ? ?if (!p0)
>> + ? ? ? ?return -1;
>
>
> return AVERROR(ENOMEM);
>
Changed.  Should I intercept this value in flac_write_header() then
return it before attempting to write the padding?

>> + ? ?p = p0;
>> +
>> + ? ?bytestream_put_byte(&p, last_block?0x84:0x04);
>> + ? ?bytestream_put_be24(&p, len);
>> + ? ?ff_vorbis_comment_write(&p, m, vendor);
>> +
>> + ? ?put_buffer(pb, p0, len+4);
>> + ? ?av_freep(p0);
>> + ? ?p = NULL;
>
>
> setting p to NULL is pointless here.
>
True, but I recall it being good practice to set pointers to null when freed.

>> ?static int flac_write_header(struct AVFormatContext *s)
>> ?{
>> - ? ?return ff_flac_write_header(s->pb, s->streams[0]->codec);
>> + ? ?int ret;
>> + ? ?/* unsigned int dur = 0; /* see comment below
>> + ? ?av_log(NULL, AV_LOG_ERROR, "[flac tags] duration=%d\n", dur);*/
>
>
> those 2 lines should not be in the final patch.
>
Removed.

>> + ? ?ret = ff_flac_write_header(s->pb, s->streams[0]->codec, 0);
>> + ? ?if (ret)
>> + ? ? ? ?return ret;
>> + ? ?flac_write_block_comment(s->pb, s->metadata, 0, s->streams[0]->codec->flags & CODEC_FLAG_BITEXACT);
>
>
> long line. please wrap at 80 chars.
>
> you could maybe simplify it a little by adding:
> AVCodecContext *codec = s->streams[0]->codec;
> and using that instead of the whole series of dereferences each time.
>
Used your AVCodecContext pointer since it can be used twice.

>> + ? ?flac_write_block_padding(s->pb, 8192, 1);
>> + ? ? ? ?/* flac defaults to placing a seekpoint every 10s,
>> + ? ? ? ? * so one might add padding to allow that later
>> + ? ? ? ? * but there seems to be no simple way to get the duration here
>> + ? ? ? ? * so lets try the flac default of 8192 bytes */
>
>
> please put the comment above the statement it refers to. you can add a
> blank line above if that would make it easier to read. ?"flac default"
> is ambiguous since it does not differentiate between the official Xiph
> commandline FLAC encoder and the FLAC format in general.
>
Changed.  I assume you guys wouldn't approve of it being "flac.exe
defaults to..."


> Index: tests/vsynth.regression.ref
> ===================================================================
> --- tests/vsynth.regression.ref	(revision 20344)
> +++ tests/vsynth.regression.ref	(working copy)
> @@ -217,8 +217,8 @@
>  389386 ./tests/data/a-alac.m4a
>  95e54b261530a1bcf6de6fe3b21dc5f6 *./tests/data/alac.vsynth.out.wav
>  stddev:    0.00 PSNR:999.99 bytes:  1058444/  1058444
> -7781a016edfc242a39e4d65af02d861a *./tests/data/a-flac.flac
> -353368 ./tests/data/a-flac.flac
> +151eef9097f944726968bec48649f00a *./tests/data/a-flac.flac
> +361582 ./tests/data/a-flac.flac
>  95e54b261530a1bcf6de6fe3b21dc5f6 *./tests/data/flac.vsynth.out.wav
>  stddev:    0.00 PSNR:999.99 bytes:  1058444/  1058444
>  26a7f6b0f0b7181df8df3fa589f6bf81 *./tests/data/a-wmav1.asf
Is this how one should change the regression test?  Run the test, see
the change of the md5sum(?) and filesize(?) which get printed, then
alter them in tests/vsynth.regression.ref
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg_flac-tags_12_r20344M.diff
Type: application/octet-stream
Size: 12351 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091022/16ef59eb/attachment.obj>