[FFmpeg-devel] [PATCH] new generic metadata API

Michael Niedermayer michaelni
Sun Nov 2 01:08:52 CET 2008


On Mon, Oct 20, 2008 at 11:22:43PM +0200, Aurelien Jacobs wrote:
> On Mon, 13 Oct 2008 19:27:29 +0200
> Michael Niedermayer <michaelni at gmx.at> wrote:
> 
> > On Mon, Sep 29, 2008 at 12:46:54AM +0200, Aurelien Jacobs wrote:
> > > On Sun, 28 Sep 2008 00:27:40 +0200
> > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > 
> > > > 
> > > > On Sat, Sep 27, 2008 at 04:03:53PM +0200, Aurelien Jacobs wrote:
> > > > > On Tue, 23 Sep 2008 15:21:43 +0200
> > > > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > [...]
> > > > 
> > > > > > [note, also see the nut-devel ML for some related change suggestion i just
> > > > > >  posted]
> > > > > 
> > > > > Seen it.
> > > > > I've added generic flattening and un-flattening support, following your
> > > > > proposed nut scheme. The exact formatting could be changed latter if
> > > > > a different scheme is accepted.
> > > > 
> > > > What does it do with 2
> > > > Author tags? does it convert to Author / Author2?
> > > 
> > > Yes.
> > > 
> > 
> > > > Similarly, does it remove the '2' postfix when unflattening?
> > > 
> > > No. I'm not sure if it's a good idea to remove it...
> > 
> > mkv->nut-mkv should generally end up with equivalent metadata, similarly
> > nut->mkv->nut should too.
> > Also what the user of libavcodec sees from both should be roughly the same.
> 
> That was exactly what I thought about when I deceided it wasn't a
> good idea.
> What if original mkv file literally contains:
>   AUTHOR=Mr. X
>   AUHTOR2=Mr. Y
> I think the mkv spec don't forbid this (anyway, other containers could
> allow it). So now, mkv->nut->mkv shouldn't remove the '2' to end up
> with equivalent metadata. This would be even more important if someone
> deceides to store tags such as:
>   ID3V1=yes
>   ID3V2=no
> or any other names which have a trailing number with an actual meaning.
> 
> So removing the '2' could destruct some really useful information
> in some situations, while keeping it shouldn't have any bad effect,
> except looking a little worse sometimes.

The 2 systems are supposed to be bijective. If one cant represent something
then we already have a problem IMHO
This is slightly related to escaping.


> 
> > > Anyway, that could be changed later without touching the API.
> > 
> > > 
> > > > does it prefix non standard parts with 'X-' when muxing nut?
> > > 
> > > It didn't. Now it does.
> > 
> > "X-" really means free form user data vs. the lack of it means that the
> > data is exactly in the format as written in the spec.
> > simply adding and removing 'X-' depending on the name being on a list
> > likely does not achive this.
> 
> OK. Now instead of blindly adding some prefix, ff_metadata_format() will
> call a callback function which can precisely validate the name and format
> of tags, and return the appropriate prefix (or totaly discard the tag).
> nutenc now checks strict validity of tags that only accept a limited set
> of values and stores invalid tags as non-official one (X- prefix).
> 
> > Also this patch does not seem to convert time formats, language code and
> > country code formats. I think its a rather strong assumtation that every
> > 2 containers use compatible ones.
> 

> Regarding language/country code, the only valid internal format is
> ISO 639-2 - ISO 3166-1. Currently, all demuxers supporting language are
> using this formats to export it.

yes but what about 2 vs 3 letter?


[...]
> > > > [...]
> > > > > Index: libavformat/avformat.h
> > > > > ===================================================================
> > > > > --- libavformat/avformat.h	(revision 15392)
> > > > > +++ libavformat/avformat.h	(working copy)
> > > > > @@ -22,8 +22,8 @@
> > > > >  #define AVFORMAT_AVFORMAT_H
> > > > >  
> > > > >  #define LIBAVFORMAT_VERSION_MAJOR 52
> > > > > -#define LIBAVFORMAT_VERSION_MINOR 22
> > > > > -#define LIBAVFORMAT_VERSION_MICRO  1
> > > > > +#define LIBAVFORMAT_VERSION_MINOR 23
> > > > > +#define LIBAVFORMAT_VERSION_MICRO  0
> > > > >  
> > > > >  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> > > > >                                                 LIBAVFORMAT_VERSION_MINOR, \
> > > > 
> > > > > @@ -314,6 +314,20 @@
> > > > >      struct AVInputFormat *next;
> > > > >  } AVInputFormat;
> > > > >  
> > > > > +typedef struct AVMetadataTag AVMetadataTag;
> > > > > +
> > > > > +typedef struct AVMetadata {
> > > > > +    unsigned int nb;
> > > > > +    AVMetadataTag **list;
> > > > > +} AVMetadata;
> > > > > +
> > > > > +struct AVMetadataTag {
> > > > > +    char *name;
> > > > > +    char *value;
> > > > > +    char *lang;
> > > > > +    AVMetadata nested;
> > > > > +};
> > > > > +
> > > > >  enum AVStreamParseType {
> > > > >      AVSTREAM_PARSE_NONE,
> > > > >      AVSTREAM_PARSE_FULL,       /**< full parsing and repack */
> > > > 
> > > > Is it needed to have these structs in a public header?
> > > 
> > > Yes. Applications have to be able to parse the full tree to
> > > display it for example. See ffplay.c:dump_metadata() for what
> > > an application may require.
> > > 
> > > > Making the structs less public would allow more changes without breaking
> > > > the API/ABI, we for example might one day want to use AVTree for it, and or
> > > > might even want to switch to a flat internal format.
> > > 
> > > I understand your concern, but I think the price to pay would be too
> > > high. It would require some tree iterator functions and some individual
> > > fields accessor functions, etc...
> > 
> > Iam not insisting on this, id just like to understand better if it really
> > would be more complex to avoid depeding on the tree structure. And what
> > relation there is between such iterator function and the code in the end
> > user app, it may very well be that the lack of such iterator would make
> > accessing the data harder. After all the programmer accessing it at least
> > needs to understand the structure.
> > 
> > is there more needed than:
> > void enumerate(const AVMetadata *m, int indent, void *opaque, func)
> > {
> >     int i;
> >     for (i=0; i<m->nb; i++) {
> >         func(opaque, m->list[i], indent);
> >         if (m->list[i]->nested.nb)
> >             enumerate(&m->list[i]->nested, indent+1, opaque, func);
> >     }
> > }
> > 
> > ?
> 
> This would probably work in most (all?) situations, but this could somewhat
> complicate calling apps. For example if I want to skip one sub-tree, or
> parse only a sub-range of the tag list, my calling app will still receive
> all tags and will need to maintain some internal state to know what tag
> to discard or not.
> Direct access allows to trivially skip recursing in a sub-branch, or
> directly access a sub-range...

direct access possibility will require the whole design to be investigated
much more carefully because we would not be able to change it later.
One issue is that the whole design is O(n^2) for many operations, this
appears negligible for the normal use case of a few (<100) metadata tags
per file.
Though if some files use or require 100k metadata tags this would break
down. Basically the system is not scaleable, and with direct access we
wont be able to fix it later in case it becomes needed ...

[...]

ill review the patc later (just wanted to send these out today ...)

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

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/20081102/3a2fd7ff/attachment.pgp>



More information about the ffmpeg-devel mailing list