3
\$\begingroup\$

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;
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The memory copy/move operations are used to copy the data into the awaiting buffer given by the audio system callback. I am not sure there is a way to avoid that - I was under the impression that memory copy was fairly fast. The audio system comes in with a request for so many samples (n floats) and it might vary. \$\endgroup\$ – Jeffrey Drake Jul 4 '11 at 10:40
  • \$\begingroup\$ You may not be able to avoid the copies to the destination buffer, that depends on the implementation of the media library you're using. The moves, however should be fairly trivial to remove. \$\endgroup\$ – harald Jul 4 '11 at 10:58
  • \$\begingroup\$ I am not sure how one would go about removing the moves. The moves are to move it back to the head of the buffer, and it is done with a move because it is potentially overlapping. \$\endgroup\$ – Jeffrey Drake Jul 4 '11 at 21:06
  • \$\begingroup\$ not sure what your point is on the typedef? you can forward declare a typedef and struct? \$\endgroup\$ – Keith Nicholas Jul 5 '11 at 2:52
  • \$\begingroup\$ @iaefai see edited answer for a suggestion. @Keith Nicholas forward declaring anonymous typedefed structs cause all kinds of problems with type redefinition errors etc. They can be overcome, but by dropping the typedef altogether they just disappear. \$\endgroup\$ – harald Jul 5 '11 at 8:24
0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ actually, you already have extracted it.....ok, I'd then use it in the code :) \$\endgroup\$ – Keith Nicholas Jul 4 '11 at 0:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.