[FFmpeg-devel] [PATCH] lavf/aiffenc: ID3 tags support

Matthieu Bouron matthieu.bouron at gmail.com
Mon Jan 7 14:04:44 CET 2013


On Mon, Jan 07, 2013 at 12:39:26PM +0100, Clément Bœsch wrote:
> On Sun, Jan 06, 2013 at 12:26:24AM +0100, Matthieu Bouron wrote:
> > ---
> >  libavformat/Makefile  |   2 +-
> >  libavformat/aiffenc.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++----
> >  tests/ref/lavf/aiff   |   4 +-
> >  3 files changed, 142 insertions(+), 15 deletions(-)
> > 
> > diff --git a/libavformat/Makefile b/libavformat/Makefile
> > index 2266dee..a60a971 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -60,7 +60,7 @@ OBJS-$(CONFIG_AEA_DEMUXER)               += aea.o pcm.o
> >  OBJS-$(CONFIG_AFC_DEMUXER)               += afc.o
> >  OBJS-$(CONFIG_AIFF_DEMUXER)              += aiffdec.o pcm.o isom.o \
> >                                              mov_chan.o
> > -OBJS-$(CONFIG_AIFF_MUXER)                += aiffenc.o isom.o rawenc.o
> > +OBJS-$(CONFIG_AIFF_MUXER)                += aiffenc.o isom.o id3v2enc.o
> >  OBJS-$(CONFIG_AMR_DEMUXER)               += amr.o
> >  OBJS-$(CONFIG_AMR_MUXER)                 += amr.o
> >  OBJS-$(CONFIG_ANM_DEMUXER)               += anm.o
> > diff --git a/libavformat/aiffenc.c b/libavformat/aiffenc.c
> > index 525bd49..0b360bc 100644
> > --- a/libavformat/aiffenc.c
> > +++ b/libavformat/aiffenc.c
> > @@ -20,19 +20,61 @@
> >   */
> >  
> >  #include "libavutil/intfloat.h"
> > +#include "libavutil/opt.h"
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "aiff.h"
> >  #include "avio_internal.h"
> >  #include "isom.h"
> > -#include "rawenc.h"
> > +#include "id3v2.h"
> >  
> > -typedef struct {
> > +typedef struct AIFFOutputContext {
> 
> Do you need to name the struct?

I probably don't.

> 
> > +    const AVClass *class;
> >      int64_t form;
> >      int64_t frames;
> >      int64_t ssnd;
> > +    int audio_stream_idx;
> > +    AVPacketList *pict_list;
> > +    ID3v2EncContext id3v2;
> > +    int id3v2_version;
> >  } AIFFOutputContext;
> >  
> > +static int put_id3v2_tags(AVFormatContext *s, AIFFOutputContext *aiff)
> > +{
> > +    int ret;
> > +    uint64_t pos, end;
> > +    AVIOContext *pb = s->pb;
> > +    AVPacketList *pict_list = aiff->pict_list;
> > +
> > +    if (!pb->seekable)
> > +        return 0;
> > +
> > +    if (!av_dict_count(s->metadata))
> > +        return 0;
> > +
> > +    avio_wl32(pb, MKTAG('I', 'D', '3', ' '));
> 
> Since the format is big endian based maybe avio_wb32(pb, MKBETAG(...))
> would make more sense.

Makes sense however the rest of the code use the avio_wl32 + MKTAG, so i
prefer to keep it as is.

> 
> > +    avio_wb32(pb, 0);
> > +    pos = avio_tell(pb);
> > +
> > +    ff_id3v2_start(&aiff->id3v2, pb, aiff->id3v2_version, ID3v2_DEFAULT_MAGIC);
> > +    ff_id3v2_write_metadata(s, &aiff->id3v2);
> > +    while (pict_list) {
> > +        if ((ret = ff_id3v2_write_apic(s, &aiff->id3v2, &pict_list->pkt)) < 0)
> > +            return ret;
> > +        pict_list = pict_list->next;
> > +    }
> > +    ff_id3v2_finish(&aiff->id3v2, pb);
> > +
> > +    end = avio_tell(pb);
> > +
> > +    /* Update chunk size */
> > +    avio_seek(pb, pos - 4, SEEK_SET);
> > +    avio_wb32(pb, end - pos);
> > +    avio_seek(pb, end, SEEK_SET);
> > +
> > +    return 0;
> > +}
> > +
> >  static void put_meta(AVFormatContext *s, const char *key, uint32_t id)
> >  {
> >      AVDictionaryEntry *tag;
> > @@ -53,9 +95,26 @@ static int aiff_write_header(AVFormatContext *s)
> >  {
> >      AIFFOutputContext *aiff = s->priv_data;
> >      AVIOContext *pb = s->pb;
> > -    AVCodecContext *enc = s->streams[0]->codec;
> > +    AVCodecContext *enc;
> >      uint64_t sample_rate;
> > -    int aifc = 0;
> > +    int i, aifc = 0;
> > +
> > +    aiff->audio_stream_idx = -1;
> > +    for (i = 0; i < s->nb_streams; i++) {
> > +        AVStream *st = s->streams[i];
> > +        if (aiff->audio_stream_idx < 0 && st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
> > +            aiff->audio_stream_idx = i;
> > +        } else if (st->codec->codec_type != AVMEDIA_TYPE_VIDEO) {
> > +            av_log(s, AV_LOG_ERROR, "Only audio streams and pictures are allowed in AIFF.\n");
> > +            return AVERROR(EINVAL);
> > +        }
> > +    }
> > +    if (aiff->audio_stream_idx < 0) {
> > +        av_log(s, AV_LOG_ERROR, "No audio stream present.\n");
> > +        return AVERROR(EINVAL);
> > +    }
> > +
> > +    enc = s->streams[aiff->audio_stream_idx]->codec;
> >  
> >      /* First verify if format is ok */
> >      if (!enc->codec_tag)
> > @@ -132,7 +191,8 @@ static int aiff_write_header(AVFormatContext *s)
> >      avio_wb32(pb, 0);                    /* Data offset */
> >      avio_wb32(pb, 0);                    /* Block-size (block align) */
> >  
> > -    avpriv_set_pts_info(s->streams[0], 64, 1, s->streams[0]->codec->sample_rate);
> > +    avpriv_set_pts_info(s->streams[aiff->audio_stream_idx], 64, 1,
> > +                        s->streams[aiff->audio_stream_idx]->codec->sample_rate);
> >  
> >      /* Data is starting here */
> >      avio_flush(pb);
> > @@ -140,11 +200,50 @@ static int aiff_write_header(AVFormatContext *s)
> >      return 0;
> >  }
> >  
> > +static int aiff_write_packet(AVFormatContext *s, AVPacket *pkt)
> > +{
> > +    AIFFOutputContext *aiff = s->priv_data;
> > +    AVIOContext *pb = s->pb;
> > +    if (pkt->stream_index == aiff->audio_stream_idx)
> > +        avio_write(pb, pkt->data, pkt->size);
> > +    else {
> > +        int ret;
> > +        AVPacketList *pict_list, *last;
> > +
> 
> > +        if (s->streams[pkt->stream_index]->codec->codec_type != AVMEDIA_TYPE_VIDEO)
> > +            return 0;
> 
> This should not happen, what about a av_assert0()?
> 
> > +
> > +        if (s->streams[pkt->stream_index]->nb_frames >= 1)
> > +            return 0;
> > +
> 
> Please mux the first one and print a warning (think of an animated gif or
> png, where the user hasn't realized there are several images).
> 
> > +        pict_list = av_mallocz(sizeof(AVPacketList));
> > +        if (!pict_list)
> > +            return AVERROR(ENOMEM);
> > +
> > +        if ((ret = av_copy_packet(&pict_list->pkt, pkt)) < 0)
> > +            return ret;
> > +
> > +        if (!aiff->pict_list)
> > +            aiff->pict_list = pict_list;
> > +        else {
> > +            last = aiff->pict_list;
> > +            while (last->next)
> > +                last = last->next;
> > +            last->next = pict_list;
> 
> I may be misunderstanding the code, but why do you build a list if you're
> only storing one picture?

You can put several pictures in the ID3 tags: covert art, waveform, ...

> 
> BTW, can't this code be factorized somewhere with the mp3 muxer, in
> lavf/utils or something?

Not sure if this can be factorized easily since the mp3enc code does not
use an AVPictureList and write directly the apic frames into the stream.

> 
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> >  static int aiff_write_trailer(AVFormatContext *s)
> >  {
> > +    int ret;
> >      AVIOContext *pb = s->pb;
> >      AIFFOutputContext *aiff = s->priv_data;
> > -    AVCodecContext *enc = s->streams[0]->codec;
> > +    AVPacketList *pict_list = aiff->pict_list;
> > +    AVCodecContext *enc = s->streams[aiff->audio_stream_idx]->codec;
> >  
> >      /* Chunks sizes must be even */
> >      int64_t file_size, end_size;
> > @@ -155,10 +254,6 @@ static int aiff_write_trailer(AVFormatContext *s)
> >      }
> >  
> >      if (s->pb->seekable) {
> > -        /* File length */
> > -        avio_seek(pb, aiff->form, SEEK_SET);
> > -        avio_wb32(pb, file_size - aiff->form - 4);
> > -
> >          /* Number of sample frames */
> >          avio_seek(pb, aiff->frames, SEEK_SET);
> >          avio_wb32(pb, (file_size-aiff->ssnd-12)/enc->block_align);
> > @@ -170,12 +265,43 @@ static int aiff_write_trailer(AVFormatContext *s)
> >          /* return to the end */
> >          avio_seek(pb, end_size, SEEK_SET);
> >  
> > +        /* Write ID3 tags */
> > +        if ((ret = put_id3v2_tags(s, aiff)) < 0)
> > +            return ret;
> > +
> > +        /* File length */
> > +        file_size = avio_tell(pb);
> > +        avio_seek(pb, aiff->form, SEEK_SET);
> > +        avio_wb32(pb, file_size - aiff->form - 4);
> > +
> >          avio_flush(pb);
> >      }
> >  
> > +    while (pict_list) {
> > +        AVPacketList *next = pict_list->next;
> > +        av_free_packet(&pict_list->pkt);
> > +        av_freep(&pict_list);
> > +        pict_list = next;
> > +    }
> > +
> >      return 0;
> >  }
> >  
> > +#define OFFSET(x) offsetof(AIFFOutputContext, x)
> > +#define DEC AV_OPT_FLAG_DECODING_PARAM
> > +static const AVOption options[] = {
> > +    { "id3v2_version", "Select ID3v2 version to write. Currently 3 and 4 are supported.",
> > +      OFFSET(id3v2_version), AV_OPT_TYPE_INT, {.i64 = 4}, 3, 4, DEC },
> 
> Decoding param? I believe it should be an encoding flag.

Indeed.

> 
> > +    { NULL },
> > +};
> > +
> > +static const AVClass aiff_muxer_class = {
> > +    .class_name     = "AIFF muxer",
> > +    .item_name      = av_default_item_name,
> > +    .option         = options,
> > +    .version        = LIBAVUTIL_VERSION_INT,
> > +};
> > +
> >  AVOutputFormat ff_aiff_muxer = {
> >      .name              = "aiff",
> >      .long_name         = NULL_IF_CONFIG_SMALL("Audio IFF"),
> > @@ -183,9 +309,10 @@ AVOutputFormat ff_aiff_muxer = {
> >      .extensions        = "aif,aiff,afc,aifc",
> >      .priv_data_size    = sizeof(AIFFOutputContext),
> >      .audio_codec       = AV_CODEC_ID_PCM_S16BE,
> > -    .video_codec       = AV_CODEC_ID_NONE,
> > +    .video_codec       = AV_CODEC_ID_PNG,
> >      .write_header      = aiff_write_header,
> > -    .write_packet      = ff_raw_write_packet,
> > +    .write_packet      = aiff_write_packet,
> >      .write_trailer     = aiff_write_trailer,
> >      .codec_tag         = (const AVCodecTag* const []){ ff_codec_aiff_tags, 0 },
> > +    .priv_class        = &aiff_muxer_class,
> >  };
> > diff --git a/tests/ref/lavf/aiff b/tests/ref/lavf/aiff
> > index 2a5cd81..8b8c489 100644
> > --- a/tests/ref/lavf/aiff
> > +++ b/tests/ref/lavf/aiff
> > @@ -1,3 +1,3 @@
> > -b0d42747a6fc99a5cd1ab0e861671f3a *./tests/data/lavf/lavf.aif
> > -90182 ./tests/data/lavf/lavf.aif
> > +731d863ff976966827a166fa30b32400 *./tests/data/lavf/lavf.aif
> > +90220 ./tests/data/lavf/lavf.aif
> 
> Why is the test changing? A picture is muxed in now?

The title field is set to "lavftest" in the metadata, so it triggers the
ID3 tags writting.

[...]


More information about the ffmpeg-devel mailing list