[FFmpeg-devel] [PATCH] lavf/aiffenc: ID3 tags support
Matthieu Bouron
matthieu.bouron at gmail.com
Mon Jan 7 21:07:17 CET 2013
On Mon, Jan 07, 2013 at 02:04:44PM +0100, Matthieu Bouron wrote:
> 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.
>
> [...]
Patch updated.
I also added an option to trigger ID3 tags writting (-write_id3v2) as Paul
suggested.
The covert arts are handled with the same stream based logic. Since mp3enc
and wtvenc use this logic, i think it's ok to stick with it until a
cleaner solution is found.
Regards,
Matthieu
-------------- next part --------------
>From a83e6cb83e7a8705e6a1d0e34151a45aaf048245 Mon Sep 17 00:00:00 2001
From: Matthieu Bouron <matthieu.bouron at gmail.com>
Date: Fri, 4 Jan 2013 21:19:40 +0100
Subject: [PATCH] lavf/aiffenc: ID3 tags support
---
libavformat/Makefile | 2 +-
libavformat/aiffenc.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 147 insertions(+), 12 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..41ba761 100644
--- a/libavformat/aiffenc.c
+++ b/libavformat/aiffenc.c
@@ -20,19 +20,62 @@
*/
#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 {
+ const AVClass *class;
int64_t form;
int64_t frames;
int64_t ssnd;
+ int audio_stream_idx;
+ AVPacketList *pict_list;
+ ID3v2EncContext id3v2;
+ int write_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', ' '));
+ 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 +96,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 +192,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 +201,54 @@ 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;
+
+ /* warn only once for each stream */
+ if (s->streams[pkt->stream_index]->nb_frames == 1) {
+ av_log(s, AV_LOG_WARNING, "Got more than one picture in stream %d,"
+ " ignoring.\n", pkt->stream_index);
+ }
+ if (s->streams[pkt->stream_index]->nb_frames >= 1)
+ return 0;
+
+ 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;
+ }
+ }
+
+ 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 +259,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 +270,46 @@ static int aiff_write_trailer(AVFormatContext *s)
/* return to the end */
avio_seek(pb, end_size, SEEK_SET);
+ /* Write ID3 tags */
+ if (aiff->write_id3v2)
+ 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 ENC AV_OPT_FLAG_ENCODING_PARAM
+static const AVOption options[] = {
+ { "write_id3v2", "Enable ID3 tags writing.",
+ OFFSET(write_id3v2), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1, ENC },
+ { "id3v2_version", "Select ID3v2 version to write. Currently 3 and 4 are supported.",
+ OFFSET(id3v2_version), AV_OPT_TYPE_INT, {.i64 = 4}, 3, 4, ENC },
+ { 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 +317,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,
};
--
1.8.1
More information about the ffmpeg-devel
mailing list