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

[Justin Ruggles]

James Darnley wrote:

> Updated patch attached.
> 
> 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

They can be separate.  It's pretty clear that the new functions will
work ok for the ogg muxer, but it would still be nice to see a patch for
that.

>>> +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?

Yes, that would be good.

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

it is already done for you.
from libavutil/mem.c:
void av_freep(void *arg)
{
    void **ptr= (void**)arg;
    av_free(*ptr);
    *ptr = NULL;
}

>> 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


yes. and you also need to change rotozoom.regression.ref
I usually like to also run make test with the new values and make sure
everything passes so I don't screw things up by accident and turn Fate
yellow.


>  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...

> +    ret = ff_flac_write_header(s->pb, codec, 0);
> +    if (ret)
> +        return ret;
> +
> +    flac_write_block_comment(s->pb, s->metadata, 0,
> +                             codec->flags & CODEC_FLAG_BITEXACT);
> +
> +    /* the command line flac encoder 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 */


that is fine.

grammar nit-pick: let's

> +
> +int ff_vorbis_comment_write(uint8_t **p, AVMetadata *m, const char *vendor_string)
> +{
> +    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++) {
> +            unsigned int len1, len2;
> +            len1 = strlen(m->elems[i].key);
> +            len2 = strlen(m->elems[i].value);


unsigned int len1 = ...
unsigned int len2 = ...

> +/* returns the number of bytes required by ff_vorbis_comment_write() */
> +int ff_vorbis_comment_length(AVMetadata *m, const char *vendor_string);
> +
> +/* p requires the number of bytes returned by ff_vorbis_comment_length() */
> +int ff_vorbis_comment_write(uint8_t **p, AVMetadata *m, const char *vendor_string);


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.


-Justin