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

[Baptiste Coudurier]

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.

> +{
> +    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 ?

>
> [...]
>
> +
> +    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) {

> +        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.

> +        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.

> +    url_fskip(pb, atom.size - (url_ftell(pb) - atom.offset));

Is it needed ? mov_read_default should skip garbage at the end of atoms.

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312