[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