Recording Audio Continuously in C

Question

As an ongoing little side project of mine, I've been working on recording audio in c. You can the code's progression by looking at past versions (V1.0, V2.0).

I've found that my past versions were too limited. They required a certain time frame for which they would record, and then stop. This isn't very practical for many real-world applications, so I rewrote everything so the computer would always be listening, and only record necessary data.

Here is what I would like reviewed:

• Memory consumption: Obviously a huge potential problem is lots of space being consumed in recording and saving the audio to file. Am I wasting space at all? Every little bit counts. Note: file storage container must be FLAC.

• Speed: I have to be processing the data in real time. I can't hang for anything or I might lose precious audio data. Is there anywhere I could speed up processing?

• Syntax/Styling: How does my code look? What about it could make it look better? Anything I'm doing wrong syntax-wise?

Feel free to review other stuff as well, I would just like reviews to be focused on the above. Keep in mind I've been away from the C language for a bit now, so I may have forgotten some of the more simple things ;)

---

main.c:

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <string.h>
#include <time.h>
#include <portaudio.h>
#include <sndfile.h>

#define FRAMES_PER_BUFFER 1024

typedef struct
{
    uint16_t formatType;
    uint8_t numberOfChannels;
    uint32_t sampleRate;
    size_t size;
    float *recordedSamples;
} AudioData;

typedef struct
{
    float *snippet;
    size_t size;
} AudioSnippet;

AudioData initAudioData(uint32_t sampleRate, uint16_t channels, int type)
{
    AudioData data;
    data.formatType = type;
    data.numberOfChannels = channels;
    data.sampleRate = sampleRate;
    data.size = 0;
    data.recordedSamples = NULL;
    return data;
}

float avg(float *data, size_t length)
{
    float sum = 0;
    for (size_t i = 0; i < length; i++)
    {
        sum += fabs(*(data + i));
    }
    return (float) sum / length;
}

long storeFLAC(AudioData *data, const char *fileName)
{

    uint8_t err = SF_ERR_NO_ERROR;
    SF_INFO sfinfo =
    {
        .channels = data->numberOfChannels,
        .samplerate = data->sampleRate,
        .format = SF_FORMAT_FLAC | SF_FORMAT_PCM_16
    };

    SNDFILE *outfile = sf_open(fileName, SFM_WRITE, &sfinfo);
    if (!outfile) return -1;

    // Write the entire buffer to the file
    long wr = sf_writef_float(outfile, data->recordedSamples, data->size / sizeof(float));
    err = data->size - wr;

    // Force write to disk and close file
    sf_write_sync(outfile);
    sf_close(outfile);
    puts("Wrote to file!!!!");
    return err;
}

int main(void)
{
    PaError err = paNoError;
    if((err = Pa_Initialize())) goto done;
    const PaDeviceInfo *info = Pa_GetDeviceInfo(Pa_GetDefaultInputDevice());
    AudioData data = initAudioData(44100, info->maxInputChannels, paFloat32);
    AudioSnippet sampleBlock =
    {
        .snippet = NULL,
        .size = FRAMES_PER_BUFFER * sizeof(float) * data.numberOfChannels
    };
    PaStream *stream = NULL;
    sampleBlock.snippet = malloc(sampleBlock.size);
    time_t talking = 0;
    time_t silence = 0;
    PaStreamParameters inputParameters =
    {
        .device = Pa_GetDefaultInputDevice(),
        .channelCount = data.numberOfChannels,
        .sampleFormat = data.formatType,
        .suggestedLatency = info->defaultHighInputLatency,
        .hostApiSpecificStreamInfo = NULL
    };

    if((err = Pa_OpenStream(&stream, &inputParameters, NULL, data.sampleRate, FRAMES_PER_BUFFER, paClipOff, NULL, NULL))) goto done;
    if((err = Pa_StartStream(stream))) goto done;
    for(int i = 0;;)
    {
        err = Pa_ReadStream(stream, sampleBlock.snippet, FRAMES_PER_BUFFER);
        if (err) goto done;
        else if(avg(sampleBlock.snippet, FRAMES_PER_BUFFER) > 0.000550) // talking
        {
            printf("You're talking! %d\n", i);
            i++;
            time(&talking);
            data.recordedSamples = realloc(data.recordedSamples, sampleBlock.size * i);
            data.size = sampleBlock.size * i;
            if (data.recordedSamples) memcpy((char*)data.recordedSamples + ((i - 1) * sampleBlock.size), sampleBlock.snippet, sampleBlock.size);
            else
            {
                free(data.recordedSamples);
                data.recordedSamples = NULL;
                data.size = 0;
            }
        }
        else //silence
        {
            double test = difftime(time(&silence), talking);
            if (test >= 1.5 && test <= 10)
            {
                char buffer[100];
                snprintf(buffer, 100, "file:%d.flac", i);
                storeFLAC(&data, buffer);
                talking = 0;
                free(data.recordedSamples);
                data.recordedSamples = NULL;
                data.size = 0;
            }
        }
    }

done:
    free(sampleBlock.snippet);
    Pa_Terminate();
    return err;
}

