[FFmpeg-devel] [PATCH] moves ff_set_mpeg4_time to mpegvideo_enc.c

[Michael Niedermayer]

Hi

On Tue, Jul 10, 2007 at 11:20:46AM +0200, Aurelien Jacobs wrote:
> On Tue, 10 Jul 2007 10:55:39 +0200
> Michael Niedermayer <michaelni at gmx.at> wrote:
> 
> > Hi
> > 
> > On Tue, Jul 10, 2007 at 10:15:55AM +0200, Aurelien Jacobs wrote:
> > > On Tue, 10 Jul 2007 04:05:01 +0200
> > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > 
> > > > Hi
> > > > 
> > > > On Mon, Jul 09, 2007 at 11:48:16PM +0200, Aurelien Jacobs wrote:
> > > > > On Mon, 9 Jul 2007 18:07:42 +0200
> > > > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > > 
> > > > > > Hi
> > > > > > 
> > > > > > On Mon, Jul 09, 2007 at 03:16:10PM +0200, Aurelien Jacobs wrote:
> > > > > > > On Fri, 6 Jul 2007 10:44:55 +0200
> > > > > > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > > > > 
> > > > > > > > Hi
> > > > > > > > 
> > > > > > > > On Fri, Jul 06, 2007 at 01:51:20AM +0200, Aurelien Jacobs wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > The attached patch moves ff_set_mpeg4_time to mpegvideo_enc.c because
> > > > > > > > > it is only used by this file. This allows to make the function static.
> > > > > > > > > Moreover this function is used for mpeg1/2 as well as mpeg4 so it
> > > > > > > > > definitely don't belong in h263.c and it really deserve a renaming
> > > > > > > > > (as suggested by the FIXME comment).
> > > > > > > > > 
> > > > > > > > > Is it OK ?
> > > > > > > > 
> > > > > > > > well ...
> > > > > > > > 
> > > > > > > > the function should be split, some of the things are needed for all
> > > > > > > > b frame encodings some are mpeg4 specific, later belong in h263.c
> > > > > > > > not mpegvideo*.c unless you split mpeg4 out of h263.c ...
> > > > > > > 
> > > > > > > I'm not sure of what you mean exactly. The only mpeg4 specific thing
> > > > > > > in this function is the call to ff_mpeg4_init_direct_mv().
> > > > > > > I now moved it out of the function (see attached patch). Tell me
> > > > > > > if that's what you mean.
> > > > > > 
> > > > > > the only thing in this function which is not mpeg4 specific is
> > > > > > pb_time and pp_time and they are also almost not used outside
> > > > > > mpeg4
> > > > > > 
> > > > > > calling the function set_mpeg_time() is TOTALLY wrong, its mpeg4
> > > > > > specific, just out of convenience some non mpeg4 code also used
> > > > > > these variables
> > > > > > that is motion estimation and error resilience uses pb_time and pp_time
> > > > > > to find the distance between frames ...
> > > > > 
> > > > > OK. But calculation of pp_time and pb_time depends on all other values
> > > > > this function manipulate.
> > > > 
> > > > no it does not, it only needs half of them
> > > 
> > > I was very sure that they depends on everything, but I now just
> > > realized that I confused s->avctx->time_base and s->time_base :-(
> > > Sorry about that.
> > > 
> > > Now I think I see what you mean, so here is a new try.
> > 
> > better but
> > dont call it set_mpeg_time its not mpeg specific
> > maybe call it set_frame_distances or something
> > 
> > also it shouldnt call ff_set_mpeg4_time() as the 2 functions do quite
> > different things, ff_set_mpeg4_time() should be called explicitly
> 
> Ok. I think I got it right, now. Ok to apply ?

ok

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

No great genius has ever existed without some touch of madness. -- 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/20070710/fcbd75cf/attachment.pgp>