Onset detection using FFT

Question

I am using this onset source code and this fft file to try to perform onset detection. Currently the code is working, but as my DSP and C skills are subpar I could use some advice on how to improve it.

My main points of insecurity are the FFT (maybe there is a more suitable method out there?) and the C code. All advice appreciated!

NB: This code will eventually being going into iPhone project.

OnsetsDS *ods = malloc(sizeof *ods); 
float* odsdata = (float*) malloc(onsetsds_memneeded(ODS_ODF_RCOMPLEX, 512, 11));
onsetsds_init(ods, odsdata, ODS_FFT_FFTW3_HC, ODS_ODF_RCOMPLEX, 512, 11, 44100);
int i;
int x;
double (*vocalData)[2] = malloc(2 * 512 * sizeof(double));
double (*doubleFFTData)[2] = malloc(2 * 512 * sizeof(double));
for (i = 0; i < vocalBuffer.numFrames; i=i+512){
 bool onset; 
 for (x = 0; x < 512; x++){
 *vocalData[x] = (double)vocalBuffer.buffer[i+x]; 
 }
 fft(512, vocalData, doubleFFTData);
 float *floatFFTData = malloc(512 * sizeof(float));
 int z;
 for (z = 0; z < 512; z++){
 floatFFTData[z] = (float)*doubleFFTData[z];
 }
 onset = onsetsds_process(ods, floatFFTData);
 if (onset){
 printf("onset --> %i\n", i);
 }
}
free(ods->data); // Or free(odsdata), they point to the same thing in this case
return nil; 

Answer 1

The link in the post to your fft file is broken, so I can't see the function prototype. This is a pity, because it is otherwise difficult to know what to make of your arrays:

double (*vocalData)[2] = malloc(2 * 512 * sizeof(double));

This says vocalData is a pointer to an array of two doubles. You have allocated 1024 doubles at this address. So on a 64bit system you have:

 8 bytes
vocalData[0] -> |--------|
 |--------|
vocalData[1] -> |--------|
 |--------|
vocalData[2] -> |--------|
 |--------|
 etc 

You fill the array with

for (x = 0; x < 512; x++){
 *vocalData[x] = (double)vocalBuffer.buffer[i+x];
}

This fills alternate doubles in your allocated space and leaves the others uninitialized.

 8 bytes
vocalData[0] -> |xxxxxxxx|
 |--------|
vocalData[1] -> |xxxxxxxx|
 |--------|
vocalData[2] -> |xxxxxxxx|
 |--------|
 etc 

I don't know for sure that this is not what you want, but...

Also all that converting from float to double and then back to float is very inefficient. As onsetsds_process takes floats, I can see the need, but maybe you should use an FFT function that also deals in floats.

Finally it would be better to #define FFT_SIZE 512 at the top and use FFT_SIZE throughout instead of the explicit 512. This is better practice, for example making it much easier to change the FFT length if it is necessary.

Oh and don't forget to free any memory you have allocated once you have finished with it. In this case, that means all of your allocations should be freed on exit (although if your return 0 is the end of the program that is academic).

• Simon Forsberg: Answers are normally a bit longer than a simple comment. This could have been posted as a comment to the question instead of an answer. If you could expand the answer a bit, then it is more worthy of being an answer.
• Malachi: Assume that the OP doesn't know what you are talking about, please explain to him how to do this, maybe showing some example code. I totally agree with you but what you have is not a review, it's a one liner, and will not get any up votes.
• Simon Forsberg: Well done, this answer has been significantly improved!