[FFmpeg-devel] [PATCH] use new metadata API in mov (de)muxer

Baptiste Coudurier baptiste.coudurier
Thu Feb 12 21:38:08 CET 2009


Hi Aurelien,

Aurelien Jacobs wrote:
> Hi,
> 
> Attached patch converts the mov muxer and demuxer to the new metadata API.
> It properly (de)mux lang when available. Note that current muxer code set
> lang to english when lang is unknown. I left this behavior unchanged,
> especially because I don't know what numeric language code to use for
> unknown language.
>
> This patch should certainly be applied in several parts.
> 

Definitely ;)

> [...]
>
> Index: libavformat/isom.c
> ===================================================================
> --- libavformat/isom.c	(revision 17136)
> +++ libavformat/isom.c	(working copy)
> @@ -257,7 +257,7 @@ int ff_mov_iso639_to_lang(const char *lang, int mp
>      }
>      /* XXX:can we do that in mov too? */
>      if (!mp4)
> -        return 0;
> +        return -1;
>      /* handle undefined as such */
>      if (lang[0] == '\0')
>          lang = "und";
> @@ -265,9 +265,9 @@ int ff_mov_iso639_to_lang(const char *lang, int mp
>      for (i = 0; i < 3; i++) {
>          unsigned char c = (unsigned char)lang[i];
>          if (c < 0x60)
> -            return 0;
> +            return -1;
>          if (c > 0x60 + 0x1f)
> -            return 0;
> +            return -1;
>          code <<= 5;
>          code |= (c - 0x60);
>      }

This hunk looks weird. Is it really needed ? Especially I belive the
FFMAX in muxer looks weird and could benefit from the return 0.

> [...]
>
> @@ -512,7 +514,8 @@ static int mov_read_mdhd(MOVContext *c, ByteIOCont
>      st->duration = (version == 1) ? get_be64(pb) : get_be32(pb); /* duration */
>  
>      lang = get_be16(pb); /* language */
> -    ff_mov_lang_to_iso639(lang, st->language);
> +    if (ff_mov_lang_to_iso639(lang, language))
> +        av_metadata_set(&st->metadata, "language", language);
>      get_be16(pb); /* quality */

Looks ok

> [...]
>
> -    case MKTAG(0xa9,'n','a','m'):
> -        str = c->fc->title; size = sizeof(c->fc->title); break;
> +    case MKTAG(0xa9,'n','a','m'): key = "title";     break;
> +    case MKTAG(0xa9,'a','u','t'):
>      case MKTAG(0xa9,'A','R','T'):
> -    case MKTAG(0xa9,'w','r','t'):
> -        str = c->fc->author; size = sizeof(c->fc->author); break;
> -    case MKTAG(0xa9,'c','p','y'):
> -        str = c->fc->copyright; size = sizeof(c->fc->copyright); break;
> +    case MKTAG(0xa9,'w','r','t'): key = "author";    break;
> +    case MKTAG(0xa9,'c','p','y'): key = "copyright"; break;
>      case MKTAG(0xa9,'c','m','t'):
> -    case MKTAG(0xa9,'i','n','f'):
> -        str = c->fc->comment; size = sizeof(c->fc->comment); break;
> -    case MKTAG(0xa9,'a','l','b'):
> -        str = c->fc->album; size = sizeof(c->fc->album); break;
> +    case MKTAG(0xa9,'i','n','f'): key = "comment";   break;
> +    case MKTAG(0xa9,'a','l','b'): key = "album";     break;
> +    case MKTAG(0xa9,'d','a','y'): key = "year";      break;
> +    case MKTAG(0xa9,'g','e','n'): key = "genre";     break;
> +    case MKTAG(0xa9,'t','o','o'):
> +    case MKTAG(0xa9,'e','n','c'): key = "muxer";     break;

I saw that in vorbis demuxer you exported metadata "as is", but here you
 apply some generic metadata.

1) Shouldn't we also export "as is" everything in "udta" atom coded in a
somewhat standard way (itunes, 3gp, mov) ? This is what I would call
"generic" and user could retrieve it if wanted.

2) Translate API defined tags, like "comment", "album", etc.. ? In this
case every demuxer must translate to these defined tags, in a separate
step, I believe.

I believe this should be done in an array, so it would be easy to add a
new conversion if needed. Maybe add it in AVOutputFormat or
AVInputFormat so user has a way to know which metadata is supported.

