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

[Limin Wang]

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

Hi,

* Michael Niedermayer <michaelni at gmx.at> [2007-03-06 13:38:41 +0100]:

> Hi
> 
> On Tue, Mar 06, 2007 at 11:24:04AM +0800, Limin Wang wrote:
> > 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. 
> 
> well you could pass a pointer to them, and iam not saying you should just that
> this would be one possible solution

[...]

> 
> > 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? 
> 
> no, iam against making local variables global, i originally misstakely thought
> they where global already before your patch ...

Then I'm glad to ask for to define a av_encode context like my first patch. Or
av_encode_init() will have 6 input(global variable) + 5 output parameters and
the patch size will double increase, I can't accept this solution.

> > > >                       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.
> 
> yes but it doesnt free anything in case of failure with your patch
> also if av_encode() fails it should return an error not exit()

No, if av_encode_init failed, it'll call av_encode_close internally to free
the allocate resource. Yes, av_encode_init should return an error not exit(),
originally I free all resource in av_encode_close, so exit() is OK before.
The original av_encode didn't check its return code, so av_encode_main/close
follow it.

Many exit() exist in av_encode_init() already, cleanup them first?


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

iQEVAwUBRe4gOEztbf7dKiuoAQIwoAf/RV9yh8PUfzhUSd8mGz3U2P+ud8JR/mN2
1ft+0LcM42cPVkhGMyKu0yfRlq8eASF8A2IOux2ERw91tLsocZdTBALTQRaVCiZy
nwROpYQqe1FhGG84tFgDGadgz93z71zYQTtu6r/WP3KEC83grJrDTWUS2UWPW9sX
k74fekObx5/e76ayWuwT2k8LHjeSOLywJpfF1vUcUONgYNHXM4exB61Nu7aE51tN
WluyhL3sY7pI3VaitdbgC9FX4RlrZL+qfWD57mOLMD0Z/tRjhJN/X1AEDZJ9BDzv
qP2CpSotEzAvjzfouczfZ3hMu2LqQI2CywjG/8C39mcVF+FPtzD35g==
=tTUT
-----END PGP SIGNATURE-----