[FFmpeg-devel] [PATCH] attachments support in matroska demuxer

[Michael Niedermayer]

On Thu, Jan 17, 2008 at 01:00:26AM +0100, Aurelien Jacobs wrote:
> Evgeniy Stepanov wrote:
> 
> > On Thursday 03 January 2008 04:23:09 Michael Niedermayer wrote:
> > > On Thu, Jan 03, 2008 at 01:38:39AM +0100, Aurelien Jacobs wrote:
> > > > On Sat, 29 Dec 2007 22:38:24 +0300
> > > > Evgeniy Stepanov <eugeni.stepanov at gmail.com> wrote:
> > >
> > > [...]
> > >
> > > > Conclusions:
> > > > - Attachment really looks like a stream itself. You can select
> > > >   the one you want to remux. For example the film poster would
> > > >   just be a jpeg stream with a single picture. The specificity
> > > >   of those streams would be:
> > > >
> > > >     * contain one single packet
> > > >     * not pts
> > >
> > > Contain no packets, just one global extradata, this was proposed
> > > already. If you use normal packets there will be problems with them
> > > disapearing if a stream is split timewise or if you seek before
> > > demuxing the first packet.
> > > Iam not completely happy with this use of streams but we will need
> > > some solution and maybe its the least ugly, we will see ...
> > >
> > > at least iam pretty sure extradata to be a better choice than 1
> > > normal packet
> > >
> > > >     * has a filename (important so that an ASS decoder can
> > > >       select the proper ttf based on it's name)
> > > >
> > > >     * always demuxed before other (normal) streams (IIRC)
> > >
> > > extradata should be available before packets ...
> > >
> > > >   It seems to me that attachment would fit pretty well, each one
> > > >   in its own AVStream. Does this sound reasonable ?
> > > > - Here is how remuxing to other formats (avi, mov...) could work:
> > > >     * supported codec (jpeg poster) could be muxed as a normal
> > > >       JPEG stream with only one picture
> > > >     * ttf streams should be written in a separate file (that's
> > > >       the way various players expect to find ttf fonts when
> > > >       demuxing AVI, IIRC)
> > > >
> > > > - I think the only required API modifications to support this
> > > >   would be:
> > > >     * adding AVStream.filename and maybe
> > > > AVStream.id_of_link_stream.
> > > >     * adding CODEC_ID_TTF ? (ugly, but shouldn't cause much
> > > > trouble)
> > > >     * adding a way to mux ttf font in a separate file (maybe use
> > > >       a separate muxer for this, ie. call 2 different muxers from
> > > >       ffmpeg. seems ugly, but I've no better idea right now).
> > >
> > > Well, there are a few more things i think
> > > We need name / (mime) type to remux attachments
> > > (CD Cover / ? / tiff)
> > > (Fan art / image/x-photoshop / psd)
> > > There also could be other things like Author, ...
> > >
> > > Also we need a way to get the attachments to the decoders, yes we
> > > dont have a ASS/SSA decoder but lets assume we had.
> > >
> > > And while we can just store the streams in avi/mov, where does the
> > > filename/name/type go? if its dropped you could end up with a huge
> > > number of images and not know what each is.
> > >
> > > > What do you think about this proposal ? Does it sounds like a
> > > > reasonable base ?
> > > >
> > > > If it's not, the only viable alternative seems to be
> > > > AVFormatContext.attachments just like in the original patch,
> > > > but with some additionnal code to allow "remuxing", etc...
> > >
> > > yes, these seem to be the 2 options we have
> > 
> > This patch implements attachments as separate streams with extradata.
> 
> OK. Here are things issues in this patch:
>  - CODEC_TYPE_NB must stay at the end of the enum.
>  - Adding attachments to AVFormatContext is probably a leftover from
>    your previous patch.
>  - I don't like the fake MATROSKA_TRACK_TYPE_ATTACHMENT and everything
>    depending on it.
>  - I don't think CODEC_ID_ATTACHMENT is of any use if it's used for
>    every kind of attachement. CODEC_TYPE_ATTACHMENT already gives the
>    same information.
> 
> I fixed all those issues in the attached patch.
> 
> Moreover, I tried to associate a specific CODEC_ID_* to different kind
> of attachements. To me, this seems like a good idea, but I would like
> to know what others think about it ?
> 
[...]

> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revision 11544)
> +++ libavformat/avformat.h	(working copy)
> @@ -21,8 +21,8 @@
>  #ifndef FFMPEG_AVFORMAT_H
>  #define FFMPEG_AVFORMAT_H
>  
> -#define LIBAVFORMAT_VERSION_INT ((52<<16)+(4<<8)+0)
> -#define LIBAVFORMAT_VERSION     52.4.0
> +#define LIBAVFORMAT_VERSION_INT ((52<<16)+(5<<8)+0)
> +#define LIBAVFORMAT_VERSION     52.5.0
>  #define LIBAVFORMAT_BUILD       LIBAVFORMAT_VERSION_INT
>  
>  #define LIBAVFORMAT_IDENT       "Lavf" AV_STRINGIFY(LIBAVFORMAT_VERSION)
> @@ -347,6 +347,9 @@
>  
>  #define MAX_REORDER_DELAY 4
>      int64_t pts_buffer[MAX_REORDER_DELAY+1];
> +
> +    char *filename;

> +    char *type;

iam against some undocumented char *type
please document precissely what it repressents and how it differs from
codec_tag, stream_codec_tag and codec_id
or even better get rid of it and use codec_tag or explain why the type
here should be special cased relative to these funny codec id strings in
matroska

filename should be documented as well, and in stream type independant way like
the "source filename" of the stream or so


>  } AVStream;


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080120/fedec32e/attachment.pgp>