[MPlayer-dev-eng] [PATCH] More accurate af_volume

[Reimar Döffinger]

Hi,
>      else{
>        // Cutoff set to 10Hz for forgetting factor
> +      s->fast = 0;

What is this needed for?

> -	af_msg(AF_MSG_INFO,"[volume] The maximum volume was %0.2fdB \n", m);
> +    } else {
> +	m = (1.0/SHRT_MAX) * s->imax;
> +	af_to_dB(1, &m, &m, 20.0);
>      }
> +    af_msg(AF_MSG_INFO,"[volume] The maximum volume was %0.2fdB \n", m);

Changing the indentation of that af_msg is cosmetics, which is not
allowed by our CVS policy.

> @@ -145,10 +150,18 @@ static af_data_t* play(struct af_instanc
>      int         len = c->len/2;			// Number of samples
>      for(ch = 0; ch < nch ; ch++){
>        if(s->enable[ch]){
> -	register int vol = (int)(255.0 * s->level[ch]); 
> +	int vol = (int)(65536.0 * s->level[ch]); 
> +	int xmax = ((SHRT_MAX<<16) + 0x7fff) / vol;
> +	int xmin = -xmax - 0x10001 / vol;
>  	for(i=ch;i<len;i+=nch){
> -	  register int x = (a[i] * vol) >> 8;
> -	  a[i]=clamp(x,SHRT_MIN,SHRT_MAX);
> +	  int x = a[i];
> +	  if (x > s->imax)
> +	      s->imax = x;
> +	  if (-x > s->imax)
> +	      s->imax = -x;
> +	  x = clamp(x, xmin, xmax);
> +	  x = (vol * x + 0x8000) >> 16;
> +	  a[i]=x;
>  	}

I would really hate to bloat a performance-critical so much.
First, getting the maximum really should be a separate filter IMHO, this
has no place in the volume filter (and believe me, those two ifs can
easily mean a 50% performance hit). The fact that it was already done
for the float case does not change this...
Next, I don't understand why you need to clamp before scaling?
And if that rounding really is worth one add more...
Also I think the register before int vol should stay.
And last but not least, I think your code assumes int is 32 bit, right?
Is there an architecture where this is not the case? And if yes, how to
handle this case?

Greetings,
Reimar Döffinger