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

[Reimar Döffinger]

On Sun, Feb 20, 2005 at 01:55:16PM -0600, Zoltan Hidvegi wrote:
> Reimar Döffinger wrote:
> > > -	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.

Maybe the -d options of diff helps...

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

1 GHz!! MPlayer is supposed to run on much slower CPUs! Also, an if that
evaluates to false can be very costly, too, especially on architectures
that do not do complicated branch prediction (does MPlayer run on old
MIPS *g* ?)

> > Next, I don't understand why you need to clamp before scaling?
> 
> If you don't, the multiply will overflow on 32-bit.

As I understand the code, vol will be 65536 at most. As you assume that
int is at least 32 bits and the input is 16 bits I don't think it can overflow??

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

Not on the machines I own. But I tend to say: show that this actually
makes a relevant (which means here something you can hear) difference
to the output and we can leave it in.

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

Well, if you only think about machines that have MMX, yes, it probably
doesn't make a difference here. MPlayer is the fastest player and I want
it to stay like that - and for that you really will have to provide a
good justification for making something slower - and not the other way
round.

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

Exactly what I meant... So the question would be: Is there any
architecture where it is 16 bit only?

Greetings,
Reimar Döffinger