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

Michael Niedermayer michaelni
Thu Mar 1 17:43:11 CET 2007


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)



[...]
>      /* close each encoder */
>      for(i=0;i<nb_ostreams;i++) {
>          ost = ost_table[i];
> -        if (ost->encoding_needed) {
> -            av_freep(&ost->st->codec->stats_in);
> -            avcodec_close(ost->st->codec);
> +        if (ost && ost->encoding_needed) {
> +            if( ost->st->codec->stats_in)
> +                av_freep(&ost->st->codec->stats_in);
> +            if( ost->st->codec )
> +                avcodec_close(ost->st->codec);

i dont think these checks are needed, the one for av_freep is definitly not
and either way they would belong into a seperate bugfix patch


>          }
>      }
>  
>      /* close each decoder */
>      for(i=0;i<nb_istreams;i++) {
>          ist = ist_table[i];
> -        if (ist->decoding_needed) {
> -            avcodec_close(ist->st->codec);
> +        if (ist && ist->decoding_needed) {
> +            if( ist->st->codec )
> +                avcodec_close(ist->st->codec);

same as above


>          }
>      }
>  
> -    /* finished ! */
> +    if( bit_buffer )
> +        av_freep(&bit_buffer);

the check is unneeded


>  
> -    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];
> @@ -2012,12 +2046,87 @@
>          }
>          av_free(ost_table);
>      }
> +
> +   if (ist_table) {
> +        for(i=0;i<nb_istreams;i++) {
> +            ist = ist_table[i];
> +            if( ist )
> +                av_free(ist);
> +        }
> +        av_free(ist_table);
> +    }

cosmetic change and the if(ist) also isnt needed



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


[...]
>      ti = getutime();
> -    av_encode(output_files, nb_output_files, input_files, nb_input_files,
> -              stream_maps, nb_stream_maps);
> +
> +    /* start the file converter */
> +    av_encode();

hmm i would prefer if 
output_files, nb_output_files, input_files, nb_input_files, stream_maps, nb_stream_maps
would still be passed as arguments as they have been before but this is 
not important


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20070301/8255e5d1/attachment.pgp>



More information about the ffmpeg-devel mailing list