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

Bartsevich, Dmitry Bartsevich
Fri Jan 29 18:13:59 CET 2010


> From: ffmpeg-devel-bounces at mplayerhq.hu 
> [mailto:ffmpeg-devel-bounces at mplayerhq.hu] On Behalf Of 
> Aurelien Jacobs
> Sent: Thursday, January 21, 2010 2:54 AM
> 
> > 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.
Ok, right.

> 
> > +
> > +    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.
Ok, will be fixed.

> 
> > +        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().
Ok.

> 
> > +        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.
So, convert tag name to upper case? According to spec tag name should not contain spaces.
Should this be verified as well (e.g. replace spaces by underscores)?

> Also you may want to extract the language out of 
> metadata_tag->key when present. That way you could write 
> MATROSKA_ID_TAGLANG.
Do you mean extracting language code from tag keys like "TAG_NAME-lng"? Should suffix
be validated against list of ISO language codes or everything that matches "-[a-z]{3}$" regexp
may be considered language code?

> 
> > [...]
> > +    // 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.
Ok, will be done.

> Also some tags should not be written as they are treated 
> specially (eg. title, language, description...)
In matroskadec.c I've found the following special tag names:
title
language
description
filename
Is there anything else? The tags are in lower case, should case-sensitive comparison
be used to detect such strings, so that e.g. 'TITLE' tag will be written to tags section?

Br,
Dmitry.



More information about the ffmpeg-devel mailing list