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

Matthieu Bouron matthieu.bouron at gmail.com
Mon Jul 22 11:42:43 CEST 2013


On Wed, Jul 17, 2013 at 04:21:16PM +0000, Matthieu Bouron wrote:
> 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.

To avoid all the intrusive code in the mov/mp4 muxer, all apic streams
must not be referenced in the muxer streams array (s->streams) since the
muxer map stream indexes to track indexes.
This means, adding s->apic_streams + s->write_apic_packets to the muxer,
don't break the reference between packets and streams and makes ffmpeg
works with this.
This probably also means adding the same things on the demuxer side.

Do you think this could be an acceptable solution ? Maybe there is a
better way to do it.

Please comment,
Matthieu

[...]


More information about the ffmpeg-devel mailing list