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

[Matthieu Bouron]

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.

 [...]