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

[Limin Wang]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

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

The variables is initialized and allocate in av_encode_init(), so if we keep
it as local variables of av_encode(), it'll cause av_encode_init() output 
these variables which will cause the code chaos. Current svn have no such
issue for all variables is local. How about to move these variables to 
global as a separate patch before do the av_encode() split? 

> 
> 
> [...]
> >                       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"

Sorry, I can't sure your meaning. If we define a context for av_encode, then
if av_encode_init failed, it should return NULL context, the invoker don't
need free anything, it's av_encode_init job.


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

How about to use these global variables instead of input from av_encode again?
I don't think it's good idea to input these variable for every av_encode_*
functions. If that's OK, I want to submit separate patch to fix these first.


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

Thanks for the guide, it helps a lot and I learn more ffmpeg develop rules.

Thanks,
Limin
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iQEVAwUBReze1Eztbf7dKiuoAQIW7gf/a4YXqMYImuc14jlVzhsp2Gjs9UNCVMa5
3aOjXV28/jkOUd7OuTMP5IYVEKxJY4a06VfZzuYck5WdF+nbxXxz9DbL9UzkLnIC
Wo03kgRE4tDlnnq0wVZG/v5sIR7W2AB3Bbqp7GT/TfEvA+vYZOYI/vejXi5+eMii
xvErAgU5vsa5Hr/GIORik6zR4vKjgMdOhFXozfQuHIdxWsklcoKgR7ZcajqWsy8E
pJ7ugeFZnwvTvofH2X3zzRuB2Hl0cD39E29vVF0uFsqviUf5wu6Utqw+MNjDokf2
XAU/IlIKWTbtMZtHHJMnXfwpBVDd9fTfz2Dp3ZjbfT7hfhFGgeG10g==
=ydV+
-----END PGP SIGNATURE-----