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

[Baptiste Coudurier]

On 6/29/10 10:25 PM, Reimar D?ffinger wrote:
> On Tue, Jun 29, 2010 at 04:47:32PM -0700, Baptiste Coudurier wrote:
>> 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.
>
> Have you actually _tried_ changing the parameters of all those functions?
 >
> I had to pick each single one from the table, search for it's declaration
> and then change it.
> Searching for the define to get the argument names is quite simple in
> comparison.
> But you have to maintain it, I'll change it if that's the only objection.
>

I do know for sure that I've been working way more with that file than 
you. Please remove the define.

>>>>> @@ -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.
>
> As said, a second table is possible, or it is possible to stay
> closer to the current code with attached patch.
>
> [...]
>
> @@ -922,8 +933,12 @@
>
>           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) {
> +                int res;
> +                if (klv.key[5] == 0x53) {
> +                    res = mxf_read_local_tags(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type);
> +                } else
> +                    res = metadata->read(mxf, s->pb, 0, 0, NULL);
> +                if (res<  0) {
>                       av_log(s, AV_LOG_ERROR, "error reading header metadata\n");
>                       return -1;
>                   }
>

What you do is only cosmetic changes. Please keep the code the way it 
is. If we need to change the prototype because a feature requires it, we 
will change it.

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