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

[Michael Niedermayer]

Hi

On Wed, Aug 30, 2006 at 02:28:57PM +0200, Michel Bardiaux wrote:
> 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?

sure but my suggestion wouldnt need the ?: and without that the one liner
really seems to be the better solution


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

it will likely be similar yes


> 
> >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.

yes, and factoring that check out and putting it before the decission which
of the 3 functions is called also seems like its a good idea


> 
> >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?

of course it should be a seperate patch :)

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is