[FFmpeg-devel] [PATCH][RFC] add a list of generic tag names

[Aurelien Jacobs]

On Sat, Sep 26, 2009 at 12:25:27AM +0200, Anton Khirnov wrote:
> Hi,
> attached is a patch for $subj. Suggestions for new tags/renames are
> welcome, it'll also probably need some attention from the grammar
> na^W^Wenglish experts :)
> 
> year/date - the old code had 'year', but many formats support full date,
> so 'year' can be misleading.

I agree that date should be better.
And as long as it's in ISO 8601 format or similar, muxer that only support
year can just mux the 4 first characters...

> From a2558208fb6e283419e400f3d3e3f18c879de1db Mon Sep 17 00:00:00 2001
> From: Anton Khirnov <wyskas at gmail.com>
> Date: Fri, 25 Sep 2009 22:50:15 +0200
> Subject: [PATCH] add a list of generic tags and change demuxers to follow it.
> 
> ---
>  libavformat/asf.c            |    4 ++--
>  libavformat/avformat.h       |   22 +++++++++++++++++++++-
>  libavformat/matroska.c       |    4 ++--
>  libavformat/oggparsevorbis.c |    2 +-
>  4 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/asf.c b/libavformat/asf.c
> index 79ef614..8cf9655 100644
> --- a/libavformat/asf.c
> +++ b/libavformat/asf.c
> @@ -117,12 +117,12 @@ const ff_asf_guid ff_asf_language_guid = {
>  };
>  
>  const AVMetadataConv ff_asf_metadata_conv[] = {
> -    { "AlbumArtist", "artist"    },
> +    { "AlbumArtist", "albumauthor"},

I don't really like this one, and it's not used in any other format AFAICT.
what about using "performer" instead ?

>      { "AlbumTitle" , "album"     },
>      { "Author"     , "author"    },
>      { "Genre"      , "genre"     },
>      { "Copyright"  , "copyright" },
>      { "TrackNumber", "track"     },
> -    { "Year"       , "year"      },
> +    { "Year"       , "date"      },

OK.

> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index be8c374..55f66d5 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -99,7 +99,27 @@ int av_metadata_set(AVMetadata **pm, const char *key, const char *value);
>  
>  /**
>   * Converts all the metadata sets from ctx according to the source and
> - * destination conversion tables.
> + * destination conversion tables. If one of the tables is NULL, then
> + * tags are converted to/from ffmpeg generic tag names. Currently defined
> + * generic tag names are:
> + * album        -- name of the set this work belongs to
> + * albumauthor  -- main author of the set

I don't like albumauthor. Please get ride of it for now...
We can disscuss it later, after this patch is committed.

> + * albumsort    -- used instead of album for sorting
> + * author       -- main author of the work
> + * authorsort   -- used instead of author for sorting
> + * comment      --
> + * composer     -- artist who composed the work, if different from author
> + * copyright    -- name of copyright holder
> + * date         -- date when the work was created, preferably in ISO 8601
> + * disc         -- number of a subset, i.e. disc in a multi-disc collection
> + * encoder      -- person who encoded the file

Hummm... To me it seems like encoder is generaly the name/version of
the software which was used to encode the file. Not the name of a
person. Am I wrong ?

> + * genre        --
> + * language     --
> + * performer    -- artist who performed the work, if different from author
> + * publisher    --
> + * title        -- name of the work
> + * titlesort    -- used instead of title for sorting
> + * track        -- number of this work in the set

Maybe it's worth to note that track can also contain the total number of
track in the set, in this form:  "current/total"  (eg. 05/12).

You could probably also add "filename" (original name of the file) to
this list. It's used by at least 3 demuxers.

I'm not sure all the '-sort' are that useful, but anyway it's very ugly
to duplicate every line. So if you really want to document '-sort', do
it in a generic way by just mentionning that each can have a '-sort' variant.

Also I think all of those tags needs a proper description. Just listing
a tag name without describing its purpose is mostly useless.

> diff --git a/libavformat/matroska.c b/libavformat/matroska.c
> index f101a00..1bfb7c9 100644
> --- a/libavformat/matroska.c
> +++ b/libavformat/matroska.c
> @@ -92,8 +92,8 @@ const CodecMime ff_mkv_mime_tags[] = {
>  };
>  
>  const AVMetadataConv ff_mkv_metadata_conv[] = {
> -    { "ARTIST"        , "artist" },
> -    { "LEAD_PERFORMER", "artist" },
> +    { "ARTIST"        , "author" },
> +    { "LEAD_PERFORMER", "performer" },
>      { "PART_NUMBER"   , "track"  },

OK.

> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index afc3fcb..ea090d5 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -39,7 +39,7 @@ const AVMetadataConv ff_vorbiscomment_metadata_conv[] = {
>      { "ARTIST"     , "author" },
>      { "TITLE"      , "title"  },
>      { "ALBUM"      , "album"  },
> -    { "DATE"       , "year"   },
> +    { "DATE"       , "date"   },
>      { "TRACKNUMBER", "track"  },

OK.

Your patch is also missing some conversion from "year" to "date", at
least in mov.c, oggparsevorbis.c and id3v1.c.
You will also need to add a conversion table for apetag.c (used in ape
and mpc) with the following conversions:
  "Artist" => author
  "Year"   => date
Many demuxers are using the tag "muxer" to store the name of the
encoding software. Maybe those should be converted to "encoder".

You will also need to update metadata_compat.c, to add support for
new "standard" tags such as date, performer, etc...

Overall I like the patch. It's a nice step forward.
Oh and sorry for the delay !

Aurel