[Ffmpeg-devel] [PATCH] Wildcard and catalog image sequences

[Michel Bardiaux]

Michael Niedermayer wrote:
> Hi
> 
> On Wed, Aug 30, 2006 at 02:00:38PM +0200, Michel Bardiaux wrote:
>> Michael Niedermayer wrote:
>>> Hi
>>>
>>> On Wed, Aug 30, 2006 at 12:38:30PM +0200, Michael Niedermayer wrote:
>>> [...]
>>>>>>> +static void free_catalog(int last_index, char*** pindex_vector)
>>>>>>> +{
>>>>>>> +    int i;
>>>>>>> +    char** index_vector = *pindex_vector;
>>>>>>> +    for(i=0;index_vector&&i<=last_index;i++)
>>>>>>> +        av_free(index_vector[i]);
>>>>>>> +    av_free(index_vector);
>>>>>>> +    *pindex_vector = NULL;
>>>>>> av_freep()
>>>>>> and the index_vector variable seems unneeded
>>>>> Wilco, but my code was actually correct. It seems to me you *require* 
>>>>> compact code and are ready to reject a patch simply because you find the 
>>>>> coding style too verbose for your taste!
>>>> yes, code must be simple, patches must be minimal, no superfluous changes
>>> the coding rules also say that:
>>> "Main priority in FFmpeg is simplicity and small code size (=less bugs). "
>>> this has been written by fabrice IIRC and was there since a very long time
>>>
>>> [...]
>> I would dispute that a 4-liner instead of a 1-liner using ?: generates 
>> more object code; and it does not seem *simpler* to me. I think you tend 
>> too much towards compactness even at the price of obfuscation. But, OK, 
>> I'll try to respect your wishes.
> 
> looking at for example:
> 
>>> +int filename_catalog_test(const char *filename)
>>> +{
>>> +    if(!filename)
>>> +        return (-1);
>>> +    else if(filename[0]=='@')
>>> +        return 0;
>>> +    else
>>> +        return (-1);
> 
> versus
> 
>> return filename && filename[0]=='@';
> 
> i cant see how the later is more obfuscated, additionally

I dont mean all one-liners are obfuscated, only that they quickly become 
so. Esp. since the correct one is

return (filename && filename[0]=='@')?0:-1; // justified below.

Still reasonable, but the one about wildcards is even bigger. If one has 
to code for multiple levels of conditions, nested ?: become quite 
unreadable. So there is a limit to one-liners, and where it is is a 
matter of taste, isnt it?

You dont dispute that the object code size is likely to be the same?

> 1. is the NULL check really needed at all?

I dont know: there was one in the original filename_number_test() so I 
put one too. Keeping maximum symmetry between all 3 kinds of sequences 
seemed a good strategy for implementation.

> 2. in C 0 is false not 0 is true, using 0 as true is broken as logical and/or
>    will no longer have their intuitive meaning, do you disagree here?

Not at all. I knew you want patches to be as little intrusive as 
possible, so I tried not to change anything to numbered sequences, which 
are known to work. Hence I had to conform to the API defined by 
get_frame_filename and filename_number_test, which is on the lines of 
UNIX-style syscalls. Maybe this has to be changed, but shouldnt that be 
a different patch?

Greetings,
-- 
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:mbardiaux at mediaxim.be

Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/