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

[Aurelien Jacobs]

Baptiste Coudurier wrote:

> Hi,
> 
> Aurelien Jacobs wrote:
> > Aurelien Jacobs wrote:
> > 
> >> Baptiste Coudurier wrote:
> >>
> >>> Hi,
> >>>
> >>> On 2/14/2009 5:13 PM, Aurelien Jacobs wrote:
> >>>> Aurelien Jacobs wrote:
> >>>>
> >>>>> 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.
> >>>>>>>
> >>>>>>> [...]
> >>>>>>>
> >>>>>>> -    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 ?
> >>>> Well. I still think my patch is the right way to do it. For formats
> >>>> which don't have free text keys, the demuxer should export the small
> >>>> set of allowed tags each with a comprehensible text key.
> >>> Well, I'm still not happy with the language mechanism, but oh well...
> >>>
> >>>> So finally, here is the demuxer part of the patch, split, but 
> >>>> otherwise unchanged. First patch switch to the new API. Second patch
> >>>> adds support for a few more tags.
> >>> I also think that before adding more tags we should clean the code by
> >>> using an array.
> >> OK. Applied the demuxer patch. I will send a patch to use an array for
> >> keys/atom.
> > 
> > Here is a patch using an array for keys/atom.
> 
> Hummm I didn't realize it would require messing with mov_read_default,

I didn't realized it at first, either. I was about to send a patch
without touching mov_read_default when I finally realized that it
was incomplete.

> So you can just add the new tags, sorry.

Don't worry. Applied.

Aurel