[MPlayer-dev-eng] Chroma roll-off filter

[Reimar Doeffinger]

Hello,
On Thu, Jun 28, 2007 at 12:16:25AM +0800, Stanley wrote:
[...]
> 				if (i >= pos1 && i < pos2) {
> 					*dest = level_hi * (*src);
> 				} else if (i >= pos2 && i < pos3) {
> 					float level = (float)(pos3 - i) / (pos3 -pos2) * (level_hi - level_low)  + level_low;
> 					*dest = level * *src;
> 				} else {
> 					*dest = *src;
> 				}
> 			} else {
> 				if (i >= pos1 && i < pos2) {
> 					float level = (float)(i - pos1) / (pos2 -pos1) * (level_hi - level_low) + level_low;
> 					*dest = level * *src;
> 				} else if (i >= pos2 && i < pos3) {
> 					*dest = level_hi * *src;
> 				} else {
> 					*dest = *src;
> 				}
> 			}

This is horribly inefficient, you are doing two floating point
multiplications and one division per pixel.
You should do something like this IMO:
unsigned delta = (level_hi - level_low)/(pos2 - pos1) * (1 << 24);
unsigned level_low_int = level_low * (1 << 24);
in config, then in the loop:
unsigned level = level_low_int;
*dest = (level * *src) >> 24;
level += delta;
Should reduce it to one multiply, one shift and one add and all integer.
The calculations may be one-off, please check yourself (though it should
hardly be visible anyway).
Doing the comparisions in the inner loop is also less than optimal, you
could process the four different areas one after the other, thus also
taking advantage of the accelerated memcpy function for the copying.

[...]
> static int config(struct vf_instance_s* vf,
>         int width, int height, int d_width, int d_height,
> 	unsigned int flags, unsigned int outfmt){
> 
> 	// convert percentage to pixel
> 	vf->priv->pos1 = (int)(vf->priv->pos1 / 100 * width);
> 	vf->priv->pos2 = (int)(vf->priv->pos2 / 100 * width);
> 	vf->priv->pos3 = (int)(vf->priv->pos3 / 100 * width);

This will break horribly when config is called more than once, which I
think might easily happen.
At least there is no reason not to just add a few ints to the struct to
hold the values in pixel.

> #define ST_OFF(f) M_ST_OFF(struct vf_priv_s,f)
> static m_option_t vf_opts_fields[] = {
>   {"level_hi", ST_OFF(level_hi), CONF_TYPE_FLOAT, 0, 0 ,0, NULL},
>   {"level_low", ST_OFF(level_low), CONF_TYPE_FLOAT, 0, 0 ,0, NULL},
>   {"pos1", ST_OFF(pos1), CONF_TYPE_FLOAT, 0, 0 ,0, NULL},
>   {"pos2", ST_OFF(pos2), CONF_TYPE_FLOAT, 0, 0 ,0, NULL},
>   {"pos3", ST_OFF(pos3), CONF_TYPE_FLOAT, 0, 0 ,0, NULL},

Since all these have a valid range you should specify it.

>   {"direction", ST_OFF(direction), CONF_TYPE_INT, 0, 0 ,0, NULL},

And this allows only for two values for all I can see, specifying
horizontal or vertical.
So make it a flag and name it e.g. horizontal so everyone can see which
value is which direction without having to look at the manual. 

Greetings,
Reimar Doeffinger