Possible bug in down sample function in reverb?

[SwarmSysUser]

The function oversample_stepdown appears to put the samples through the recovery low pass filter but always returns the first input value. I think it should look more like this:

// input length must be oversample->factor
static float oversample_stepdown(sf_rv_oversample_st *oversample, float *input)
{
    if (oversample->factor == 1)
    {
        return input[0];
    }
    else
    {
        int i;
        float   accum = 0.0f;
        for (i = 0; i < oversample->factor; i++)
        {
            accum += biquad_step(&oversample->lpfD, input[i]);
        }
        accum /= (float)oversample->factor;
        return accum;
    }
}

[velipso]

I haven't looked at this in a while, but that does look wrong.

Here's the code in Freeverb3 that I was attempting to clean up (I think):

http://git.savannah.gnu.org/cgit/freeverb3.git/tree/freeverb/src.cpp#n159

void FV3_(src)::src_d_iir2(fv3_float_t *input, fv3_float_t *output, long factor, long numsamples, FV3_(biquad) * iir)
{
  for(long i = 0;i < numsamples*factor;i ++) input[i] = iir->process(input[i]);
  for(long i = 0;i < numsamples;i ++) output[i] = input[factor*i];
}

They're processing in blocks, whereas I'm processing one sample at a time. So you can think of it as hard-coding numsamples to 1.

Original code in sndfilter:

// input length must be oversample->factor
static inline float oversample_stepdown(sf_rv_oversample_st *oversample, float *input){
  if (oversample->factor == 1)
    return input[0];
  for (int i = 0; i < oversample->factor; i++)
    biquad_step(&oversample->lpfD, input[i]);
  return input[0];
}

I think what I'm missing is saving the return value of biquad_step:

  for (int i = 0; i < oversample->factor; i++)
    input[i] = biquad_step(&oversample->lpfD, input[i]);
  return input[0];

Now, I'd rather not stomp the input array, so instead I should do something like:

// input length must be oversample->factor
static inline float oversample_stepdown(sf_rv_oversample_st *oversample, float *input){
  if (oversample->factor == 1)
    return input[0];
  float out = biquad_step(&oversample->lpfD, input[0]);
  for (int i = 1; i < oversample->factor; i++)
    biquad_step(&oversample->lpfD, input[i]);
  return out;
}

That seems like it matches the spirit of Freeverb3. What do you think?

[SwarmSysUser]

Well that certainly looks like what the code is doing.

[SwarmSysUser]

Also the code does not flush denormal numbers to zero. Do you configure the FCU to do this?

[velipso]

no, I debated whether to do optimizations like that, and decided against it to aid in readability... I think there are a lot of optimizations that could be done on the code (ex: iirc, Apple provides hardware instructions for doing biquad math on a buffer, which Chromium uses but I don't), but wanted to try and remain as simple as possible, so it was more accessible

I'm about to push up the fix

[velipso]

I just added a note to the readme to make it clear I favored simple over fast.

Thanks for bringing up these issues!