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

[Baptiste Coudurier]

On 6/30/10 9:25 AM, Reimar D?ffinger wrote:
> On Wed, Jun 30, 2010 at 12:01:08AM -0700, Baptiste Coudurier wrote:
>> On 6/29/10 10:25 PM, Reimar D?ffinger wrote:
>>>>>>> +#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.
>
> This wasn't about this file in general, but the annoying task of changing
> the function arguments.
> I thought I had found a really nice way to avoid that pain in the future.
> Though since they are now all identical I guess sed works reasonably well
> now, too.
>
>>>           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;
>>>                   }

This hunk is ok after all. I agree there is nothing else to do anyway.

>> [...]
>>
>> 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.
>
> (removed for now in below patch despite following objections)

Patch is ok, and yes the int (*read)() situation sucked.

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