[FFmpeg-devel] [PATCH] metadata conversion API
Aurelien Jacobs
aurel
Sat Feb 28 00:07:11 CET 2009
Michael Niedermayer wrote:
> On Thu, Feb 26, 2009 at 11:14:15PM +0100, Aurelien Jacobs wrote:
> > Michael Niedermayer wrote:
> >
> > > On Thu, Feb 26, 2009 at 02:49:01AM +0100, Aurelien Jacobs wrote:
> > > > Michael Niedermayer wrote:
> > > >
> > > > > On Thu, Feb 26, 2009 at 02:13:51AM +0100, Aurelien Jacobs wrote:
> > > > > > Hi,
> > > > > >
> > > > > > There is one last and important issue I want to address with the new
> > > > > > metadata API.
> > > > > > Old API allowed client apps and muxers to get a few select well known
> > > > > > tags (title, author...). With the new API, there is no simple way to
> > > > > > do that, right now. For example, if you demux an ASF file, and want to
> > > > > > get the name of the album, av_metadata_get(..., "album", ...) won't
> > > > > > give you any results, because ASF stores this information in a tag
> > > > > > named "AlbumTitle". There are lots of examples with various demuxers,
> > > > > > even for simple common tags. This also prevent correct remuxing between
> > > > > > different containers.
> > > > > >
> > > > > > One simple solution would be to force any demuxers to export their tags
> > > > > > respecting a well defined list of known common key names (for all tags
> > > > > > that have a corresponding common name). But this have issues which makes
> > > > > > this impractical. It looses information. The client app can't retrieve
> > > > > > the actual key names used in the original file. And this would prevent
> > > > > > lossless remuxing in the same container.
> > > > > >
> > > > > > So I built up another solution. The principle is to allows (de)muxers
> > > > > > to associate a conversion table to a AVMetadata. This table describe
> > > > > > the correspondence between the native format key names and the generic
> > > > > > common key names. Then with this conversion table,
> > > > > > av_metadata_get(..., "album", ...) is able to get the native tag
> > > > > > corresponding the the generic "album" key. And a muxer is able to
> > > > > > convert a whole AVMetadata to it's own native format.
> > > > > >
> > > > > > First patch implements this conversion table API.
> > > > > > Second patch just shows a simple example of usage of this API (yes
> > > > > > I know it duplicates the ff_asf_metadata_conv table and I don't
> > > > > > intend to apply it as is, it's only a simple incomplete and unclean
> > > > > > example for now).
> > > > > >
> > > > > > Also note the the av_metadata_get_conv() function which may look useless
> > > > > > in this patch set, is in fact required for applications such as ffmpeg,
> > > > > > to "copy" the conv table from input format ctx to output format ctx.
> > > > > >
> > > > > > And finally, here is a concrete example of what this API allows. A simple
> > > > > > stream copy of those Matroska tags:
> > > > > > LEAD_PERFORMER: Tori Amos
> > > > > > ALBUM: Under the Pink
> > > > > > generates the following ASF tags:
> > > > > > AlbumArtist: Tori Amos
> > > > > > AlbumTitle: Under the Pink
> > > > >
> > > > > IMHO The correct way to implement such conversion table is
> > > > > by adding it to AVInputFormat & AVOutputFormat
> > > > > and adding a simple av_metadata_conv() that takes the input metadata and
> > > > > both tables to produce outpt metadata.
> > > > > Thi convertion, which is just a single line of code would be called by the
> > > > > user app.
> > > > > muxers would only store exactly the metadata tags given to them and demuxers
> > > > > would only read exactly the metadata tags, convertion would be entirely
> > > > > outside of them.
> > > >
> > > > I've thought about this possibility too. But then, do you think we should also
> > > > add a conversion table to AVStream, AVChapter, etc... ?
> > > > We may need to have a conversion table to handle their metadata in a different
> > > > way than the main metadata...
> > >
> > > I think the table should be opaque to the user app (for the momemt, the details
> > > can be exported later when they have stabilized)
> >
> > OK.
> >
> > > and could contain AVStream/AVChapter information, it could even contain more
> > > complex translation info, i mean theres no guarantee for a 1:1 mapping
> > > for example one format might store per stream authors, while the other might
> > > support only per file authors the convertion code could together with these
> > > tables convert between these 2.
> >
> > Good idea. Well, for now, I've kept the simplest possible version (ie. with
> > no AVStream/AVChapter information, nor anything else fancy). But we can
> > improve this later, in any possible way, as long as the tables are opaque.
> >
> > New patches attached. They are significantly simpler than original versions,
> > and work as well.
> >
> > Aurel
> > Index: libavformat/metadata.c
> > ===================================================================
> > --- libavformat/metadata.c (r?vision 17619)
> > +++ libavformat/metadata.c (copie de travail)
> > @@ -18,6 +18,7 @@
> > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > */
> >
> > +#include <strings.h>
> > #include "metadata.h"
> >
> > AVMetadataTag *
> > @@ -76,6 +77,35 @@
> > return 0;
> > }
> >
>
> > +void av_metadata_conv(AVMetadata **dst, const AVMetadataConv *d_conv,
> > + AVMetadata *src, const AVMetadataConv *s_conv)
> > +{
> > + const AVMetadataConv *s_conv1 = s_conv, *d_conv1 = d_conv, *sc, *dc;
> > + AVMetadataTag *mtag = NULL;
> > + const char *key, *key2;
> > +
> > + while((mtag=av_metadata_get(src, "", mtag, AV_METADATA_IGNORE_SUFFIX))) {
> > + key = key2 = mtag->key;
> > + if (s_conv != d_conv) {
> > + if (!s_conv)
> > + s_conv1 = (const AVMetadataConv[2]){{key,key}};
> > + for (sc=s_conv1; sc->native; sc++)
> > + if (!strcasecmp(key, sc->native)) {
> > + key2 = sc->generic;
> > + break;
> > + }
> > + if (!d_conv)
> > + d_conv1 = (const AVMetadataConv[2]){{key2,key2}};
> > + for (dc=d_conv1; dc->native; dc++)
> > + if (!strcasecmp(key2, dc->generic)) {
> > + key = dc->native;
> > + break;
> > + }
>
> maybe bsearch() could be used to do the look up in the 2 tables, though
> it makes no sense ATM considering the table size so thats just a request
> for a //TODO comment
OK. Added.
> [...]
>
> > @@ -96,6 +97,13 @@
> > int av_metadata_set(AVMetadata **pm, const char *key, const char *value);
> >
> > /**
> > + * Convert a metadata set from src to dst according to their associated
> > + * d_conv and s_conv conversion tables.
> > + */
> > +void av_metadata_conv(AVMetadata **dst, const AVMetadataConv *d_conv,
> > + AVMetadata *src, const AVMetadataConv *s_conv);
> > +
> > +/**
>
> i was wondering if it might be better to pass AVFormatContexts in that, i mean
> for moving things betweem AVStream and the main metadata ...
Indeed, passing a AVFormatContext allows doing some more advanced conversions.
I don't know if those advanced possibilities would ever be used, but it's
probably worth providing the most versatile API.
It has some pros and cons:
+ more versatile and future proof
+ allows converting all the various metadata in a AVFormatContext with a
single function call
- requires one more memcpy of the metadata when stream copying (but speed
shouldn't matter here)
- requires AVFormatContext to be defined before the metadata API
Regarding this last point, right now the metadata API is defined in avformat.h
before AVFormatContext itself. There are two solutions:
- move some parts of the metadata API lower in the file (this would be harder
to understand the API while reading avformat.h)
- declare AVFormatContext at the very beginning of avformat.h
I've implemented this last solution, and I took the opportunity to make usage
of AVFormatContext more consistent across avformat.h.
Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: md_conv3.diff
Type: text/x-patch
Size: 4145 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090228/842bb953/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avformatcontext.diff
Type: text/x-patch
Size: 5174 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090228/842bb953/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list