[FFmpeg-devel] [PATCH] lavf/aiffenc: ID3 tags support
Michael Niedermayer
michaelni at gmx.at
Tue Jan 22 20:24:56 CET 2013
On Mon, Jan 21, 2013 at 11:05:26PM +0100, Matthieu Bouron wrote:
> On Mon, Jan 21, 2013 at 09:02:28PM +0000, Paul B Mahol wrote:
> > On 1/15/13, Paul B Mahol <onemda at gmail.com> wrote:
> > > On 1/15/13, Matthieu Bouron <matthieu.bouron at gmail.com> wrote:
> > >> On Fri, Jan 11, 2013 at 02:40:09PM +0100, Matthieu Bouron wrote:
> > >>> On Fri, Jan 11, 2013 at 11:17:46AM +0100, Matthieu Bouron wrote:
> > >>> > On Fri, Jan 11, 2013 at 9:50 AM, Paul B Mahol <onemda at gmail.com>
> > >>> > wrote:
> > >>> > >
> > >>> > > On 1/11/13, Matthieu Bouron <matthieu.bouron at gmail.com> wrote:
> > >>> > > > On Thu, Jan 10, 2013 at 04:43:47PM +0100, Cl??ment B??sch wrote:
> > >>> > > >> On Mon, Jan 07, 2013 at 09:07:17PM +0100, Matthieu Bouron wrote:
> > >>> > > >> [...]
> > >>> > > >> > 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
> > >>> > > >>
> > >>> > > >> > 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;
> > >>> > > >>
> > >>> > > >> Can be moved locally to put_id3v2_tags()
> > >>> > > >
> > >>> > > > Updated in new patch.
> > >>> > > >
> > >>> > > >>
> > >>> > > >> > + 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;
> > >>> > > >> > +
> > >>> > > >>
> > >>> > > >> if (s->metadata)?
> > >>> > > >
> > >>> > > > I remove this check since the user choose or not the write id3
> > >>> > > > tags
> > >>> > > > even if
> > >>> > > > there is no metadata available.
> > >>> > > >
> > >>> > > >>
> > >>> > > >> > + 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 av_copy_packet fails for some reason, you leak pict_list.
> > >>> > > >
> > >>> > > > Updated in new patch.
> > >>> > > >
> > >>> > > >>
> > >>> > > >> > + 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,
> > >>> > > >> > };
> > >>> > > >>
> > >>> > > >> Rest LGTM.
> > >>> > > >>
> > >>> > > >
> > >>> > > > New patch attached, thanks for the review !
> > >>> > >
> > >>> > > Use git send-mail.
> > >>> > >
> > >>> > > Is it ok to write empty id3 chunk?
> > >>> >
> > >>> > Quoting the specification (http://id3.org/id3v2-00):
> > >>> >
> > >>> > 3.2 [...]
> > >>> > A tag must contain at least one frame. A frame must be at least 1 byte
> > >>> > big, excluding the 6-byte header.
> > >>> >
> > >>> > So it seems not to be ok to write an empty id3 chunk.
> > >>> > So i better re-introduce the check in this form if (!s->metadatz &&
> > >>> > !aiff->pict_list) so that covert arts can be written even if there is
> > >>> > no metadata.
> > >>>
> > >>> New patch attached.
> > >>>
> > >>
> > >> Ping
> > >
> > > I dislike implementation design but "as is" patch is probably good enough.
> > >
> >
> > Michael?
>
> I forgot to add a padding byte if the id3 data is not even.
> Patch updated.
>
> Regards,
> Matthieu
>
> [...]
> Makefile | 2
> aiffenc.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 153 insertions(+), 12 deletions(-)
> 6b81c3cf231b4cb555c0b7fffef7b5f9b883c11b 0001-lavf-aiffenc-ID3-tags-support.patch
> From 670fea80f99a368c93fe463e0adb2eedad7dc53e 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
applied
thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130122/23a79cdb/attachment.asc>
More information about the ffmpeg-devel
mailing list