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

[Zoltan Hidvegi]

Reimar Döffinger wrote:
[ Charset ISO-8859-1 unsupported, converting... ]
> Hi,
> >      else{
> >        // Cutoff set to 10Hz for forgetting factor
> > +      s->fast = 0;
> 
> What is this needed for?

s->fast is used to request float format, and at the end, the it is
tested if the float max volume should be printed.  But if the incoming
sample is already float, and s->fast is 1, it will not print anything,
even though it will still do all the calculations.  This just makes
sure that s->fast is 0 iff float is used.

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

It moves the message outside the condition.  Actually, I've tried to
keep the indentation of af_msg the same, but diff still picked it up,
so even if I keep the indentation the same, it will still show up in
the diff.

> 
> > @@ -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...

This is audio, suppose the worst case, 6 channel 48000Hz, 576000 ifs
which 99.9% of the time evaluate false.  On a 1GHz CPU this adds maybe
1ms overhead per audio second.  But I agree, a separate filter may be
better.

> Next, I don't understand why you need to clamp before scaling?

If you don't, the multiply will overflow on 32-bit.

> And if that rounding really is worth one add more...

An add is almost costless, it's a pure register operation.  Let's say
this: if you can show measurable performance degradation coming from
this add, then we can remove it.

> Also I think the register before int vol should stay.

OK, I can add it back, but do you know of any compiler where this
really makes a difference?  If you really worry about speed, I can
write all this in MMX, but I do not think it worths the trouble, and
it will be harder to maintain.  The float softclip uses sin(x) for
every sample, and even that is hardly noticeable on my machine.

> 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?

What makes you think that?  I did not assume 32-bit, but I do assume,
that int is at least 32-bit, but 64-bit is fine too.

Zoli