[FFmpeg-devel] [PATCH] parse udta in mov.c

[Benoit Fouet]

Hi Baptiste,

Baptiste Coudurier wrote:
> Hi Benoit,
>
> Benoit Fouet wrote:
>   
>> Hi,
>>
>> here is a patch to fill title, author, copyright and comment fields by
>> parsing udta
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: libavformat/mov.c
>> ===================================================================
>> --- libavformat/mov.c	(revision 9767)
>> +++ libavformat/mov.c	(working copy)
>> @@ -1036,6 +1036,60 @@
>>      return mov_read_default(c, pb, atom);
>>  }
>>  
>> +static void mov_parse_udta_string(ByteIOContext *pb, char *str,
>> +                                  int size, int atom_size)
>>     
>
> Don't break line in mov.c, there's no reason to.
>
>   

ok

>> +{
>> +    int i;
>> +    uint16_t str_size;
>> +
>> +    str_size = get_be16(pb); /* string length */
>>     
>
> You can merge declaration:
>
> uint16_t str_size = get_be16(pb);
>
>   
>> +               get_be16(pb); /* skip language */
>> +    for(i = 0; i < FFMIN(size, str_size); i++)
>> +        str[i] = get_byte(pb);
>>     
>
> Can't get_buffer be used here ?
>
>   

indeed

>> [...]
>>
>> +
>> +    while ( size + 8 < atom.size )
>> +    {
>>     
>
> Don't put a space after and before parenthesis, and put brace on the
> same line (coding style in mov.c):
>
> while (size + 8 < atom.size) {
>
>   

ok

>> +        uint32_t tag, atom_size;
>> +
>> +        atom_size = get_be32(pb);
>> +        tag       = get_le32(pb);
>>     
>
> I think tag_size instead of atom_size is more obvious and avoids a
> confusion with atom'.'size
>
> You can merge declaration too.
>
>   
>> +        switch (tag)
>> +        {
>>     
>
> Put brace on the same line.
>
>   

ok

>> +        case MKTAG(0xa9,'n','a','m'):
>> +            mov_parse_udta_string(pb, c->fc->title,
>> +                                  sizeof(c->fc->title), atom_size);
>>     
>
> Don't break line.
>
>   
>> +            break;
>> +        case MKTAG(0xa9,'w','r','t'):
>> +            mov_parse_udta_string(pb, c->fc->author,
>> +                                  sizeof(c->fc->author), atom_size);
>> +            break;
>> +        case MKTAG(0xa9,'c','p','y'):
>> +            mov_parse_udta_string(pb, c->fc->copyright,
>> +                                  sizeof(c->fc->copyright), atom_size);
>> +            break;
>> +        case MKTAG(0xa9,'i','n','f'):
>> +            mov_parse_udta_string(pb, c->fc->comment,
>> +                                  sizeof(c->fc->comment), atom_size);
>> +            break;
>> +        default:
>> +            url_fskip( pb, atom_size - 8 );
>> +            break;
>> +        }
>> +
>> +        size += atom_size;
>>     
>
> Maybe this is simpler:
>
> uint64_t end = url_ftell(pb) + atom.size;
>
> while (url_ftell(pb) + 8 < end) {
>     uint32_t tag_size = get_be32(pb);
>     uint32_t tag      = get_le32(pb);
>     uint64_t next     = url_ftell(pb) + tag_size - 8;
>
>     switch (tag) { ... }
>
>     url_fseek(pb, next, SEEK_SET);
> }
>
> You can then remove default skip, and remove skip at the end of
> parse_udta_string, and no need to pass atom_size.
>
>   

i liked it and used it :)

>> +    url_fskip(pb, atom.size - (url_ftell(pb) - atom.offset));
>>     
>
> Is it needed ? mov_read_default should skip garbage at the end of atoms.
>
>   

i think it's not needed, i removed it

how is this one ?

-- 
Ben
Purple Labs S.A.
www.purplelabs.com
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mov.c.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070731/f91104a4/attachment.txt>