[Ffmpeg-devel] [PATCH] split av_encode function and do related reorder
Limin Wang
lance.lmwang
Fri Mar 2 03:32:07 CET 2007
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.
>
>
> [...]
> > /* 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
fixed
>
> > }
> > }
> >
> > /* 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
fixed
>
> > }
> > }
> >
> > - /* finished ! */
> > + if( bit_buffer )
> > + av_freep(&bit_buffer);
>
> the check is unneeded
fixed
>
> >
> > - 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
fixed.
>
>
> > +
> > + 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.
Thanks,
Limin
-------------- next part --------------
Index: ffmpeg.c
===================================================================
--- ffmpeg.c (revision 8182)
+++ ffmpeg.c (working copy)
@@ -276,6 +276,12 @@
int nb_streams; /* nb streams we are aware of */
} AVInputFile;
+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;
+
#ifndef __MINGW32__
/* init terminal so that we can grab keys */
@@ -1338,6 +1344,9 @@
return -1;
}
+static int av_encode_init();
+static int av_encode_main();
+static int av_encode_close();
/*
* 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;
+
+ ret = av_encode_init();
+ if( ret < 0 ) {
+ fprintf(stderr, "av_encode_init failed\n");
+ exit(1);
+ }
+ av_encode_main();
+
+ av_encode_close();
+
+ return 0;
+}
+
+/*
+ * The following code initialize encode
+ */
+static int av_encode_init()
+{
+ int ret=0, i, j, k, n;
+ AVOutputStream *ost;
+ AVInputStream *ist;
+ AVFormatContext *is;
+ AVFormatContext *os;
+ AVCodecContext *codec;
+ AVCodecContext *icodec;
+
file_table= (AVInputFile*) av_mallocz(nb_input_files * sizeof(AVInputFile));
if (!file_table)
- goto fail;
+ goto fail1;
/* input stream init */
j = 0;
@@ -1373,12 +1401,12 @@
ist_table = av_mallocz(nb_istreams * sizeof(AVInputStream *));
if (!ist_table)
- goto fail;
+ goto fail1;
for(i=0;i<nb_istreams;i++) {
ist = av_mallocz(sizeof(AVInputStream));
if (!ist)
- goto fail;
+ goto fail1;
ist_table[i] = ist;
}
j = 0;
@@ -1435,11 +1463,11 @@
ost_table = av_mallocz(sizeof(AVOutputStream *) * nb_ostreams);
if (!ost_table)
- goto fail;
+ goto fail1;
for(i=0;i<nb_ostreams;i++) {
ost = av_mallocz(sizeof(AVOutputStream));
if (!ost)
- goto fail;
+ goto fail1;
ost_table[i] = ost;
}
@@ -1547,7 +1575,7 @@
switch(codec->codec_type) {
case CODEC_TYPE_AUDIO:
if (av_fifo_init(&ost->fifo, 2 * MAX_AUDIO_PACKET_SIZE))
- goto fail;
+ goto fail1;
if (codec->channels == icodec->channels &&
codec->sample_rate == icodec->sample_rate) {
@@ -1606,7 +1634,7 @@
avcodec_get_frame_defaults(&ost->pict_tmp);
if( avpicture_alloc( (AVPicture*)&ost->pict_tmp, codec->pix_fmt,
codec->width, codec->height ) )
- goto fail;
+ goto fail1;
}
}
if (ost->video_resample) {
@@ -1689,7 +1717,7 @@
if (!bit_buffer)
bit_buffer = av_malloc(bit_buffer_size);
if (!bit_buffer)
- goto fail;
+ goto fail1;
/* dump the file output parameters - cannot be done before in case
of stream copy */
@@ -1819,6 +1847,29 @@
fprintf(stderr, "Press [q] to stop encoding\n");
url_set_interrupt_cb(decode_interrupt_cb);
}
+
+done:
+ return ret;
+fail1:
+ ret = AVERROR(ENOMEM);
+fail:
+ av_encode_close();
+ goto done;
+}
+
+/*
+ * The following code is the main loop of the file converter
+ */
+static int av_encode_main()
+{
+ int i;
+ AVFormatContext *stream_no_data;
+ AVOutputStream *ost;
+ AVInputStream *ist;
+ AVFormatContext *is;
+ AVFormatContext *os;
+ int key;
+
term_init();
stream_no_data = 0;
@@ -1960,6 +2011,19 @@
/* dump report by using the first video and audio streams */
print_report(output_files, ost_table, nb_ostreams, 1);
+ return 0;
+}
+
+/*
+ * The following code close the av encode resource
+ */
+static int av_encode_close()
+{
+ int ret = 0;
+ int i;
+ AVOutputStream *ost;
+ AVInputStream *ist;
+
/* close each encoder */
for(i=0;i<nb_ostreams;i++) {
ost = ost_table[i];
@@ -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);
+ }
+
+ 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);
+ }
+
return ret;
- fail:
- ret = AVERROR(ENOMEM);
- goto fail1;
}
#if 0
@@ -3798,41 +3900,6 @@
printf("bench: utime=%0.3fs\n", ti / 1000000.0);
}
- /* 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;
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 481 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070302/469b5036/attachment.pgp>
More information about the ffmpeg-devel
mailing list