[FFmpeg-devel] [PATCH] metadata conversion API
Aurelien Jacobs
aurel
Sun Mar 1 00:35:17 CET 2009
On Sat, 28 Feb 2009 22:47:22 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sat, Feb 28, 2009 at 10:03:55PM +0100, Aurelien Jacobs wrote:
> > 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.
>
> :/
>
> your patch looks ok then
Applied.
Aurel
More information about the ffmpeg-devel
mailing list