[Ffmpeg-devel] [PATCH] split av_encode function and do related reorder
Michael Niedermayer
michaelni
Fri Mar 2 13:28:56 CET 2007
Hi
On Fri, Mar 02, 2007 at 10:32:07AM +0800, Limin Wang wrote:
> Hi,
>
> * Michael Niedermayer <michaelni at gmx.at> [2007-03-01 17:43:11 +0100]:
>
> > Hi
> >
> > On Wed, Feb 28, 2007 at 11:56:00AM +0800, Limin Wang wrote:
> > [...]
> > > Please review the new patch again.
> > [...]
> > > +
> > > +done:
> > > + return ret;
> > > +
> > > +fail:
> > > + av_encode_close();
> > > + goto done;
> > > +
> > > +fail1:
> > > + ret = AVERROR(ENOMEM);
> > > + goto fail1;
> > > +}
> >
> > fail1:
> > ret = AVERROR(ENOMEM);
> > fail:
> > av_encode_close();
> > goto done;
> >
> > seems simpler and much faster (last time i tried 1 goto 1 was fairly slow)
>
> fixed, however fail1 will means no memory failure, so all these case will
> change to goto fail1.
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
[...]
>
> >
> >
> > > +
> > > + if( file_table )
> > > + av_free(file_table);
> > > +
> > > + /* close output files which opened in opt_output_file */
> > > + for(i=0;i<nb_output_files;i++) {
> > > + /* maybe av_close_output_file ??? */
> > > + AVFormatContext *s = output_files[i];
> > > + int j;
> > > +
> > > + pb = &s->pb;
> > > + if (!(s->oformat->flags & AVFMT_NOFILE) && pb )
> > > + url_fclose(pb);
> > > + for(j=0;j<s->nb_streams;j++) {
> > > + if( s->streams[j]->codec )
> > > + av_free(s->streams[j]->codec);
> > > + if( s->streams[j])
> > > + av_free(s->streams[j]);
> > > + }
> > > + av_free(s);
> > > + }
> > > +
> > > + /* close input files which opened in opt_input_file */
> > > + for(i=0;i<nb_input_files;i++)
> > > + av_close_input_file(input_files[i]);
> > > +
> > > + /* free memory which allocated in opt_intra_matrix */
> > > + if(intra_matrix)
> > > + av_free(intra_matrix);
> > > +
> > > + /* free memory which allocated in opt_inter_matrix */
> > > + if(inter_matrix)
> > > + av_free(inter_matrix);
> > > +
> > > + av_free_static();
> > > +
> > > +#ifdef CONFIG_POWERPC_PERF
> > > + extern void powerpc_display_perf_report(void);
> > > + powerpc_display_perf_report();
> > > +#endif /* CONFIG_POWERPC_PERF */
> > > +
> > > + if (received_sigterm) {
> > > + fprintf(stderr,
> > > + "Received signal %d: terminating.\n",
> > > + (int) received_sigterm);
> > > + exit (255);
> > > + }
> >
> > even though diff didnt match this up i still see that it contains
> > alot of unrelated changes
> > it would be great if you could reorder the functions so that diff
> > matches this code block up too, maybe by moving av_encode() somewhere
> > else, also try diff -du
> > if all fails iam fine with the not matched up blocks too but it would
> > be more readable in svnlog if everything matches
>
> Now I have reordered av_encode at the beginning to get more readable. For
> some free/close code are put at main function, it cause the patch is bigger
> than expected. Please review it again.
[...]
> /*
> * The following code is the main loop of the file converter
> @@ -1348,18 +1357,37 @@
> 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;
> +
trailing whitespace isnt allowed in svn
[...]
> if (!file_table)
> - goto fail;
> + goto fail1;
cosmetic change
[...]
> @@ -1977,20 +2041,8 @@
> }
> }
>
> - /* finished ! */
> -
> - ret = 0;
> - fail1:
> av_freep(&bit_buffer);
> - av_free(file_table);
>
> - if (ist_table) {
> - for(i=0;i<nb_istreams;i++) {
> - ist = ist_table[i];
> - av_free(ist);
> - }
> - av_free(ist_table);
> - }
> if (ost_table) {
> for(i=0;i<nb_ostreams;i++) {
> ost = ost_table[i];
> @@ -2011,10 +2063,60 @@
> }
> av_free(ost_table);
> }
> +
> + if (ist_table) {
> + for(i=0;i<nb_istreams;i++) {
> + ist = ist_table[i];
> + av_free(ist);
> + }
> + av_free(ist_table);
> + }
this code chunk has been reordered why? either way it doesnt belong into this
patch
> +
> + if( file_table )
> + av_free(file_table);
> +
> + /* close output files which opened in opt_output_file */
> + for(i=0;i<nb_output_files;i++) {
> + /* maybe av_close_output_file ??? */
> + AVFormatContext *s = output_files[i];
> + int j;
> +
> + if (!(s->oformat->flags & AVFMT_NOFILE))
> + url_fclose(&s->pb);
> + for(j=0;j<s->nb_streams;j++) {
> + av_free(s->streams[j]->codec);
> + av_free(s->streams[j]);
> + }
> + av_free(s);
> + }
> +
> + /* close input files which opened in opt_input_file */
> + for(i=0;i<nb_input_files;i++)
> + av_close_input_file(input_files[i]);
> +
> + av_free_static();
> +
> + /* free memory which allocated in opt_intra_matrix */
> + if(intra_matrix)
> + av_free(intra_matrix);
> +
> + /* free memory which allocated in opt_inter_matrix */
> + if(inter_matrix)
> + av_free(inter_matrix);
> +
> +#ifdef CONFIG_POWERPC_PERF
> + extern void powerpc_display_perf_report(void);
> + powerpc_display_perf_report();
> +#endif /* CONFIG_POWERPC_PERF */
> +
> + if (received_sigterm) {
> + fprintf(stderr,
> + "Received signal %d: terminating.\n",
> + (int) received_sigterm);
> + exit (255);
> + }
> +
[...]
> - /* close files */
> - for(i=0;i<nb_output_files;i++) {
> - /* maybe av_close_output_file ??? */
> - AVFormatContext *s = output_files[i];
> - int j;
> - if (!(s->oformat->flags & AVFMT_NOFILE))
> - url_fclose(&s->pb);
> - for(j=0;j<s->nb_streams;j++) {
> - av_free(s->streams[j]->codec);
> - av_free(s->streams[j]);
> - }
> - av_free(s);
> - }
> - for(i=0;i<nb_input_files;i++)
> - av_close_input_file(input_files[i]);
> -
> - av_free_static();
> -
> - if(intra_matrix)
> - av_free(intra_matrix);
> - if(inter_matrix)
> - av_free(inter_matrix);
> -
> -#ifdef CONFIG_POWERPC_PERF
> - extern void powerpc_display_perf_report(void);
> - powerpc_display_perf_report();
> -#endif /* CONFIG_POWERPC_PERF */
> -
> - if (received_sigterm) {
> - fprintf(stderr,
> - "Received signal %d: terminating.\n",
> - (int) received_sigterm);
> - exit (255);
> - }
> -
> exit(0); /* not all OS-es handle main() return value */
> return 0;
> }
this is not identical, it contains changes to comments and whitespace
at least
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()
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20070302/66816279/attachment.pgp>
More information about the ffmpeg-devel
mailing list