> [...]
>
>      }
> -    if (!str)
> +    if (!key)
>          return 0;
>      if (atom.size < 0)
>          return -1;
>  
> -    get_buffer(pb, str, FFMIN3(size, str_size, atom.size));
> +    str_size = FFMIN3(sizeof(str)-1, str_size, atom.size);
> +    get_buffer(pb, str, str_size);
> +    str[str_size] = 0;
> +    av_metadata_set(&c->fc->metadata, key, str);
> +    if (*language && strcmp(language, "und")) {
> +        snprintf(key2, sizeof(key2), "%s-%s", key, language);
> +        av_metadata_set(&c->fc->metadata, key2, str);
> +    }

Well, I'll repeat myself, but having a attribute "lang" would be a lot
easier IMHO.

> [...]
>
> +
> +    len = strlen(t->key);
> +    snprintf(tag2, sizeof(tag2), "%s-", tag);
> +    while ((t2 = av_metadata_get(s->metadata, tag2, t2, AV_METADATA_IGNORE_SUFFIX))) {
> +        len2 = strlen(t2->key);
> +        if (len2 == len+4 && !strcmp(t->value, t2->value)
> +            && (l=ff_mov_iso639_to_lang(&t2->key[len2-3], 0)) >= 0) {
> +            lang = l;
> +            break;
> +        }

Ditto.

> [...]
>
>  
> -        if (mov->mode & MODE_3GP) {
> -            mov_write_3gp_udta_tag(pb, s, "titl", s->title);
> -            mov_write_3gp_udta_tag(pb, s, "auth", s->author);
> -            mov_write_3gp_udta_tag(pb, s, "gnre", s->genre);
> -            mov_write_3gp_udta_tag(pb, s, "dscp", s->comment);
> -            mov_write_3gp_udta_tag(pb, s, "albm", s->album);
> -            mov_write_3gp_udta_tag(pb, s, "cprt", s->copyright);
> -            mov_write_3gp_udta_tag(pb, s, "yrrc", "nil");
> -        } else if (mov->mode == MODE_MOV) { // the title field breaks gtkpod with mp4 and my suspicion is that stuff is not valid in mp4
> -            mov_write_string_tag(pb, "\251nam", s->title         , 0);
> -            mov_write_string_tag(pb, "\251aut", s->author        , 0);
> -            mov_write_string_tag(pb, "\251alb", s->album         , 0);
> -            mov_write_day_tag(pb, s->year, 0);
> -            mov_write_string_tag(pb, "\251enc", LIBAVFORMAT_IDENT, 0);
> -            mov_write_string_tag(pb, "\251des", s->comment       , 0);
> -            mov_write_string_tag(pb, "\251gen", s->genre         , 0);
> -            mov_write_string_tag(pb, "\251cpy", s->copyright     , 0);
> -        } else {
> -            /* iTunes meta data */
> -            mov_write_meta_tag(pb, mov, s);
> -        }
> -        return updateSize(pb, pos);
> +    put_be32(pb_buf, 0); /* size */
> +    put_tag(pb_buf, "udta");
> +
> +    if (mov->mode & MODE_3GP) {
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "titl", "title");
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "auth", "author");
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "gnre", "genre");
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "dscp", "comment");
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "albm", "album");
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "cprt", "copyright");
> +        count += mov_write_3gp_udta_tag(pb_buf, s, "yrrc", "year");
> +    } else if (mov->mode == MODE_MOV) { // the title field breaks gtkpod with mp4 and my suspicion is that stuff is not valid in mp4
> +        count += mov_write_string_metadata(s, pb_buf, "\251nam", "title"      , 0);
> +        count += mov_write_string_metadata(s, pb_buf, "\251aut", "author"     , 0);
> +        count += mov_write_string_metadata(s, pb_buf, "\251alb", "album"      , 0);
> +        count += mov_write_string_metadata(s, pb_buf, "\251day", "year"       , 0);
> +        mov_write_string_tag(pb_buf, "\251enc", LIBAVFORMAT_IDENT, 0, 0);
> +        count += mov_write_string_metadata(s, pb_buf, "\251des", "comment"    , 0);
> +        count += mov_write_string_metadata(s, pb_buf, "\251gen", "genre"      , 0);
> +        count += mov_write_string_metadata(s, pb_buf, "\251cpy", "copyright"  , 0);
> +    } else {
> +        /* iTunes meta data */
> +        count = mov_write_meta_tag(pb_buf, mov, s);
>      }
> +    updateSize(pb_buf, pos);
>  
> +    if (count > 0) {
> +        uint8_t *buf;
> +        int size = url_close_dyn_buf(pb_buf, &buf);
> +        put_buffer(pb, buf, size);
> +        av_free(buf);
> +    }
> +
>      return 0;
>  }

I believe size will be 0 when if nothing has been written, why using
count ? Funcs should return bytes written, can't this be used ?

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org





More information about the ffmpeg-devel mailing list