[FFmpeg-devel] [PATCH] Tags support for Matroska muxer

[Aurelien Jacobs]

On Tue, Jan 19, 2010 at 06:36:57PM +0200, Bartsevich, Dmitry wrote:
> The attached patch adds support for storing file-wide and track-specific
> tags to MKV files (tags reading has been already supported by Matroska
> demuxer). Tags are taken from metadata member of AVFormatContext
> and AVStream structures.

Content-Description: mkv-tags.patch
> Index: libavformat/matroskaenc.c
> ===================================================================
> --- libavformat/matroskaenc.c	(revision 21223)
> +++ libavformat/matroskaenc.c	(working copy)
> @@ -619,6 +619,62 @@
>      return 0;
>  }
>  
> +static void mkv_write_metadata(
> +    AVMetadata *metadata,
> +    ByteIOContext *pb,
> +    unsigned int target_elementid,
> +    int target_id)
> +{
> +    AVMetadataTag *metadata_tag;
> +    ebml_master tag, targets, simple_tag;
> +    const int metadata_get_flags = AV_METADATA_IGNORE_SUFFIX|AV_METADATA_MATCH_CASE;

I guess AV_METADATA_MATCH_CASE is useless here.

> +
> +    metadata_tag = NULL;
> +    while ((metadata_tag = av_metadata_get(metadata, "", metadata_tag, metadata_get_flags)) != NULL) {
> +        tag = start_ebml_master(pb, MATROSKA_ID_TAG, 0);

This should be out of the loop ! You should write only one MATROSKA_ID_TAG
per target. Only MATROSKA_ID_SIMPLETAG should be in the loop.

> +        if (target_elementid != 0) {
> +            targets = start_ebml_master(pb, MATROSKA_ID_TAGTARGETS, 0);
> +            put_ebml_uint(pb, target_elementid, target_id);
> +            end_ebml_master(pb, targets);
> +        }

This should be out of the loop too, and MATROSKA_ID_TAGTARGETS must be
written unconditionnaly (its presence is mandatory according to the
spec). So only the target_id should be under if().

> +        simple_tag = start_ebml_master(pb, MATROSKA_ID_SIMPLETAG, 0);
> +        put_ebml_string(pb, MATROSKA_ID_TAGNAME, metadata_tag->key);

Here you should ensure that the written key is all caps according to the spec.
Also you may want to extract the language out of metadata_tag->key when
present. That way you could write MATROSKA_ID_TAGLANG.

> [...]
> +    // Write file-level tags
> +    mkv_write_metadata(s->metadata, pb, 0, 0);
> +
> +    // Write stream-level tags
> +    for (i = 0; i < s->nb_streams; i ++)
> +        mkv_write_metadata(s->streams[i]->metadata,

You may also want to write chapter-level tags.
Also some tags should not be written as they are treated specially
(eg. title, language, description...)

Aurel