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

[Panagiotis Issaris]

Hi,

On Thu, 2007-01-18 at 15:12 +0100, Michael Niedermayer wrote:
> On Thu, Jan 18, 2007 at 01:07:03PM +0100, Panagiotis Issaris wrote:
> > On Mon, 2006-12-18 at 22:04 +0100, Michael Niedermayer wrote:
> > > On Fri, Dec 15, 2006 at 11:34:37PM +0100, Panagiotis Issaris wrote:
> > > [...]
> > > btw 2 tips
> > > 1. submit small patches, a 40k motion estimation cleanup patch would be
> > >    a nightmare for me and whoever submits it ...
> > > 2. look at snow.c and try to reduce the number of lines of code which
> > > depend on MpegEncContext if it reaches 0 then motion estimation can be
> > > used without MpegEncContext ...
> > I've been looking at a way to do this, and started by looking for each
> > access of the MpegEncContext field in the SnowContext struct.
> > 
> > [...]
> > Afterwards, I filtered out all references to members of the embedded
> > MotionEstContext and came to this list:
> > avctx, b8_stride, bit_rate, current_picture, current_picture_ptr, dsp,
> > f_code, flags, frame_bits, height, lambda, lambda2, last_picture,
> > last_picture_ptr, linesize, mb_height, mb_stride, mb_width, mb_x, mb_y,
> > me, me_method, misc_bits, mv_bits, new_picture, obmc_scratchpad,
> > out_format, pict_type, picture_number, p_tex_bits, qscale,
> > quarter_sample, rc_context, total_bits, unrestricted_mv, uvlinesize,
> > width
> 
> some of these are just needed for the ratecontrol but not ME i think ...
Ah, okay. I'll try to figure out which ones are needed for ME.


> > Some of these fields are also available in SnowContext and are mostly
> > likely assigned to the equivalently named fields in the MpegEncContext
> > only to get motion estimation working:
> > avctx, dsp, new_picture, current_picture, last_picture, lambda, lambda2,
> > b_width (==mb_width?), b_height (==mb_height?)
> > 
> > So, the MotionEstContext apparently does not provide enough contextual
> > data for the motion estimation functions and therefore they need access
> > to the encompassing MpegEncContext. Would that be a good part to focus
> > on first? I'd guess one way to solve this would be to move the required
> > fields into the MotionEstContext, or another way would be to pass the
> > required field in the relevant function calls of the motion estimation
> > functions. Both would require quite a lot of changes as the
> > encode_frame() functions accesses quite a lot of MpegEncContext fields. 
> 
> yes
Ok.


> > Or should a new indepent context be created, that is not specific to
> > mpeg (and related codecs) encoding but contains enough info to allow
> > motion estimation? But, ... I'd guess the result would be nearly the
> > same as moving all the needed stuff to MotionEstContext, right?
> 
> thats an option too, but one very important thing is that the context
> can be accessed without an additional pointer dereference (like AVFrame)
> what i mean is that the common variables like width/height/... could
> be in a common context which is at the begin of MotionEstContext and
> MotionEstContext is part of MpegEncContext and SnowContext
Hmm... I am not sure I understand the above correctly. Are you
suggesting something like this?

struct SomeNewContext {
int width;
...
};

struct MotionEstContext { 
  SomeNewContext sn;
  ... 
  
};

struct MpegEncContext { MotionEstContext me; ... };
struct SnowContext { MotionEstContext me; ... };


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; ... };


The motion estimation functions that currently take a MpegEncContext
pointer as their first parameter, could then (hopefully) work like this:
do_some_part_of_motion_estimation(&mec.me, ...)


> no variable duplications and fast access but probably more work then
> simply duplicating variables in MotionEstContext and copying them
> over during init ...
That would be a lot simpler, but the other approach would lead to a
better and cleaner solution in the long term, right?

With friendly regards,
Takis
-- 
vCard: http://www.issaris.org/pi.vcf
Public key: http://www.issaris.org/pi.key