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

Panagiotis Issaris takis.issaris
Mon Jan 29 17:27:35 CET 2007


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.

With friendly regards,
Takis

-- 
vCard: http://www.issaris.org/pi.vcf
Public key: http://www.issaris.org/pi.key
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pi-20070129T171911-ffmpeg-motion_estimation.diff
Type: text/x-patch
Size: 81279 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070129/bd0c0ac5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070129/bd0c0ac5/attachment.pgp>



More information about the ffmpeg-devel mailing list