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

[Reimar Döffinger]

Hello,
On Wed, Aug 02, 2006 at 03:14:55PM +0200, Baptiste Coudurier wrote:
> > 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 ?

Yes, but I like to it more explicit. But the "more correct" is more
about signed vs. unsigned.

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

Yes.

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

Sure I could do that, but I'd have to search the whole document for it
*g*
Also, MXF_TT_FUNC has no corresponding type in the spec of course...
I'd prefer if you could choose the names *g*

> >          }
> > -        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 ?

Well, there could be a length array for tag type, but how to handle
MXF_TT_FUNC or MXF_TT_UID_ARRAY?

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

True, but except for the extra checks (which if you insist I can remove)
it is exactly as ugly as the current code. But only once instead of how
many? five times?

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

Maybe, but if the end result will be really better? And what if one day
we wanted to add checks if maybe tags are used that the specs do not
allow in that section?
I like this approach because it is quite flexible.
Also I find those tag description tables far more readable than the
current switch-case.
Of course another possibility would be to keep the switch-cases as they
currently are and just add functions to read those UID-arrays. That
alone would be quite a progress IMHO.

Greetings,
Reimar D?ffinger