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

[Justin Ruggles]

James Darnley wrote:

> Updated patch attached for review.
> [...]


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

That works.

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

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.

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


Does oggparsevorbis.c use AVMetadataConv after this patch?

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


It already is.  When we read the tags, they are converted to uppercase,
but that's not even really necessary; it's just a convention used by
some people.  Metadata conversion is already case insensitive since
av_metadata_conv() uses strcasecmp().


> Index: libavformat/oggparsevorbis.c
> ===================================================================
> --- libavformat/oggparsevorbis.c	(revision 20337)
> +++ libavformat/oggparsevorbis.c	(working copy)
> @@ -29,19 +29,8 @@
>  #include "libavcodec/bytestream.h"
>  #include "avformat.h"
>  #include "oggdec.h"
> +#include "vorbiscomment.h"


see comment above.

> +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);

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

> +
> +    return 0;
> +}
> +
>  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.

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

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

> Index: libavformat/vorbiscomment.c
> ===================================================================
[...]


the rest looks good except for the cosmetic stuff Diego mentioned already.

-Justin