Compile with: gcc main.c -lsndfile -lportaudio (assuming you have those libraries installed)

Answer 1

Indeed you must have been working on this project for a while, since I answered a question on SO related to one of your old versions a while back. At least some of the feedback I had is still pertinent even now.

Code and style comments

1. #include <stdio.h>
   #include <stdlib.h>
   #include <math.h>
   #include <string.h>
   #include <time.h>
   #include <portaudio.h>
   #include <sndfile.h>
   

   The problem starts already here. You make heavy use of uint32_t and other such types, without #include <stdint.h>. I assume this works because you're probably still on Mac OS X, and this would also explain why your GCC doesn't complain about C99 loop initial declarations without -std=gnu99.

2. As mentioned previously, you make unnecessary use of pointer arithmetic, which makes the code hard to read. I definitely prefer @nhgrif's version of your average; I find sum += fabs(data[i]); eminently easier on the eye.

3. The avg() function has at least a few other issues. First, if you want to compute when someone is talking, I wouldn't do a straight average, even with a gimmick like fabs(). I'd do an RMS (Root-Mean-Square) instead and then subtract the regular average (without fabs()) from the RMS value; this computes the audio "power" in the buffer while ignoring any DC bias (non-zero average), which your code isn't robust against. This is directly connected to the number of decibels in your sample by the same power law used to "compand" the input from the microphone (It could be A-law or u-law, or maybe something else).

   Second, and related; At 1024 frames and a sample rate of 44100Hz, your buffers cover a period of just 23 milliseconds; In other words, only one complete cycle of a sound with frequency 43 Hz. I don't know how low @rob0t's voice goes, but you may want to increase that amount if you will be dealing with low-frequency content. When computing the RMS and DC (average), you can only get an accurate result if you capture precisely an integer number of cycles for the frequencies of interest to you; But if your buffer spans enough cycles of all frequencies of interest to you, any inaccuracy introduced by partial cycles is drowned out by the complete ones in the average.

   Thirdly, your buffer has 1024 frames, but the accumulator is float. I suspect you don't care about precision much here, but you may be interested in knowing that because IEEE floating-point arithmetic isn't exactly like real math, the order in which floating-point values are added may make a small difference. In particular, if you were to add up 1024 identical values, you'll loose about 10 out of 24 bits of precision on your last accumulations. This is because the accumulator will have grown to 2^10 times the size of the accumulands, and additions into the accumulator will only use the 14 MSBs while rounding off the 10 LSBs. I'd use double here unless it were proven to me that avg() is a performance bottleneck and that vectorization has already done all it can.

   Fourthly and lastly,

   return (float) sum / length;

   is entirely redundant: sum is already a float, and you're casting it to float again. If you wanted to carry out this division using floating-point math, this is already guaranteed; In C, floating-point values outrank any integer in the implicit conversions. Remember, casts don't apply to the operation (/), but to their operand (here, sum).

