[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