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

[Justin Ruggles]

James Darnley wrote:

> Hello,
> I have attached a patch which adds the creation of vorbis comments
> from AVMetadata and the writing of this as a
> METADATA_BLOCK_VORBIS_COMMENT in a flac file.

Thank you for sending the patch.

> Could someone please double check libavformat/makefile and the
> #includes on the files to eliminate any redundant options or to add
> any I have missed.


Although the VorbisComment reading function should probably go into
vorbiscomment.c eventually (or now?), which might simplify the includes
and the Makefile.

> 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 for any comments.


Please update the regression tests.

> 

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

> Index: libavformat/matroskaenc.c
> ===================================================================
> --- libavformat/matroskaenc.c	(revision 20332)
> +++ libavformat/matroskaenc.c	(working copy)
> @@ -462,7 +462,7 @@
>          if (codec->codec_id == CODEC_ID_VORBIS || codec->codec_id == CODEC_ID_THEORA)
>              ret = put_xiph_codecpriv(s, dyn_cp, codec);
>          else if (codec->codec_id == CODEC_ID_FLAC)
> -            ret = ff_flac_write_header(dyn_cp, codec);
> +            ret = ff_flac_write_header(dyn_cp, codec, 1);
>          else if (codec->codec_id == CODEC_ID_H264)
>              ret = ff_isom_write_avcc(dyn_cp, codec->extradata, codec->extradata_size);
>          else if (codec->extradata_size)


This looks ok.

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

>  
>  /**
>   * VorbisComment metadata conversion mapping.
>   * from Ogg Vorbis I format specification: comment field and header specification
>   * http://xiph.org/vorbis/doc/v-comment.html
>   */
> -const AVMetadataConv ff_vorbiscomment_metadata_conv[] = {
> -    { "ARTIST"     , "author" },
> -    { "DATE"       , "year"   },
> -    { "TRACKNUMBER", "track"  },
> -    { 0 }
> -};


You should move the documentation along with the table.


> Index: libavformat/flacenc.c
> ===================================================================
> --- libavformat/flacenc.c	(revision 20332)
> +++ libavformat/flacenc.c	(working copy)
> @@ -22,15 +22,20 @@
>  #include "libavcodec/flac.h"
>  #include "avformat.h"
>  #include "flacenc.h"
> +#include "metadata.h"
> +#include "vorbiscomment.h"
> +#include "libavcodec/avcodec.h"


you don't need to include avcodec.h explicitly.

> +#include "libavcodec/bytestream.h"
>  
> -int ff_flac_write_header(ByteIOContext *pb, AVCodecContext *codec)
> +int ff_flac_write_header(ByteIOContext *pb, AVCodecContext *codec, int last_block)
>  {
> -    static const uint8_t header[8] = {
> -        0x66, 0x4C, 0x61, 0x43, 0x80, 0x00, 0x00, 0x22
> +    uint8_t header[8] = {
> +        0x66, 0x4C, 0x61, 0x43, 0x00, 0x00, 0x00, 0x22
>      };
>      uint8_t *streaminfo;
>      enum FLACExtradataFormat format;
>  
> +    header[4] = (last_block)?0x80:0x00;
>      if (!ff_flac_is_extradata_valid(codec, &format, &streaminfo))
>          return -1;
>  
> @@ -45,9 +50,58 @@
>      return 0;
>  }


This change looks ok.

>  
> +int flac_write_block_padding(ByteIOContext *pb, unsigned int n_padding_bytes, int last_block)
> +{
> +    put_byte(pb, (last_block)?0x81:0x01);


unneeded parentheses around last_block

> +    put_be24(pb, n_padding_bytes);
> +    while(n_padding_bytes>0)
> +    {
> +        put_byte(pb, 0);
> +        n_padding_bytes--;
> +    }
> +    return 0;
> +}
> +
> +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;
> +    p = p0;
> +
> +    bytestream_put_byte(&p, (last_block)?0x84:0x04);


unnecessary parentheses.

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

> +    p = NULL;
> +
> +    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 = ((s->duration)); // * s->streams[0]->time_base.den) / s->streams[0]->time_base.num);
> +    /* seems to be zero in my testing */
> +    av_log(NULL, AV_LOG_ERROR, "[flac tags] duration=%d\n", dur);


as you mentioned, this doesn't work.

> +    ret = ff_flac_write_header(s->pb, s->streams[0]->codec, 0);
> +    if (ret)
> +        return ret;
> +    if (dur>0)
> +    {
> +        flac_write_block_comment(s->pb, s->metadata, 0, s->streams[0]->codec->flags & CODEC_FLAG_BITEXACT);
> +        flac_write_block_padding(s->pb, (dur/10)*18, 1);


The default padding for the reference flac encoder is 8192 bytes.  That
would allow for a pretty large seek table if needed.

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

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


document this.

> +{
> +    int len = 8;
> +    int i;
> +    len += strlen(vendor_string);
> +    if (m)
> +    {
> +        len += m->count * 4;
> +        for (i=0; i<m->count; i++)
> +            len += strlen(m->elems[i].key) + 1 + strlen(m->elems[i].value);
> +    }
> +    return len;
> +}


this looks ok.

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

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

> +            bytestream_put_le32(p, len1+1+len2);
> +            bytestream_put_buffer(p, m->elems[i].key, len1);
> +            bytestream_put_byte(p, '=');
> +            bytestream_put_buffer(p, m->elems[i].value, len2);
> +        }
> +    } else
> +        bytestream_put_le32(p, 0);
> +    return 0;
> +}
> 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