[FFmpeg-devel] [PATCH] mxfdec: make it work with other calling conventions

[Baptiste Coudurier]

On 6/29/10 2:35 PM, Reimar D?ffinger wrote:
> On Tue, Jun 29, 2010 at 01:02:05PM -0700, Baptiste Coudurier wrote:
>> On 6/29/10 11:51 AM, Reimar D?ffinger wrote:
>>> On Tue, Jun 29, 2010 at 08:30:01PM +0200, Reimar D?ffinger wrote:
>>>> Hello,
>>>> currently mxfdec assumes that it can unpunished pass more arguments to functions
>>>> than they were declared with.
>>>> This is not true in general, in particular not for stdcall.
>>>> While I am not aware of any FFmpeg platform using it, IMHO this code is still
>>>> wrong.
>>>> Attached is a patch that fixes it, and I think it is not particularly bad.
>>>> It also fixes the last remaining warnings ("function declaration is not a prototype")
>>>> in that file.
>>
>> I don't have this warning.
>> gcc version 4.4.4 (Ubuntu 4.4.4-6ubuntu2)
>
> Right, I had been using -Wstrict-prototypes
>
>>> +#define METADATA_READ_FUNC(name) int name(void *arg, ByteIOContext *pb, int tag, int size, UID uid)
>>
>> I don't like the #define.
>
> Well, the alternative is having to change every single read function
> manually in case you'd ever need an additional parameter.
> And I found it very hard to find them.

And when you add a new function, you have to look up for the define to 
know which parameters are available. This is even more painful.

>>> @@ -919,11 +931,14 @@
>>>               url_fseek(s->pb, klv.offset, SEEK_SET);
>>>               break;
>>>           }
>>> +        if (IS_KLV_KEY(klv.key, mxf_primer_pack_key)) {
>>> +            mxf_read_primer_pack(mxf);
>>> +            continue;
>>> +        }
>>>
>>>           for (metadata = mxf_metadata_read_table; metadata->read; metadata++) {
>>>               if (IS_KLV_KEY(klv.key, metadata->key)) {
>>> -                int (*read)() = klv.key[5] == 0x53 ? mxf_read_local_tags : metadata->read;
>>> -                if (read(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type)<   0) {
>>> +                if (mxf_read_local_tags(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type)<   0) {
>>
>> This mechanism makes it easier to add new functions.
>> Several metadata use 0x53 and it is needed later.
>
> Could I get that in understandable?
> klv.key[5] == 0x53 is true for all except one, and even if it was a
> second table would not be that much additional code, without having
> to rely on key[5] corresponding to what the code needs to do.
> The alternative is to change the signature of mxf_read_primer_pack to
> match mxf_read_local_tags.

The contrary, next time we add a function parsing metadata not using 
0x53, we will need an ugly if (key), and index tables do not use 0x53 
for example.

So the alternative solution is prefered.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org