What is wrong with my low pass filter?

Question

I have an array of int samples ranging from 32766 to -32767. In part of trying to create an envelope detector I've written a low pass filter, but it doesn't seem to be doing the job. Please keep in mind I'm trying to filter an entire array in one shot (no buffers).

This is not streamed, but applied to recorded audio for later playback. It is written in C. An example cutoff argument would be 0.5.

void lopass(int *input, float cutoff, int *output) 
{

float sample = 0;

for (int i=1 ; i < (1430529-10); i++) // we will go through all except the last 10 samples 
{
    for (int j = i; j < (i+10); j++) { // only do this for a WINDOW of a hundred samples

        float _in = (float)input[j];
        float _out = (float)output[j-1];

         sample = (cutoff * _in) + (32766 - (32766*cutoff)) * _out;

    }

    output[i] = (int)sample;
}

}

I thought that I would run my filtering statement on a window of 10 samples. Not only is it super slow, but it doesn't really do much but seemingly lower the overall amplitude. \

If you have any advice, or suggestions (or code!) on how to do this properly, that would be great!

Answer 1

A low-pass filter is basically some variant of averaging a number of values together. That means at least in the normal case your inner loop will accumulate a value. It's hard to guess the exact intent from your code, but you end up with something on the extremely general order of:

sample = 0;
for (int j=i; j<i+10; j++)
    sample += input[j];
output[i] = sample / 10;

As it stands right now, this just does averaging, with no cutoff specified -- that means it has a fixed (and fairly slow) cuttoff curve. The cutoff is governed only by the number of samples in the window.

To control the cutoff, you do not (at least normally) multiply all the input values by the same amount -- that would basically just modify the scale factor. Instead, you take a set of samples (10 of them, in your case) of the cutoff curve you want to apply, run them through an inverse FFT, and get a set of 10 coefficients. You then apply those coefficients in your loop:

sample = 0;
for (j=0; j<10; j++)
   sample += input[i+j] * coefficients[j];
output[i] = sample;

The number of samples in your window isn't normally an input to the design process -- rather, it's an output. You start by specify the cutoff frequency (as a fraction of the sampling frequency) and the cutoff width, and based on those you compute the necessary window size.

There are quite a few different techniques for computing your coefficients. Regardless of how you compute them, however, you normally end up with something on this general order -- accumulate the sum of the samples in the window, each multiplied by its respective coefficient.

The EE times had a pretty good article on filter design a few years ago.

Answer 2

It's a broken moving-window filter of 10 samples in the inner loop (where you actually use only the last sample of the 10), when in your comments you say you want 100 samples in your rectangular filter window.

The first error will give you a filter transition frequency 10X too high.

Answer 3

Don't know if it's relevant, but in your code the inner loop is doing nothing

for (j=???; j<???; j++) {
  sample = ???;
}

is the same as

// for (j=???; j<???; j++) {
  sample = ???; // for last j
// }

Answer 4

The arithmetic in the filter looks wrong, and as @pmg already pointed out, you are not storing output values correctly. It should probably be:

void lopass(int *input, float cutoff, int *output) 
{
    float sample = 0.0f;

    output[0] = 0.0f;

    for (int i = 1 ; i < (1430529 - 10); i++)
    {
        for (int j = i; j < (i + 10); j++)
        { 
            float _in = (float)input[j];
            float _out = (float)output[j-1];

            sample = (cutoff * _in) + (1.0f - cutoff) * _out;

            output[i] = (int)sample;
        }
    }
}

There are still a few minor issues to be fixed but this should at least work as a fairly crude single pole recursive (IIR) filter.