[Ffmpeg-devel] Motion estimation related refactoring (was: Native H.264 encoder)

[Michael Niedermayer]

Hi

On Mon, Jan 29, 2007 at 05:27:35PM +0100, Panagiotis Issaris wrote:
> Hi,
> 
> On Fri, 2007-01-19 at 15:36 +0100, Panagiotis Issaris wrote:
> > Hi,
> > 
> > On Thu, 2007-01-18 at 18:17 +0100, Michael Niedermayer wrote:
> > > On Thu, Jan 18, 2007 at 04:59:19PM +0100, Panagiotis Issaris wrote:
> > > [...]
> > > > Or do imply moving all the fields needed for motion estimation out of
> > > > MpegEncContext into MotionEstContext (at the top)?
> > > > 
> > > > struct MotionEstContext { 
> > > > /* new MotionEstContext fields */
> > > > int width;
> > > > ... 
> > > > /* older MotionEstContext fields */
> > > > ...
> > > > };
> > > > 
> > > > struct MpegEncContext { MotionEstContext me; ... };
> > > > struct SnowContext { MotionEstContext me; ... };
> > > 
> > > 
> > > i meant something like:
> > > #define ME_FIELDS\
> > > int width;\
> > > int height;\
> > > ...
> > > 
> > > struct MotionEstContext { 
> > > ME_FIELDS
> > > }
> > > 
> > > struct SnowContext {
> > > ME_FIELDS
> > > 
> > > ...
> > > }
> > > 
> > > struct MpegEncContext {
> > > ME_FIELDS
> > > 
> > > ...
> > > }
> > > 
> > > but the more i think about it the less i like it ...
> > > 
> > > the problem with your suggestion above is that it would result in code like
> > > s.me.width instead of s.width which is a big problem readability wise ...
> > 
> > Currently the MpegEncContext struct is huge imho:
> > sizeof(MpegEncContext): 8672
> > compared to f.e.:
> > sizeof(MotionEstContext): 344. 
> > 
> > Having a separate struct which contains only the variables which need to
> > be available for all codecs might ameliorate this problem a bit.
> > 
> > 
> >  struct BaseCodecContext {
> >  int width;
> >  ...
> >  };
> >  
> >  struct MotionEstContext { 
> >    BaseCodecContext sn;
> >    ... 
> >    
> >  };
> >  
> >  struct MpegEncContext { MotionEstContext me; ... };
> >  struct SnowContext { MotionEstContext me; ... };
> >  
> > If it appears first in the struct and you object to using s.me.width,
> > you can still cast it ;)
> The attached patch is in no way intended to be applied as it breaks _everything_.
> 
> I'm posting it just to get some feedback to determine whether it is
> worth proceeding and finishing such a patch. The patch basically pulls
> out everything out of MpegEncContext that is needed for motion_est.c and
> motion_est_template.c, meaning that the functions in motion_est.c and
> motion_est_template.c can use BaseCodecContext instead of MpegEncContext
> as their first parameter. Futhermore, I tried to adapt snow.c to match
> these changes. And, I started on adapting mpegvideo.[hc], but that work
> isn't finished yet. Before finishing it, I'd rather hear if such an
> approach would be acceptable. Or ofcourse if anyone would have any
> alternatives for getting motion estimation out of MpegEncContext.
> 
> Again, the patch is not meant for inclusion as it is _not_ working,
> breaks mpegvideo.c compilation and it makes snow encoding fail too.

iam against code like s->bc.dsp.idct_permutation its hard enough to read
as it is changing about half of the lines where MpegEncContext is accessed
to ->bc. style is not ok

also id prefer if the motion est cleanup could be done in small patches
>100k doesnt qualify and this patch also qualifies as a horrible 
cosmetic+functional mix

and the way this patch makes the motion estimation code independant isnt
really making it independant, a codec not based on the new BaseCodecContext
still needs to copy everything over, this is only slightly better then
with MpegEncContext

what i would rather try is to clean it up one variable at a time
look at qscale, theres just a single use of it, all others are commented out
and i belive that single case can safely be replaced by lambda with
appropriate scaling and a few PSNR per bitrate encodings at different
qualities (such encodings take near zero user time, just write a small
script which encodes 2 or 3 short <5min test files at several bitrates
from really low to really high and with user specified options) such a
script is really usefull for changes where the regression tests wont
help ...

then look at MpegEncContext.(uv)linesize its a duplicate of 
MotionEstContext.(uv)stride

or take msmpeg4_version, its used only for one thing and that is to limit
the MV range to 16 due to limitations of msmpeg4 this could as well be done
be the caller by passing an appropriately set range value

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20070130/5270349d/attachment.pgp>