4. storeFLAC() has no logging in case of errors, so I have no immediate explanations as to why your code, built on Linux, succeeds only in creating zero-length files. The error code it returns is completely ignored.

   SNDFILE *outfile = sf_open(fileName, SFM_WRITE, &sfinfo);
   if (!outfile) return -1;
   
   // Write the entire buffer to the file
   

   And due to lack of braces around early return statements, it's harder for reviewers to add their own debugging code, because it's that much more of a chore to add braces on top of a debug printf(). It's nice to be considerate to your reviewers, including your future self.

5. main() is in sore need of refactoring, specifically extraction of a few functions for each major step. The code you wrote does not "speak" to me; I must invest some effort to understand what it does. At least it is approximately a screenful, which is in my opinion a good size for a function.

6. This is gross misuse of goto:

   int main(void)
   {
     PaError err = paNoError;
     if((err = Pa_Initialize())) goto done;
     const PaDeviceInfo *info = Pa_GetDeviceInfo(Pa_GetDefaultInputDevice());
     AudioData data = initAudioData(44100, info->maxInputChannels, paFloat32);
     AudioSnippet sampleBlock =
     {
         .snippet = NULL,
         .size = FRAMES_PER_BUFFER * sizeof(float) * data.numberOfChannels
     };
     ...
   
     done:
     free(sampleBlock.snippet);
     Pa_Terminate();
     return err;
   }
   

   While I do approve of goto for error handling, you've here committed the sin of jumping over the initialization of local variables, then bothering to use them. Nothing guarantees that sampleBlock.snippet is initialized when you attempt to free() it: You haven't even done sampleBlock's initialization if PortAudio failed to initialize!

   This is an error you would have discovered had you used GCC's and Clang's -Wall -Wextra flags to ask for many, many useful diagnostics. This error was reported as

   portaudio.c: In function ‘main’:
   portaudio.c:135:6: warning: ‘sampleBlock.snippet’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     free(sampleBlock.snippet);
         ^
   

   . Note however that this warning only appears when optimizations are enabled; This is because the analysis passes GCC requires to discover such uninitialized uses are only run at -O1 and up. It may thus be worthwhile for you to build regularly at high optimization levels, where GCC puts more effort into analysis and can as a side-effect discover potential bugs.

7. This line:

   if((err = Pa_OpenStream(&stream, &inputParameters, NULL, data.sampleRate, FRAMES_PER_BUFFER, paClipOff, NULL, NULL))) goto done;
   

   It's pretty obvious that if you need to scroll that far right, you have greatly exceeded my 80-column soft limit. You've also not put braces around your statement, and here a nice debug printf would have been helpful. I certainly want to know if my stream failed to open.

   For functions with a large number of arguments with long names, I tend to put them down one per line. Don't be afraid to spend several lines on them.

8. Congratulations on your use of difftime() to portably and safely determine time differences. I've learned something. However, time_t is commonly defined as an integer number of seconds since the Epoch. Since you will be calling time() around 43 times per second, many consecutive calls will give the same time, and thus a difference of 0 s, while some calls will give a difference of 1 s, despite only really being separated by 23 ms.

   Better here would have been to use a higher-resolution wall-clock timer. For this type of timing I advise gettimeofday(); For extreme resolutions I advise clock_gettime() on Linux, or the much easier-to-use and lower-overhead mach_absolute_time() on Mac OS X.

