[MPlayer-cvslog] CVS: main/libaf af_ladspa.c, NONE, 1.1 .cvsignore, 1.1, 1.2 Makefile, 1.18, 1.19 af.c, 1.34, 1.35

[Ivo]

Hi,

On Thursday 23 December 2004 10:03, Reimar Dffinger wrote:
> >     if ( (setup->bufsize != nsamples/nch) || (setup->nch != nch) ) {
>
> It actually might be a good idea to only check if  nsamples/nch >
> setup->bufsize. Same for nch (you might even go with MAX_NCH always). But
> it would require changing some loops, so better after the release...

That's actually quite a good idea. Will save lots of free()'s and 
malloc()'s. I have seen MPlayer switch buffersize from 1024 to 2048 bytes 
and back every few seconds sometimes (depending on the load).

> >     for (p=0; p<setup->bufsize; p++) {
> >         for (i=0; i<nch; i++) {
> >             setup->inbufs[i][p] = ( (float) audio[p*nch + i] ) /
> > 32768.0f;
>
> I just don't think that is what you want. It will give you values in
> between -1 and (1 - 1/32768), thus scaling positive values a bit smaller
> than negative ones. This will produce inaccuracies with almost any filter
> that does anything useful.. I really think you should use unsigned as
> input format and multiply (faster than division) by (2.0f / 65535.0f) and
> then subtract 1.0f. And for converting back add 1.0f and multiply by
> (65535.0f / 2.0f). You can of course write the numbers out, if you don't
> want to rely on the compiler optimizing it...

I agree. I don't know what I was thinking there, although it is better than 
what was previously in the proposed patch. The inaccuracy is only 0.003%, 
so I don't think it'll be a big problem in -pre6. Nevertheless, it's wrong 
so I'll fix this.

> >     for (p=0; p<setup->bufsize; p++) {
> >         for (i=0; i<nch; i++) {
> >             v = [...]
>
> I also forgot to say that as v is only used here it might be a good idea
> to declare it here and make it a register variable.

I was looking into a way of removing the use of a temp variable all 
together, but because of the hard-clipping needed, that'll only be slower, 
so I will make it a register variable as you suggest.

I will make these changes after christmas and (hopefully :) ) before the new 
year.

--Ivo