[FFmpeg-devel] [PATCH] Metadata

[Michael Niedermayer]

On Sun, Jan 04, 2009 at 06:41:00PM +0100, Aurelien Jacobs wrote:
> Michael Niedermayer wrote:
> 
> > Hi
> > 
> > Attached patch adds generic metadata support to
> > ffmpeg.c, and the avi muxer and demuxer
> > The implementation should be pretty much as simple as possible.
> > 
> > Differences to aurels variant
> > * flat metadata, no trees
> 
> This is OK for me.
> I don't really care about trees. Anyway it's rarely used, so I simply won't
> support it in mkv at first. It can always be improved later in the mkv
> (de)muxer, flattening tags such as AUTHOR/ADDRESS.
> 
> > * fully abstracted, implementation can be changed with no effect on ABI/API
> > * only a small set of muxers & demuxers are updated yet.
> > 
> > Comments welcome, especally from aurel.
> 
> First, several general questions/comments:
> 

> * When a tag is muxed as key=value with its lang set to english and a
> default_lang flag, how should it be stored in AVMetaData ? I guess
> the value should be duplicated, like this:
>   key = value
>   key-eng = value

yes


> 
> * why is metadata.{c,h} in libavcodec while it's only used in libavformat ?
> I think the patch shouldn't touch libavcodec at all...

It was because i thought some idiot might put metadata at the codec level
and we might want to extract that too. But i dont mind moving the files to
lavf if you prefer? We can always move them back if it becomes needed ...


> 
> Some details about the patch:
> 
> Index: libavcodec/metadata.h
> ===================================================================
> --- libavcodec/metadata.h	(revision 0)
> +++ libavcodec/metadata.h	(revision 0)
> @@ -0,0 +1,38 @@
> > + [...]
> > +#ifndef AVCODEC_METADATA_H
> > +#define AVCODEC_METADATA_H
> > +
> > + [...]
> > +
> > +#endif
> 
> The missing comment on the #endif will upset Diego...

right, fixed


> 
> > Index: libavcodec/avcodec.h
> > ===================================================================
> > --- libavcodec/avcodec.h	(revision 16302)
> > +++ libavcodec/avcodec.h	(working copy)
> > @@ -400,7 +400,50 @@
> >   */
> >  #define FF_MIN_BUFFER_SIZE 16384
> > 
> > +
> > +/*
> > + * public Metadata API.
> > + * Important concepts, to keep in mind
> > + * 1. keys are unique, there are never 2 tags with equal keys, this is also
> > + *    meant semantically that is key=Author, key=Author2 is illegal as well.
> > + *    All authors have to be placed in the same tag for the case of Authors.
> 
> I think it's impossible to enforce in demuxer supporting generic metadata.
> How could a demuxer differentiate those 2 cases:
>  - key=Author1, key=Author2
>  - key=IPV4, key=IPV6
> The demuxer has no idea of the semantic.

It wasnt meant to be enforced like that, rather that a demuxer shouldnt
knowingly produce Author1 Author2 from a container that allowed multiple
identical keys

new text:
 * 1. keys are unique, there are never 2 tags with equal keys, this is also
 *    meant semantically that is a demuxer should not knowingly produce
 *    several keys that are litterally different but semantically identical,
 *    like key=Author5, key=Author6.
 *    All authors have to be placed in the same tag for the case of Authors.


> 
> > + * 3. A tag whichs value is translated has the ISO 639 3-letter language code
> > + *    with a '-' between appended. So for example Author-ger=Michael, Author-eng=Mike
> 
> No support for country code (ISO 3166-1) ?
> Just a question, I personally don't care.

i can add it if anyone wants ...
We also can easily add it later, i had no special reason to omit it, it
was just that i didnt immedeatly saw any real use case for it so i thought
simpler is better when it can be added later ...


> 
> > + * @return found tag or NULL, the value of the tag may be av_realloced or
> > + *         changed by the caller, the key MUST NOT be changed.
> 
> IMO there's no reason to encourage people messing directly with the tag
> value when there is a clean API to do it.

indeed, removed



> 
> > Index: libavformat/utils.c
> > ===================================================================
> > --- libavformat/utils.c	(revision 16397)
> > +++ libavformat/utils.c	(working copy)
> > @@ -2305,6 +2306,9 @@
> >          av_free(s->chapters[s->nb_chapters]);
> >      }
> >     av_freep(&s->chapters);
> > +    if(s->meta_data)
> > +        av_freep(&s->meta_data->elems);
> > +    av_freep(&s->meta_data);
> 
> You are leaking all the elems->key and elems->value.

fixed


> It may be worth adding a av_metadata_empty() function to do this...

maybe, though can be added later easily, removial is harder


> 
> 
> Overall, this patch looks like a sane basic implementation.
> It misses a few important things (see below) but the core API
> could probably already be commited (with no version bump, no
> (de)muxer change and no ffmpeg/ffplay change) so that we can
> improve it.

will commt after reading through the patch again (it alraedy passed
my tests)


> 
> Here are a few improvements that are IMO needed (and that I
> may provide a patch for, when basic API is commited):
> 
> * Add a compatibility layer until lavf v53.0.0 so that people who
>   read or write in AVFormatContext.title (or author or anything)
>   with lavf 52.x.x still gets the correct behavior.
>   This can probably be extracted and ported from my patch.
> 
> * Add a bunch of #if LIBAVFORMAT_VERSION_MAJOR < 53 to remove
>   all the old metadata API in next version
> 
> * Add AVMetaData in AVStream, AVProgram and AVChapter (same as
>   AVFormatContext).
> 
> * Maybe add a way to normalize metadata keys for formats supporting
>   only a limited set of keys. For example when muxing in mp3, if we
>   have an 'artist' key but no 'author' key, the 'artist' should be
>   stored instead of 'author'.
>   This is only an internal API that can be added latter.
> 
> * Several formats support storing ISO 639-2 lang in a separate
>   field. If we store lang in the key field, we will probably need
>   some internal helper functions to store/extract lang to avoid
>   duplicating it in various muxers.
>   This is only an internal API that can be added latter.
> 
> When we are finally happy with the API, we can bump lavf minor,
> change all (de)muxers to the new API, and then update ffmpeg/ffplay.
> 
> Does this plan sound OK ?

yes

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20090104/6ac821b5/attachment.pgp>