[Ffmpeg-devel] Re: [PATCH] MXF tag parser

[Baptiste Coudurier]

Hi

Reimar D?ffinger wrote:
> Hello,
> the attached patch implements a special mxf tag parsing function to
> avoid code duplication.
> It also has the side effect of doing more checks and playing my
> problematic (i.e. non-conformant) files while also showing a warning
> about the problem.
> It might be possible to optimize a bit further, esp. some names, but I
> think it is already quite helpful, esp. for avoiding copy-and-paste when
> adding support for more stuff.
> It also changes quite a few types from int to uint32_t, because
> 1) I think it's more correct

Isn't "int" always at least 32bit ? If types size are exactly like in
377M I'm all for it, can you please seperate that patch from the other ?

> 2) checks like "if (tmp_u32 >= UINT_MAX / sizeof(UID))" (before
> malloc(tmp_u32 * sizeof(UID)) ) are not correct for signed types AFAICT.

Humm, I'm not sure. Problem here is mul overflow. You mean negative
tmp_u32 ?

>  [...]
> +typedef enum {
> +    MXF_TT_BE32, MXF_TT_BE64, MXF_TT_FRAC,
> +    MXF_TT_UID, MXF_TT_ID, MXF_TT_BUF4,
> +    MXF_TT_UID_ARRAY, MXF_TT_FUNC,
> +    MXF_TT_LAST
> +} MXFTagType;

Please use 377M type names if possible. (MXFRational or Rational)

> [...]
>          }
> -        bytes_read += size + 4;
> +        switch (tagd->type) {
> +            case MXF_TT_BE32:
> +                if (size != 4) {skip = size; break;}

Can't the check be generic for all tags ?

> [...]
> +            case MXF_TT_UID_ARRAY:
> +                if (size < 8) {skip = size; break;}
> +                tmp_u32 = get_be32(pb);
> +                if (tmp_u32 >= UINT_MAX / sizeof(UID)) {skip = size - 4; break;}
> +                if (get_be32(pb) != sizeof(UID)) {skip = size - 8; break;}
> +                if (tmp_u32 * sizeof(UID) != size - 8) {skip = size - 8; break;}
> +                if (tmp_u32) {
> +                    tmp_puid = av_malloc(tmp_u32 * sizeof(UID));
> +                    get_buffer(pb, tmp_puid, tmp_u32 * sizeof(UID));
> +                }
> +                *(uint32_t *)tagd->dest = tmp_u32;
> +                *(UID **)tagd->dest2 = tmp_puid;

Ugly

> [...]

According to specs, static Local Tags are unique, and dynamic ones
unique to the partition they are used in.

I believe the whole parsing can be made in one function parsing all
needed tags with a switch, tag specified sizes in a table for quick look
or even using a uint32_t containing tag + size, since that will be also
 unique.

Then specific MetadataSet type allocated according to byte 15 of the KLV
key.

I need to apply another patch which respects the "Object Oriented
approach" of MXF.

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