[FFmpeg-devel] [PATCH] metadata conversion API
Aurelien Jacobs
aurel
Sat Feb 28 22:03:55 CET 2009
On Sat, 28 Feb 2009 20:22:41 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sat, Feb 28, 2009 at 12:07:11AM +0100, Aurelien Jacobs wrote:
> > 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
>
> -void av_metadata_conv(AVFormatContext *ctx, const AVMetadataConv *d_conv,
> +void av_metadata_conv(struct AVFormatContext *ctx, const AVMetadataConv *d_conv,
> const AVMetadataConv *s_conv);
> [...]
./libavformat/avformat.h:106: warning: 'struct AVFormatContext' declared inside parameter list
./libavformat/avformat.h:106: warning: its scope is only this definition or declaration, which is probably not what you want
So at the very least it requires moving declaration of struct AVFormatContext
to the top of the file. Updated patch attached.
> > +void av_metadata_conv(AVFormatContext *ctx, const AVMetadataConv *d_conv,
> > + const AVMetadataConv *s_conv)
> > +{
> > + int i;
> > + metadata_conv(&ctx->metadata, d_conv, s_conv);
> > + for (i=0; i<ctx->nb_streams ; i++)
> > + metadata_conv(&ctx->streams [i]->metadata, d_conv, s_conv);
> > + for (i=0; i<ctx->nb_chapters; i++)
> > + metadata_conv(&ctx->chapters[i]->metadata, d_conv, s_conv);
> > + for (i=0; i<ctx->nb_programs; i++)
> > + metadata_conv(&ctx->programs[i]->metadata, d_conv, s_conv);
> > +}
>
> this in place convertion has a disadvatage,
> its rather easy to entangle itself in more complex convertions
> i think 2 AVFormatContext and src and dst would be easier
What do you propose exactly ? Making a copy of the dst context and
use it as src or something like that ? This sounds useless to me.
If you are talking about using input context as src and output context
as dst, it is not possible because they both won't have the same
number/order of AVStream, AVChapter, etc... So it's not possible to
know how to do the metadata mapping between input AVStream and
output AVStream.
Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: md_conv4.diff
Type: text/x-diff
Size: 4533 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090228/5baf8a35/attachment.diff>
More information about the ffmpeg-devel
mailing list