[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