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

[Baptiste Coudurier]

Reimar D?ffinger wrote:
> 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.

I prefer the simplest way.

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

Well, I did that for the whole source file. Also, MXF is always BE.
MXFUint32 is what it's in the specs for example.

> Also, MXF_TT_FUNC has no corresponding type in the spec of course...
> I'd prefer if you could choose the names *g*

MXFParseFunction would be nice.

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

hummm MXFStrongRefArray is 8 + 16n, I find that MXF_TT_FUNC weird.
Maybe both treated as exceptions.

>>> +            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 do not agree here. Code is duplicated, right, but clear, simple and
obvious. IMHO if it's about factorization, factorize everything that can
be but without loosing simplicity.

That's a demuxer. Fixing later if code is obfuscated will be a pain in
the ass. MOV demuxer was a good example. Now it's almost clean and
clear, but contains some duplicated code.

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

That would mean non valid files. Specs are specs and SMPTE usually
really consider everything in the specs. Like I said, if we really want
to support broken files, that code needs to be really isolated. Your
file with '0' tag size is broken.

> I like this approach because it is quite flexible.

Generic parsing will be even more flexible: just add tag and where to
store based on type of object. MXF is using "object oriented" approach.

> Also I find those tag description tables far more readable than the
> current switch-case.

I don't, but that's my own point of you, don't take it badly, like I
find GXF demuxer code really obfuscated.

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

That might be good, yes:

case 0x....: mxf_read_strongref_array(pb, &refs_array); break;

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