[FFmpeg-devel] [PATCH] use new metadata API in mov (de)muxer
Aurelien Jacobs
aurel
Thu Feb 12 23:06:08 CET 2009
Baptiste Coudurier wrote:
> 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 ;)
At least I was lucid about this :-)
> > [...]
> >
> > 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.
Indeed, the FFMAX is only here to get back the old behavior of getting
0 in case of error.
The return -1 is needed to distinguish between lang="eng" and
lang="random_unrecognized". It helps to validate that a tag is really
in the form key-lang (with a valid lang) and not key-follow_up_of_key.
Yes, this is due to this "API" used to store lang and that you don't
like.
> > [...]
> >
> > - 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.
That would indeed be more generic, but from my understanding, the
metadata key is supposed to be a user understandable string. A end
user application should display the straight key string.
mov atom identifier don't really qualify as user readable string...
BTW: does mov have a small limited set of well know supported
metadata atom, or is anyone allowed to set any random metadata
at will ?
> 2) Translate API defined tags, like "comment", "album", etc.. ?
This needs to be added to the API, and I already have something in
mind for this.
> 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.
That's not what I had in mind, but that sounds interesting. I will
think about it...
> > [...]
> >
> > }
> > - 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.
I agree about this. Well in this part (demuxer), the additional code
is small enough that I don't really bother me.
> > [...]
> >
> > +
> > + 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.
Yes, this one (muxer) is significantly more ugly !
And that's the reason why I don't like this lang API very much.
But on the other hand, I will only be troublesome in 2 places,
namely the mov muxer and the mkv muxer. So it's not that much
code to deal with, and I've decided I could live with it. I'm
not motivated anymore to fight about this lang API, so I'm
just trying to get things finally implemented.
If you refuse the current API and really want to get it
changed, just tell me. I would then wait for you to get it
changed before I update this mov patch.
> > [...]
> >
> >
> > - 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 ?
Size won't be zero, because at least the "preamble" was already written
unconditionally. But maybe I could move this and get ride of count.
Will have a look at it.
I will prepare updated (and hopefully split) patches.
Aurel
More information about the ffmpeg-devel
mailing list