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

[Michael Niedermayer]

Hi

On Tue, Feb 27, 2007 at 09:44:36AM +0800, Limin Wang wrote:
> Hi,
> 
> * Michael Niedermayer <michaelni at gmx.at> [2007-02-26 15:40:19 +0100]:
> 
> > Hi
> > 
> > On Mon, Feb 26, 2007 at 07:55:09PM +0800, Limin Wang wrote:
> > > Hi,
> > > 
> > > The attached patch try to cleanup ffmpeg.c av_encode function, I have pass
> > > "make test" test. Please have review and give your comments. I don't know
> > > whether ffmpeg developer like big function?
> > > 
> > > I feel it has too many lines code and too difficult to maintain and develop.
> > > So I split it into three functions:
> > > av_transcode_init(): ffmpeg transcode initialization
> > > av_transcode_start(): main loop of file transcode
> > > av_transcode_close(): ffmpeg transcode close, including resource/mem release,
> > > the memory release order is first allocate(in av_transcode_init), last release.
> > > 
> > > If the patch is OK, I'll split av_transcode_init() into more functions for it's
> > > too big still.
> > 
> > [...]
> > 
> > > Index: ffmpeg.c
> > > ===================================================================
> > > --- ffmpeg.c	(revision 8129)
> > > +++ ffmpeg.c	(working copy)
> > > @@ -277,6 +277,14 @@
> > >      int nb_streams;       /* nb streams we are aware of */
> > >  } AVInputFile;
> > >  
> > > +typedef struct AVTranscodeContext {
> > > +    AVOutputStream **ost_table;
> > > +    AVInputStream **ist_table;
> > > +    AVInputFile *file_table;
> > > +    int nb_istreams;
> > > +    int nb_ostreams;
> > > +} AVTranscodeContext;
> > 
> > i see no sense in this struct the variables should stay globals as they are
> > global to the process, moving them into a struct just makes it harder to
> > access them
> > 
> > also adding such a struct must be in a seperate patch and commit from
> > other unrelated changes like spliting av_encode()
> 
> OK, I have updated the patch to use global variables directly.
> 
> 
> > spliting av_encode() certainly is welcome, but a patch doing so should
> > be quite small and rather look like:
> > 
> > -av_encode(){
> > +av_encode_init(){
> > ...
> > +}
> > +av_encode_main(){
> > ...
> > +}
> > +av_encode_close(){
> > 
> > 
> > +av_encode(){
> > +   av_encode_init();
> > +   av_encode_main();
> > +   av_encode_close();
> > +}
> 
> Have updated the patch to reflect it. Please review it again.

could you reorder the code so that  the patch size is minimized?
like i suggested above, that is ..._close() after ..._main()


[...]
>      if (!file_table)
> -        goto fail;
> +        goto fail_nomem;
>  

that is nice and i would like it in svn but its a seperate issue and
should be in a seperate patch and commit


[...]

> @@ -1458,7 +1572,8 @@
>                      stream_maps[n-1].stream_index;
>  
>                  /* Sanity check that the stream types match */
> -                if (ist_table[ost->source_index]->st->codec->codec_type != ost->st->codec->codec_type) {
> +                if (ist_table[ost->source_index]->st->codec->codec_type
> +                    != ost->st->codec->codec_type) {

cosmetic change


[...]
> @@ -1728,7 +1843,8 @@
>                  exit(1);
>              }
>              if (avcodec_open(ost->st->codec, codec) < 0) {
> -                fprintf(stderr, "Error while opening codec for output stream #%d.%d - maybe incorrect parameters such as bit_rate, rate, width or height\n",
> +                fprintf(stderr, "Error while opening codec for output stream #%d.%d "
> +                        "- maybe incorrect parameters such as bit_rate, rate, width or height\n",
>                          ost->file_index, ost->index);

another cosmetic change, and there are more, cosmetic changes must be
in seperate patches then functional changes

[...]
> +done:
> +    return ret;
> +
> +fail1:
> +    av_encode_close();
> +    goto done;
> +
> +fail_nomem:
> +    ret = AVERROR(ret);
> +    goto fail1;
> +}

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/20070228/89b29d99/attachment.pgp>