[FFmpeg-devel] [PATCH] Metadata

[Aurelien Jacobs]

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

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

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...

> 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.

> + * 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.

> + * @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.

> 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.
It may be worth adding a av_metadata_empty() function to do this...


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.

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 ?

Aurel