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

Michael Niedermayer michaelni at gmx.at
Wed Jul 17 17:39:18 CEST 2013


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

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.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20130717/22210401/attachment.asc>


More information about the ffmpeg-devel mailing list