9. This is madness:

   data.recordedSamples = realloc(data.recordedSamples, sampleBlock.size * i);
   data.size = sampleBlock.size * i;
   if (data.recordedSamples) memcpy((char*)data.recordedSamples + ((i - 1) * sampleBlock.size), sampleBlock.snippet, sampleBlock.size);
   else
   {
     free(data.recordedSamples);
     data.recordedSamples = NULL;
     data.size = 0;
   }
   

   Firstly, one of your lines here is excessively long again. You've here used the infamous x = realloc(x, ...) idiom, which is awful since if realloc() fails, you will leak memory.

   Moreover, you're memcpy()'ing a new block of data into your homebrew resizable vector, on top of any memcpy()'s inside realloc(). Leaving aside the fact that every 23 ms of speech you'll potentially realloc() and copy the entire array of data gathered so far, I seriously query the need for even moving any data at all. It should be possible for you to craft yourself either a double buffer or, more generally, a circular buffer, and avoid any copying at all.

   And then on top of that, look at your conditional statement again. If data.recordedSamples is indeed NULL (and thus, you've already leaked memory by loosing your last reference to that block of memory), why are you free()'ing said NULL pointer, then setting said NULL pointer to NULL again? It is completely ineffectual.

Design comments

10. There is a general lack of documentation in your code. It would be helpful to know the finer points of certain things. For instance, I can quickly take it that you only record in float format, since your undocumented structures use only float*; But what precisely is a sample, snippet or frame, according to either you or PortAudio? What's the interleaving scheme for channels, if any? What is the fundamental difference between an AudioSnippet and AudioData that compelled you to make a different structure for both of them?

11. Given that you put emphasis on not dropping any audio data at all, I advise that you make this application threaded. One thread would drive the reading of data into a circular buffer, and would perform tasks with negligible computational complexity (presumably, an RMS will take sufficiently little time as to never be the bottleneck). If this thread detects speech, it can message a worker thread to start reading this buffer at the given offset and encode it into a FLAC file. Of course, if the worker thread is too slow, eventually the reading thread will exhaust the circular buffer and will have to block, possibly dropping frames.

Answer 2

If it walks like an array and talks like an array, use it like an array.

float avg(float *data, size_t length)
{
    float sum = 0;
    for (size_t i = 0; i < length; i++)
    {
        sum += fabs(*(data + i));
    }
    return (float) sum / length;
}

So, data is an array, right? Why don't we treat it like one and access it using subscripts?

float avg(float *data, size_t length)
{
    float sum = 0;
    for (size_t i = 0; i < length; ++i)
    {
        sum += fabs(data[i]);
    }
    return (float) sum / length;
}

And I don't know C well enough to say for sure, but that cast in the return line seems unnecessary (sum already is a float).

---

Don't cheat your braceless 1-line if statements.

if (data.recordedSamples) memcpy((char*)data.recordedSamples + ((i - 1) * sampleBlock.size), sampleBlock.snippet, sampleBlock.size);

While I'm okay-ish with very simple one-line statements being on the same line and braceless, this one is quite ridiculuous. This is a one line because you've cheated and inlined way too much stuff.

if (data.recordedSamples)
{
    size_t skipIndex = (i - 1) * sampleBlock.size;
    char *destination = (char*)data.recordedSamples + skipIndex;
    memcpy(destination, sampleBlock.snippet, sampleBlock.size);
}

This is better much better (though admittedly, the variable names I picked may not make sense).

---

It's called main(), not everything()

We've overstuff main(). And that, ultimately, has lead us into the rare scenario where a goto is actually a pretty clean way of organizing the code within the function.

I'll say that again: this is actually a relatively clean way of using a goto statement. The immediately alternative is deeply nested if statements which really isn't clean at all.

But there's a better approach that makes the code cleaner and has the side-effect bonus of eliminating the goto all together: don't cram so much into main()!

Make several more functions. Call out to your functions. And let your functions have early returns.

Functions help the reader comparmentalize chunks of your code. You did a good job with avg function. "AVG" is a common abbreviation for average. I can guess by the function name alone what it does, and I don't have to read through the 6 simple lines of logic to figure out what it's doing. All I have to do is just see you're calling avg() and I know what's going on.

So find some more ways to break up and compartmentalize your code into more digestable chunks that the read can just see the function name and get the gist of what's going on here without needing to see every single line of executable code.

Answer 3

Design review

Continuous reallocation of data.recordedSamples doesn't sound right. It may eventually fail because you require not just a lot of memory, but a lot of contiguous memory. Consider at least reusing data.recordedSamples instead of freeing it. That would require just one counter (how many blocks it may currently accomodate) and reduce amount of reallocations dramatically.

It also seems that you want to keep recorded samples in a contiguous space to be able to fwrite them in a single call. I don't think reducing the number of write calls buys any performance. Most of the time is spent in data transfer, and it doesn't depend on number of calls. In fact, large writes make the execution profile less smooth: instead of constantly spending little time, your program sometimes spends much time. A perfect opportunity to miss an important audio event.

A standard approach to such problems is double buffering (I presume that writing the buffer is faster than recording it), along the lines of:

    while (keep_going) {
        read_buffer(buf1);
        swap(buf1, buf2);
        write_nonblock(buf2);
    }

Code review

• 0.000550 is a strange looking magic number. const float talking_threshold perhaps? In any case, the value needs some rationale.

• You record some lead-out silence. I also recommend to record some lead-in. Otherwise, you may miss the few "talking" samples in the very beginning of the event.

• avg is an excellent candidate for speedup. Consider hardware acceleration.

• goto is absolutely unwarranted.

Answer 4

Goto conditions are often maligned as being bad practice... and, for a reason. In almost every situation where a goto is used, it's because you are doing too much in a method. Consider your main method:

int main(void)
{
    PaError err = paNoError;
    if((err = Pa_Initialize())) goto done;

    .....

    AudioData data = initAudioData(44100, info->maxInputChannels, paFloat32);
    AudioSnippet sampleBlock =
    {
        .snippet = NULL,
        .size = FRAMES_PER_BUFFER * sizeof(float) * data.numberOfChannels
    };
    PaStream *stream = NULL;
    sampleBlock.snippet = malloc(sampleBlock.size);


    .....

    if((err = Pa_OpenStream(&stream, &inputParameters, NULL, data.sampleRate, FRAMES_PER_BUFFER, paClipOff, NULL, NULL))) goto done;
    if((err = Pa_StartStream(stream))) goto done;
    for(int i = 0;;)
    {
        err = Pa_ReadStream(stream, sampleBlock.snippet, FRAMES_PER_BUFFER);
        if (err) goto done;

        ........

    }

done:
    free(sampleBlock.snippet);
    Pa_Terminate();
    return err;
}

That whole thing could be done simply, as:

int main(void)
{
    time_t talking = 0;
    time_t silence = 0;

    PaStream *stream = NULL;
    PaError err = setup_stream(stream);
    while (!err) {
        err = process_stream(stream);
    }
    free_stream(stream);
    return err;
}

Then, your various method can simply return err; in them if there is a problem:

int setup_stream(PaStream stream) {
    PaError err = paNoError;
    err = Pa_Initialize();
    if (err) {
        return err;
    }
    const PaDeviceInfo *info = Pa_GetDeviceInfo(Pa_GetDefaultInputDevice());
    AudioData data = initAudioData(44100, info->maxInputChannels, paFloat32);
    AudioSnippet sampleBlock =
    {
        .snippet = NULL,
        .size = FRAMES_PER_BUFFER * sizeof(float) * data.numberOfChannels
    };

    sampleBlock.snippet = malloc(sampleBlock.size);
    PaStreamParameters inputParameters =
    {
        .device = Pa_GetDefaultInputDevice(),
        .channelCount = data.numberOfChannels,
        .sampleFormat = data.formatType,
        .suggestedLatency = info->defaultHighInputLatency,
        .hostApiSpecificStreamInfo = NULL
    };

    err = Pa_OpenStream(&stream, &inputParameters, NULL, data.sampleRate, FRAMES_PER_BUFFER, paClipOff, NULL, NULL);
    if (err) {
        return err;
    }
    return Pa_StartStream(stream);

}

Even that method seems too long. The process of setting up the same data is problematic.

Also, I probably messed up on the input stream parameter... it needs to be a pointer type that points to the newly-created stream on successful completion...

Answer 5

First, you aren't using braces around one-line if statements:

if (!outfile) return -1;

Failing to use braces was the cause of Apple's SSL bug, as described here.

---

Why are you doing this?

#define FRAMES_PER_BUFFER 1024

Shouldn't that be static const int FRAMES_PER_BUFFER = 1024;?

This SO question discusses this.

---

As @RubberDuck mentioned in the comment, you have an awful lot of logic in main(). This should probably be delegated to a separate method, or multiple methods. main() shouldn't contain a bunch of logic, but should be used more like an entrance point to the program.

---

Other than these points, this is some of the cleanest, neatest code I've seen. You used spaces around your operators and your code just generally looks neat.