[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