10
\$\begingroup\$

I posted my audio recording program to Code Review before, and received a stern review along with the other reviews that caused me to rewrite the entire code base. Here is what I would like reviewed:

  1. Reusability/Portability - is this code easy to use for multiple separate projects with minimal modification?

  2. Optimization - although I'm not sure there is much room for improvement here, how can I make this code perform more efficiently? Obviously I can't record things any faster, but could I store the file or start the recording process faster than I am currently?

  3. Dynamics - are there PortAudio or libsndfile features that I am not taking advantage of that could make my program more powerful?

  4. Documentation - are my comments easy to understand? Are there ways that I could expound upon them?


audio.h:

/**
 * @file audio.h
 * @brief Contains AudioData structure, and relevent audio functions.
 */

#include <stdint.h>
#include <string.h>

/**
 * Holds all of the necessary information for building an
 * audio file.
 * @var duration Contains how long the audio file will be in seconds.
 * @var formatType Contains the audio formatting type.
 * @var numberOfChannels Contains the number of audio channels.
 * @var sampleRate Contains the sample rate in Hertz.
 * @var frameIndex Contains the current frame to be processed.
 * @var maxFrameIndex Contains the number of frames that the audio file will store.
 * @var recordedSamples Contains the raw PCM audio data.
 */

typedef struct
{
    uint32_t duration;
    uint16_t formatType;
    uint16_t numberOfChannels;
    uint32_t sampleRate;
    uint32_t frameIndex;
    uint32_t maxFrameIndex;
    float* recordedSamples;
} AudioData;

int recordFLAC(AudioData data, const char *fileName);
AudioData initAudioData(uint32_t sample_rate, uint16_t channels, uint32_t duration);

record.c:

/**
 * @file record.c
 * @brief Records a FLAC audio file
 */

#include <stdio.h>
#include <stdlib.h>
#include <portaudio.h>
#include <sndfile.h>
#include "audio.h"

/**
 * @fn initAudioData
 * @param sampleRate The sample rate in Hertz in which the audio is to be recorded
 * @param channels The number of channels in which the audio is to be recorded
 * @param duration How long in seconds the recording will be
 * @return a partially initialized structure.
 */

AudioData initAudioData(uint32_t sampleRate, uint16_t channels, uint32_t duration)
{
    AudioData data;
    data.duration = duration;
    data.formatType = 1;
    data.numberOfChannels = channels;
    data.sampleRate = sampleRate;
    data.frameIndex = 0;
    data.maxFrameIndex = sampleRate * duration;
    return data;
}

/**
 * This routine will be called by the PortAudio engine when audio is needed.
 * It may be called at interrupt level on some machines so don't do anything
 * that could mess up the system like calling malloc() or free().
 */
static int recordCallback(const void *inputBuffer, void *outputBuffer, unsigned long framesPerBuffer, const PaStreamCallbackTimeInfo* timeInfo, PaStreamCallbackFlags statusFlags, void *userData)
{
    AudioData* data = (AudioData*) userData;
    const float* rptr = (const float*)inputBuffer;
    float* wptr = &data->recordedSamples[data->frameIndex * data->numberOfChannels];
    long framesToCalc;
    int finished;
    unsigned long framesLeft = data->maxFrameIndex - data->frameIndex;

    (void) outputBuffer; /* Prevent unused variable warnings. */
    (void) timeInfo;
    (void) statusFlags;
    (void) userData;

    if(framesLeft < framesPerBuffer)
    {
        framesToCalc = framesLeft;
        finished = paComplete;
    }
    else
    {
        framesToCalc = framesPerBuffer;
        finished = paContinue;
    }

    if(!inputBuffer)
    {
        for(long i = 0; i < framesToCalc; i++)
        {
            *wptr++ = 0.0f;  // left
            if(data->numberOfChannels == 2) *wptr++ = 0.0f;  // right
        }
    }
    else
    {
        for(long i = 0; i < framesToCalc; i++)
        {
            *wptr++ = *rptr++;  // left
            if(data->numberOfChannels == 2) *wptr++ = *rptr++;  // right
        }
    }
    data->frameIndex += framesToCalc;
    return finished;
}

/**
 * @fn recordFLAC
 * @param data An initiallized (with initAudioData()) AudioData structure
 * @param fileName The name of the file in which the recording will be stored
 * @return Success value
 */

