[FFmpeg-devel] [PATCH 3/3] lavf/movenc: support iTunes cover art

Matthieu Bouron matthieu.bouron at gmail.com
Wed Jul 17 18:21:16 CEST 2013


On Wed, Jul 17, 2013 at 05:39:18PM +0200, Michael Niedermayer wrote:
> On Tue, Jul 09, 2013 at 12:22:50PM +0200, Matthieu Bouron wrote:
> > On Fri, Jul 05, 2013 at 02:18:57PM +0200, Matthieu Bouron wrote:
> > > On Thu, Jul 04, 2013 at 01:20:56PM +0200, Matthieu Bouron wrote:
> > > > On Wed, Jul 3, 2013 at 10:50 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
> > > > 
> > > > > On Wed, Jul 03, 2013 at 12:08:38PM +0200, Matthieu Bouron wrote:
> > > > > > On Tue, Jul 02, 2013 at 10:43:39PM +0200, Michael Niedermayer wrote:
> > > > > > > On Tue, Jul 02, 2013 at 09:33:04PM +0200, Matthieu Bouron wrote:
> > > > > > > > On Tue, Jul 2, 2013 at 7:46 PM, Michael Niedermayer <
> > > > > michaelni at gmx.at>wrote:
> > > > > > > >
> > > > > > > > > On Sun, Jun 30, 2013 at 04:15:46PM +0200, Matthieu Bouron wrote:
> > > > > > > > > > Cover art muxing is done by introducing the -cover_stream_index
> > > > > option
> > > > > > > > > > which takes an output stream index as argument.
> > > > > > > > > > The stream used for the cover art is not muxed as a track in the
> > > > > > > > > > resulting file.
> > > > > > > > > > ---
> > > > > > > > > >  libavformat/movenc.c     | 157
> > > > > > > > > +++++++++++++++++++++++++++++++++++++++++++----
> > > > > > > > > >  libavformat/movenc.h     |   5 ++
> > > > > > > > > >  libavformat/movenchint.c |   1 +
> > > > > > > > > >  3 files changed, 152 insertions(+), 11 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > > > > > > > > > index 5577530..f411493 100644
> > > > > > > > > > --- a/libavformat/movenc.c
> > > > > > > > > > +++ b/libavformat/movenc.c
> > > > > > > > > > @@ -63,6 +63,7 @@ static const AVOption options[] = {
> > > > > > > > > >      { "ism_lookahead", "Number of lookahead entries for ISM
> > > > > files",
> > > > > > > > > offsetof(MOVMuxContext, ism_lookahead), AV_OPT_TYPE_INT, {.i64 =
> > > > > 0}, 0,
> > > > > > > > > INT_MAX, AV_OPT_FLAG_ENCODING_PARAM},
> > > > > > > > > >      { "use_editlist", "use edit list", offsetof(MOVMuxContext,
> > > > > > > > > use_editlist), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 1,
> > > > > > > > > AV_OPT_FLAG_ENCODING_PARAM},
> > > > > > > > > >      { "video_track_timescale", "set timescale of all video
> > > > > tracks",
> > > > > > > > > offsetof(MOVMuxContext, video_track_timescale), AV_OPT_TYPE_INT,
> > > > > {.i64 =
> > > > > > > > > 0}, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM},
> > > > > > > > > > +    { "cover_stream_index", "video stream index to use for
> > > > > cover art",
> > > > > > > > > offsetof(MOVMuxContext, cover_stream_index), AV_OPT_TYPE_INT,
> > > > > {.i64 = -1},
> > > > > > > > > -1, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM},
> > > > > > > > > >      { NULL },
> > > > > > > > >
> > > > > > > > > isnt AV_DISPOSITION_ATTACHED_PIC enough ?
> > > > > > > > > why is this option needed ?
> > > > > > > > >
> > > > > > > >
> > > > > > > > I guess AV_DISPOSITION_ATTACHED_PIC could be enough.
> > > > > > > > My idea here was to let the user choose the cover art from any output
> > > > > > > > streams he likes with -cover_stream_index. If the input does not
> > > > > have any
> > > > > > > > cover arts, can a custom stream be flagged as an attached pic ?
> > > > > > >
> > > > > > > if it cannot then such feature should be added but certainly doesnt
> > > > > > > belong in a single individual muxer
> > > > > >
> > > > > > You're right, using AV_DISPOSITON_ATTACHED_PIC seems a better solution
> > > > > > than introducing the cover_stream_index.
> > > > > >
> > > > > > Here is a new patch which uses all stream with AV_DISPOSITON_ATTACHED_PIC
> > > > > > as cover arts. It works for all modes which use the iTunes metadata (!3gp
> > > > > > and !mov).
> > > > > >
> > > > > > Actually the cover art codec is only checked at the end (when writing the
> > > > > > metadata). If not BMP, MJPEG or PNG the user is warned that the cover art
> > > > > > is ignored.
> > > > > >
> > > > > > Do you think this check should be included in the new mov_stream_is_apic
> > > > > > function and discard all apic streams which have not the right codec (and
> > > > > > use them as normal stream ? (IMHO it do not feel right to me to use them
> > > > > > as normal stream)).
> > > > >
> > > > > probably not
> > > > >
> > > > > did you test this with subtitles & chapters ?
> > > > >
> > > > 
> > > > I'm currently testing it with chapters and rtphint, i'll send an updated
> > > > patch as soon as all problems are fixed.
> > > > 
> > > 
> > > New patch attached tested with chapters + subtitles + cover art + rtphint.
> > 
> > Rebased patch attached.
> > 
> > Matthieu
> 
> >  movenc.c     |  194 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  movenc.h     |   10 ++-
> >  movenchint.c |   14 +++-
> >  3 files changed, 190 insertions(+), 28 deletions(-)
> > 7dddc2097412a3691adace52efca4d5510b4b655  0001-lavf-movenc-support-iTunes-cover-art.patch
> > From b69af041df927f87735cffb128b6f54550d17800 Mon Sep 17 00:00:00 2001
> > From: Matthieu Bouron <matthieu.bouron at gmail.com>
> > Date: Thu, 27 Jun 2013 18:12:50 +0200
> > Subject: [PATCH] lavf/movenc: support iTunes cover art
> > 
> > Video streams with AV_DISPOSITON_ATTACHED_PIC will be used as cover arts
> > and won't be muxed as normal tracks in the resulting file.
> > ---
> >  libavformat/movenc.c     | 194 +++++++++++++++++++++++++++++++++++++++++------
> >  libavformat/movenc.h     |  10 ++-
> >  libavformat/movenchint.c |  14 +++-
> >  3 files changed, 190 insertions(+), 28 deletions(-)
> > 
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index 42e7c48..92279f5 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -104,6 +104,15 @@ static int is_co64_required(const MOVTrack *track)
> >      return 0;
> >  }
> >  
> > +static int mov_stream_is_apic(MOVMuxContext *mov, AVStream *st)
> > +{
> > +    if ((mov->mode & MODE_3GP) || (mov->mode & MODE_MOV))
> > +        return 0;
> > +    if (st->disposition & AV_DISPOSITION_ATTACHED_PIC)
> > +        return 1;
> > +    return 0;
> > +}
> 
> maybe 3gp/mov AV_DISPOSITION_ATTACHED_PIC should print a warning
> or be treates as error or something
> 
> 
> [...]
> > @@ -3136,12 +3202,17 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
> >  {
> >      MOVMuxContext *mov = s->priv_data;
> >      AVIOContext *pb = s->pb;
> > -    MOVTrack *trk = &mov->tracks[pkt->stream_index];
> > -    AVCodecContext *enc = trk->enc;
> > +    int trk_index = ff_mov_get_track_index(mov, pkt->stream_index);
> > +    MOVTrack *trk;
> > +    AVCodecContext *enc;
> >      unsigned int samples_in_chunk = 0;
> >      int size = pkt->size;
> >      uint8_t *reformatted_data = NULL;
> >  
> 
> > +    if (trk_index < 0) return 0;
> 
> this looks a bit like a hack
> calling a function just to do nothing

If the stream index is not related to an existing track the packet is
discarded (which is the case for attached pic streams).

> 
> also the patch is somewhat intrusive (and ugly) for what it does
> theres no way this can be achived simpler and cleaner ?
> for exmple by seperating the extra stream out in generic code before
> the muxer, but thats just an idea that could allow simplifications
> for other muxers too.

Indeed, the patch is a bit (too much) intrusive since I have to find a way
to discard a specific stream and avoid writing a track for it.
AFAIK, It is basically the only solution without adding a dedicated mechanism
in lavf to handle streams with AV_DISPOSITION_ATTACHED_PIC.

Has some work been started on such mechanism ?

Regards,
Matthieu

[...]


More information about the ffmpeg-devel mailing list