FIFO queue for audio data

Question

I have created a data structure to handle some audio data I am generating (simple sine wave right now). It had some errors in it due to some misunderstanding of my ancient C knowledge that has been fixed by my cousin. I am wondering if there may yet still be more issues.

I have considered turning this into a C++ class, but because I don't have enough experience with C++ I would likely turn it into a huge memory leak waiting to happen. This is really being used with Objective C on the iPad - but nothing prevents this from being used elsewhere.

All of the functions are publicly accessible, and should be fairly obvious how they should be used. But essentially you alloc the structure, push and pop (which make it sound like a stack but it very much is not), and finally release the structure. You will notice it is in a C Object Oriented Design - I did that intentionally to make things easier to remember.

I did not use a circular buffer because I hadn't reached the ballmer peak yet when I was writing this and I couldn't quite get the right form. The queue basically works on the principle that you append on the end, and take data off the top - and the stuff left is moved up to the top.

I was looking to use some vector acceleration functions from this at some point. It presently is being used in threaded code behind locks with NSCondition.

To see how this is being used now, the structure is used in this .m file and is part of this project.

// from the header file - just converted to a typedef, untested...
typedef struct {
    float *buffer;
    float *end;
    size_t max_length;
} FIFO;

#include "FIFO.h"

FIFO *FIFO_alloc(size_t length) {
    FIFO *restrict ptr = NULL;

    ptr = (FIFO*)calloc(1, sizeof(FIFO));
    if (ptr == NULL) {
        return NULL;
    }

    ptr->buffer = (float*)calloc(length, sizeof(float));
    if (ptr->buffer == NULL) {
        free(ptr);
        return NULL;
    }

    ptr->end = ptr->buffer;
    ptr->max_length = length;

    return ptr;
}

void FIFO_release(FIFO *fifo) {
    if (fifo) {
        free(fifo->buffer);
        free(fifo);
    }
}

bool FIFO_push(FIFO *restrict fifo, const float *restrict data, size_t count) {
    if (!fifo || !data || count == 0) return false;

    // test for the length available
    if (fifo->max_length - (int)(fifo->end - fifo->buffer) < count) {
        return false;
    }

    memcpy(fifo->end, data, count*sizeof(float));
    fifo->end += count;

    return true;
}

bool FIFO_pop(FIFO *restrict fifo, float *restrict data, size_t count) {
    if (!fifo || !data) return false;
    if (count == 0 || FIFO_size(fifo) < count) return false;

    // move count items from the buffer
    memcpy(data, fifo->buffer, count*sizeof(float));
    // move empty space back to the beginning
    memmove(fifo->buffer, fifo->buffer + count, (fifo->end - fifo->buffer - count) * sizeof(float));

    // reposition end pointer
    fifo->end -= count;

    return true;
}

size_t FIFO_size(const FIFO *fifo) {
    if (fifo) {
        return (size_t)(fifo->end - fifo->buffer);
    }

    return 0;
}

size_t FIFO_maxsize(const FIFO *fifo) {
    if (fifo) {
        return fifo->max_length;
    }

    return 0;
}

Answer 1

Here's a few suggestions in addition to what Keith mentions.

I would try to avoid the memory copy and move operations. You mention a circular buffer, which is one way of doing it, another would be to have the fifo hold discrete buffers and pop each buffer from the front in it's entirety. Avoiding the copy and move operations should speed up the code significantly, which I assume is of concern when processing audio data :)

Also, I would drop the typedef, just use struct fifo instead. This makes it easier to convert this into a full ADT (Abstract Data Type) later, and just forward declare the struct to users while keeping the actual struct private.

As a final note, are you absolutely sure that the restrict keyword is the proper thing to use here?

Edit: Here's one naive way of getting rid of the memove operation:

typedef struct {
    float *buffer;
    float *start;
    float *end;
    size_t max_length;
} FIFO;


bool FIFO_pop(FIFO *restrict fifo, float *restrict data, size_t count) {
    if (!fifo || !data) return false;
    if (count == 0 || FIFO_available(fifo) < count) return false;

    // move count items from the buffer
    memcpy(data, fifo->start, count*sizeof(float));
    fifo->start += count;

    return true;
}

This will only work if you know the max sized buffer that you need, but it should give you an idea.

Answer 2

Looks pretty good....

there's a few style things I'd change

if (fifo->max_length - (int)(fifo->end - fifo->buffer) < count) {

I'd bracket things here for clarity.

potentially I'd also extract fifo->end - fifo->buffer into a separate function as you use it multiple times