[FFmpeg-devel] [PATCH] new generic metadata API
Aurelien Jacobs
aurel
Mon Oct 20 23:22:43 CEST 2008
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.
> > 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.
Regarding time format, I guess we should do something similar, using
ISO 8601 as the only valid internal format ?
For now I haven't checked this carefully, so I've simply disabled muxing
of CreationTime in nut (the only officially supported tag containing a
date for now). This ensure we won't mux broken files and is no worse than
current situation. I will check this time format issue again later.
> This also is a rather important issue as it would lead to creation of broken
> files.
> Also some fields from info packets in nut like "Uses" and "Replaces" refer to
> streams, its unlikely that blindly copying them will still have them refer
> to the correct streams.
OK. I've added a flag to those tags so that they now can't be automatically
copied when remuxing (they will simply be discarded when copying tags from
demuxer to muxer).
> > > [...]
> > > > 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...
> [...]
> > +static const char const *metadata_mapping[][6] = {
>
> the 2nd const should be after the *
Ooops... Sure. Fixed.
> > +static AVMetadataTag *find_metadata_tag(AVMetadata *metadata,
> > + const char *name, const char *lang,
> > + int strict_lang, int name_len)
> > +{
> > + int i;
> > + for (i=0; i<metadata->nb; i++)
> > + if (!strncasecmp(metadata->list[i]->name, name, name_len))
> > + if ((!lang && (!strict_lang || !metadata->list[i]->lang)) ||
> > + (lang && metadata->list[i]->lang &&
> > + !strncasecmp(metadata->list[i]->lang, lang, 3)))
> > + return metadata->list[i];
> > + return NULL;
> > +}
>
> In general id expect the user to want to see
> tag matching his language&country if no such tag exists then one in a
> language he understands, failing that too the default tag.
> It seems this is not supported ...
OK. I implemented this. Now av_metadata_get_tag() takes a list of prefered
languages by order of preference. This list can contain "default" to ask
for best effort to select lang which is stored as the default one.
If no list is specified, this function will return the first tag it
finds without checking its language.
> > +static const char *extract_metadata_lang(const char *lang,
> > + const char *name, int *len)
> > +{
> > + if (!lang && *len>4 && name[*len-4] == '-' && islower(name[*len-3])
> > + && islower(name[*len-2]) && islower(name[*len-1])) {
> > + *len -= 4;
> > + return name + *len + 1;
> > + }
> > + return lang;
> > +}
>
> i dont think this will work with country codes
Oh, it seems I forgot that part of nut. It is now implemented.
It works with the 3 different ISO 3166-1 syntax (alpha-2, alpha-3, numeric).
Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: metadata3.diff
Type: text/x-diff
Size: 113573 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081020/a9e91c17/attachment.diff>
More information about the ffmpeg-devel
mailing list