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

[Michael Niedermayer]

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


> 
> > 
> > 
> > [...]
> > >                       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()

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/3517b178/attachment.pgp>