int recordFLAC(AudioData data, const char *fileName)
{
    PaStreamParameters inputParameters;
    PaStream* stream;
    int err = 0;
    int numSamples = data.maxFrameIndex * data.numberOfChannels;
    int numBytes = numSamples * sizeof(data.recordedSamples[0]);

    data.recordedSamples = calloc(numSamples, numBytes); // From now on, recordedSamples is initialised.
    if(!data.recordedSamples)
    {
        fprintf(stderr, "Could not allocate record array.\n");
        goto done;
    }
    if((err = Pa_Initialize())) goto done;

    inputParameters.device = Pa_GetDefaultInputDevice();
    if (inputParameters.device == paNoDevice)
    {
        fprintf(stderr,"Error: No default input device.\n");
        goto done;
    }
    inputParameters.channelCount = data.numberOfChannels;
    inputParameters.sampleFormat = paFloat32;
    inputParameters.suggestedLatency = Pa_GetDeviceInfo( inputParameters.device )->defaultLowInputLatency;
    inputParameters.hostApiSpecificStreamInfo = NULL;

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

    if((err = Pa_StartStream(stream))) goto done;
    puts("=== Now recording!! Please speak into the microphone. ===");

    while(1 == (err = Pa_IsStreamActive(stream)))
    {
        Pa_Sleep(1000);
        printf("index = %d\n", data.frameIndex);
    }
    if((err = Pa_CloseStream(stream))) goto done;

done:
    Pa_Terminate();
    if(err)
    {
        fprintf(stderr, "An error occured while using the portaudio stream: %d\n", err);
        fprintf(stderr, "Error message: %s\n", Pa_GetErrorText(err));
        err = 1;      // Always manually return positive codes.
    }
    else
    {
        SF_INFO sfinfo;
        sfinfo.channels = data.numberOfChannels;
        sfinfo.samplerate = data.sampleRate;
        sfinfo.format = SF_FORMAT_FLAC | SF_FORMAT_PCM_16;
        SNDFILE* outfile = sf_open(fileName, SFM_WRITE, &sfinfo); // open to file
        if (!outfile) return -1;
        long wr = sf_write_float(outfile, data.recordedSamples, numSamples); // write the entire buffer to the file
        if (wr < numSamples) fprintf(stderr, "Only wrote %lu frames, should have written %d", wr, numSamples);
        sf_write_sync(outfile); // force write to disk
        sf_close(outfile); // don't forget to close the file
    }
    free(data.recordedSamples);
    return err;
}
\$\endgroup\$
  • \$\begingroup\$ We are not aware of the duration we would like to record until silence. Please suggest ways to modify the code to record until silence of say a given duration. \$\endgroup\$ – SC-SL Nov 3 at 14:16
11
\$\begingroup\$

Few notes.

First, you don't want to copy data in the loop. Call memset()/memcpy() instead. You need some very serious reason to not use standard library. Here is some example code:

dataSize = sizeof(*wptr) * data->numberOfChannels * framesToCalc;
if (!inputBuffer)
    memset(wptr, dataSize, 0);
else
    memcpy(wptr, rptr, dataSize);

Second, recordFLAC() still needs some refactoring. As coded, it contains both some essential functionality (such as setup and teardown) along with the code more suitable for a demo purposes (printing error messages, writing data to a file). You want to separate them. This is all the function should be doing:

record(AudioData data, char * buffer)
{
    if (Pa_Initialize()) goto done;
    if ((inputParameters.device = Pa_GetDefaultInputDevice()) == paNoDevice)
        goto done;
    // Setup the rest of inputParameters
    if (Pa_OpenStream(...)) goto done;

    // This is another suspicious moment. I am not familiar with portaudio
    // to suggest an alternative for progress reporting, etc.
    while (Pa_IsStreamActive(stream))
        Pa_Sleep(1000);

    if (Pa_CloseStream()) goto done;
done:
    Pa_Terminate();
}

Otherwise, looks quite promising.

\$\endgroup\$
  • \$\begingroup\$ Could you flesh this out a bit? For example, what loop are you talking about? There is only one loop in my program (unless you are talking about the callback), and it is printing out data, not copying it. And could you maybe give some pseudo code for how you would refactor it? \$\endgroup\$ – syb0rg Apr 22 '14 at 3:13
  • \$\begingroup\$ You're right, I am talking about the callback. Give me few moments for the code. \$\endgroup\$ – vnp Apr 22 '14 at 3:19
  • \$\begingroup\$ Interesting. I'd be interested to talk to you about this a bit more in our chat room. \$\endgroup\$ – syb0rg Apr 22 '14 at 3:26
  • \$\begingroup\$ I am there. Is there anything special I shall do? \$\endgroup\$ – vnp Apr 22 '14 at 3:36

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.