[MPlayer-dev-eng] [PATCH] vf_eq2 + vf_eq

[D Richard Felker III]

On Fri, Jan 31, 2003 at 11:26:49PM +0100, Hampa Hug wrote:
> D Richard Felker III wrote:
> 
> > On Fri, Jan 31, 2003 at 10:21:32PM +0100, Hampa Hug wrote:
> > > Based on the discussion in the other thread I made a new
> > > patch to vf_eq2.c that essentially combines vf_eq.c and
> > > vf_eq2.c. It is not well testet yet.
> > 
> > The idea is good, but I don't think you should be passing around
> > vf_eq2_t like that. Instead just pass the individual params that are
> > needed. Why? Well, for one thing, adjust_y_*() can also do saturation,
> > with no changes, except that you have to pass it the saturation values
> > and u plane info. Also adjust_u and adjust_v are identical except for
> > which params they use, so it's wasteful (and cache-inefficient) to
> > call one then the other instead of calling the same function twice
> > with different params.
> 
> I thought about that. But then you can't use function pointers
> anymore because not all functions (that do the same thing) take
> the same parameters (c/b values vs. lut). On the other hand
> I'm not happy with the way it is now. How would you define
> the three function pointers?

Two approaches I see:

1) Always pass brightness, contrast, gamma, and lut even to functions
   that don't need them. There's a stupid compiler warning that will
   complain about unused arguments, but that's lame and should be
   disabled anyway.

2) Only use function pointers when there are multiple implementations
   of the same function (i.e. C and asm/vector versions), and use
   actual conditionals for different algorithms.

IMHO approach #2 is cleaner and makes more sense to people reading the
code, but it's your call. Of course you could sorta combine methods
with small wrapper functions that take the vf_eq2_t pointer and then
call the real implementation functions, but that seems sorta wasteful
and silly.

> > Also, you should name the functions by what they do. E.g.
> > adjust_y_MMX -> affine_1d_MMX or something, rather than just
> > "adjust_{plane}".
> 
> ACK.

Hm? ACK = acknowledge, or ACK = disgusting? :)

Rich