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

Baptiste Coudurier baptiste.coudurier
Wed Jun 30 01:47:32 CEST 2010

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.

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

More information about the ffmpeg-devel mailing list