[Ffmpeg-devel] [PATCH] split av_encode function and do related reorder

[Michael Niedermayer]

Hi

On Mon, Mar 05, 2007 at 02:32:48PM +0800, Limin Wang wrote:
> Hi,
> 
> * Limin Wang <lance.lmwang at gmail.com> [2007-03-05 10:38:43 +0800]:
> 
> > Hi,
> > 
> > * Michael Niedermayer <michaelni at gmx.at> [2007-03-02 13:28:56 +0100]:
> > 
> > > Hi
> > > 
> > > well, then use fail instead, your patch used fail1 for that i just fixed the
> > > infinite loop i didnt check that the labels where correct, if they where not
> > > than of course it has to be corrected too instead of renaming all in the
> > > function
> > 
> > fixed, some place should go to fail1, will fix it after the patch is ok.
> > 
> > 
> > > trailing whitespace isnt allowed in svn
> > 
> > fixed, forget to delete-trailing-whitespace by emacs.
> >  
> > > this code chunk has been reordered why? either way it doesnt belong into this
> > > patch
> > 
> > OK, in  order to keep it untouched, I modify the ffmpeg.c directly now.
> > 
> > > this is not identical, it contains changes to comments and whitespace
> > > at least
> > 
> > should be fixed, I copy and paste directly.
> > 
> >  
> > > also i dont think the closing code from main() even belongs into
> > > av_encode_close() freeing things allocated during command line
> > > parsing is not av_encode_close() job it wasnt alocated in av_encode_main() or
> > > _init()
> > 
> > yes,  add av_exit() to do the other close job. Please review it again, now
> > it'll more readable than before.
> 
> Sorry, attached patch isn't the newest, please review the new attached patch.
> 
> 
> Thanks,
> Limin

> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 8239)
> +++ ffmpeg.c	(working copy)
> @@ -276,6 +276,12 @@
>      int nb_streams;       /* nb streams we are aware of */
>  } AVInputFile;
>  
> +static AVOutputStream **ost_table = NULL;
> +static AVInputStream **ist_table = NULL;
> +static AVInputFile *file_table = NULL;
> +static int nb_istreams = 0;
> +static int nb_ostreams = 0;

these should stay local variables of av_encode and passed as arguments to
the other av_encode_*() functions


[...]
>                       int nb_input_files,
>                       AVStreamMap *stream_maps, int nb_stream_maps)
>  {
> -    int ret, i, j, k, n, nb_istreams = 0, nb_ostreams = 0;
> -    AVFormatContext *is, *os;
> -    AVCodecContext *codec, *icodec;
> -    AVOutputStream *ost, **ost_table = NULL;
> -    AVInputStream *ist, **ist_table = NULL;
> -    AVInputFile *file_table;
> -    AVFormatContext *stream_no_data;
> -    int key;
> +    int ret;
>  
> +    ret = av_encode_init();
> +    if( ret < 0 ) {
> +        fprintf(stderr, "av_encode_init failed\n");
> +        exit(1);
> +    }

this is not a proper way to return an error from a function
also dont forget ALL the needed free and close a return -1 will
almost certainly be wrong, and this did work before your "cleanup"


> +
> +    av_encode_main();

return value is ignored


> +
> +    av_encode_close();

return value is ignored

also the agruments to av_encode() are completely ignored and not passed
into the other functions instead they are accessed directly this is not
cleaner or better then the current code in svn at all
*  a patch must contain just one seperate change (that is spliting a function
   for example) (making local variables global is a seperate change)
*  a patch must not break anything that worked before, like passing
   stream_maps/nb_stream_maps which differ from the global ones into
   av_encode()
*  you should review your patch and not expect me to do your work telling
   you every single problem your code has
   nor should you expect me to figure out exactly how to solve each problem
   if i did that i could as well split av_encode() myself

for further details see http://ffmpeg.mplayerhq.hu/ffmpeg-doc.html#SEC35

and please keep in mind reviewing patches takes time, so please check that
your patch is ok before you send it, that safes me and you some work as
i wont accept it anyway if its not perfect

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070306/c4a52a23/attachment.pgp>