Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a program I wrote as a .wav audio recording library for Linux. It was developed on a Raspberry Pi, so that may affect the dependencies required.(1)


Here is the header file (wav.h):

#include <stdint.h>

typedef struct
{
    char RIFF_marker[4];
    uint32_t file_size;
    char filetype_header[4];
    char format_marker[4];
    uint32_t data_header_length;
    uint16_t format_type;
    uint16_t number_of_channels;
    uint32_t sample_rate;
    uint32_t bytes_per_second;
    uint16_t bytes_per_frame;
    uint16_t bits_per_sample;
} WaveHeader;

WaveHeader *genericWAVHeader(uint32_t sample_rate, uint16_t bit_depth, uint16_t channels);
WaveHeader *retrieveWAVHeader(const void *ptr);
int writeWAVHeader(int fd, WaveHeader *hdr);
int recordWAV(const char *fileName, WaveHeader *hdr, uint32_t duration);

Here is the main program:

#include <alsa/asoundlib.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include "wav.h"

WaveHeader *genericWAVHeader(uint32_t sample_rate, uint16_t bit_depth, uint16_t channels)
{
    WaveHeader *hdr;
    hdr = malloc(sizeof(*hdr));
    if (!hdr) return NULL;

    memcpy(&hdr->RIFF_marker, "RIFF", 4);
    memcpy(&hdr->filetype_header, "WAVE", 4);
    memcpy(&hdr->format_marker, "fmt ", 4);
    hdr->data_header_length = 16;
    hdr->format_type = 1;
    hdr->number_of_channels = channels;
    hdr->sample_rate = sample_rate;
    hdr->bytes_per_second = sample_rate * channels * bit_depth / 8;
    hdr->bytes_per_frame = channels * bit_depth / 8;
    hdr->bits_per_sample = bit_depth;

    return hdr;
}

int writeWAVHeader(int fd, WaveHeader *hdr)
{
    if (!hdr)
        return -1;

    write(fd, &hdr->RIFF_marker, 4);
    write(fd, &hdr->file_size, 4);
    write(fd, &hdr->filetype_header, 4);
    write(fd, &hdr->format_marker, 4);
    write(fd, &hdr->data_header_length, 4);
    write(fd, &hdr->format_type, 2);
    write(fd, &hdr->number_of_channels, 2);
    write(fd, &hdr->sample_rate, 4);
    write(fd, &hdr->bytes_per_second, 4);
    write(fd, &hdr->bytes_per_frame, 2);
    write(fd, &hdr->bits_per_sample, 2);
    write(fd, "data", 4);

    uint32_t data_size = hdr->file_size - 36;
    write(fd, &data_size, 4);

    return 0;
}

int recordWAV(const char *fileName, WaveHeader *hdr, uint32_t duration)
{
    int err;
    int size;
    snd_pcm_t *handle;
    snd_pcm_hw_params_t *params;
    unsigned int sampleRate = hdr->sample_rate;
    int dir;
    snd_pcm_uframes_t frames = 32;
    const char *device = "plughw:1,0"; // USB microphone
    // const char *device = "default"; // Integrated system microphone
    char *buffer;
    int filedesc;

    /* Open PCM device for recording (capture). */
    err = snd_pcm_open(&handle, device, SND_PCM_STREAM_CAPTURE, 0);
    if (err)
    {
        fprintf(stderr, "Unable to open PCM device: %s\n", snd_strerror(err));
        return err;
    }

    /* Allocate a hardware parameters object. */
    snd_pcm_hw_params_alloca(&params);

    /* Fill it in with default values. */
    snd_pcm_hw_params_any(handle, params);

    /* ### Set the desired hardware parameters. ### */

    /* Interleaved mode */
    err = snd_pcm_hw_params_set_access(handle, params, SND_PCM_ACCESS_RW_INTERLEAVED);
    if (err)
    {
        fprintf(stderr, "Error setting interleaved mode: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }
    /* Signed 16-bit little-endian format */
    if (hdr->bits_per_sample == 16) err = snd_pcm_hw_params_set_format(handle, params, SND_PCM_FORMAT_S16_LE);
    else err = snd_pcm_hw_params_set_format(handle, params, SND_PCM_FORMAT_U8);
    if (err)
    {
        fprintf(stderr, "Error setting format: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }
    /* Two channels (stereo) */
    err = snd_pcm_hw_params_set_channels(handle, params, hdr->number_of_channels);
    if (err)
    {
        fprintf(stderr, "Error setting channels: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }
    /* 44100 bits/second sampling rate (CD quality) */
    sampleRate = hdr->sample_rate;
    err = snd_pcm_hw_params_set_rate_near(handle, params, &sampleRate, &dir);
    if (err)
    {
        fprintf(stderr, "Error setting sampling rate (%d): %s\n", sampleRate, snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }
    hdr->sample_rate = sampleRate;
    /* Set period size*/
    err = snd_pcm_hw_params_set_period_size_near(handle, params, &frames, &dir);
    if (err)
    {
        fprintf(stderr, "Error setting period size: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }
    /* Write the parameters to the driver */
    err = snd_pcm_hw_params(handle, params);
    if (err < 0)
    {
        fprintf(stderr, "Unable to set HW parameters: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }

    /* Use a buffer large enough to hold one period */
    err = snd_pcm_hw_params_get_period_size(params, &frames, &dir);
    if (err)
    {
        fprintf(stderr, "Error retrieving period size: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        return err;
    }

    size = frames * hdr->bits_per_sample / 8 * hdr->number_of_channels; /* 2 bytes/sample, 2 channels */
    buffer = (char *) malloc(size);
    if (!buffer)
    {
        fprintf(stdout, "Buffer error.\n");
        snd_pcm_close(handle);
        return -1;
    }

    err = snd_pcm_hw_params_get_period_time(params, &sampleRate, &dir);
    if (err)
    {
        fprintf(stderr, "Error retrieving period time: %s\n", snd_strerror(err));
        snd_pcm_close(handle);
        free(buffer);
        return err;
    }

    uint32_t pcm_data_size = hdr->sample_rate * hdr->bytes_per_frame * (duration / 1000);
    hdr->file_size = pcm_data_size + 36;

    filedesc = open(fileName, O_WRONLY | O_CREAT, 0644);
    err = writeWAVHeader(filedesc, hdr);
    if (err)
    {
        fprintf(stderr, "Error writing .wav header.");
        snd_pcm_close(handle);
        free(buffer);
        close(filedesc);
        return err;
    }
    int totalFrames = 0;
    for(int i = ((duration * 1000) / (hdr->sample_rate / frames)); i > 0; i--)
    {
        err = snd_pcm_readi(handle, buffer, frames);
        totalFrames += err;
        if (err == -EPIPE) fprintf(stderr, "Overrun occurred: %d\n", err);
        if (err < 0) err = snd_pcm_recover(handle, err, 0);
        // Still an error, need to exit.
        if (err < 0)
        {
            fprintf(stderr, "Error occured while recording: %s\n", snd_strerror(err));
            snd_pcm_close(handle);
            free(buffer);
            close(filedesc);
            return err;
        }
        write(filedesc, buffer, size);
    }

    close(filedesc);
    snd_pcm_drain(handle);
    snd_pcm_close(handle);
    free(buffer);
    return 0;
}

(1) : Right now the program needs a USB device for input, I left a comment where the input device is declared so that it can be changed as needed. Also, I'm not sure the program will work as intended if PulseAudio is installed.

share|improve this question
    
Apparently, Rasberry Pi is little-endian by default; but can be big-endian. –  ChrisW Jan 19 at 1:03

4 Answers 4

up vote 15 down vote accepted

A few things jump out at me immediately:

  1. Use sizeof() whenever you need the size of something with non-dynamic allocation. In other words, all of your write calls on the WaveHeader struct members should be using sizeof, not hard coded sizes.

  2. Allocation and initialization should be separate concerns. There's no need to have the same method that initializes a struct allocate it (unless it's an opaque pointer, or you actually care where it's allocated). There's no reason your WaveHeader can't be on the stack.

    • As a general rule, use automatic allocation ("the stack") first and only move to dynamic allocation when you have some compelling reason (the reason usually being size varying, too big to fit on the stack, multithreading concerns, etc).

    • Separating out initialization and allocation makes it possible for the user of the API to decide what to do with memory. Unless you need to make this decision for them, don't.

    • If you're feeling lazy, you can always have a genericWavHeaderInit and genericWaveHeaderCreate

  3. Library code should never output anything. Use error codes and allow the caller to handle error reporting. What if the error should be ignored for some reason? Well, too late. You already put out an error.

    • Imagine if built in functions output errors to stderr. It would be absurd :).
  4. Operate on resources, not what you need to create those resources

    • RecordWAV should take a snd_pcm_t and allocate/initialize it.

    • Summarize to yourself what RecordWAV does. Truly go through all the steps, explaining to yourself what each chunk of code does. It does a lot more than just record a WAV.

    • As a happy side effect, it relieves your hard coded USB port. That really should come from a command line argument or a config file or something. Wanting to use different USB ports should not require a recompile.

    • Another happy side effect: your resource cleanup code doesn't have to be repeat a trillion times with every exit point (much less repetitive, but much more important: less error prone!)

  5. This is fairly subjective, but I don't like your naming scheme.

    • subjectVerb has the nice effect of provided a "namespace" of sorts (every widely used C library has a standard prefix (curl_, qt_, glib_, apr_, etc).

    • A suffix technically accomplishes the same namespacing effect, but it's much, much rarer

  6. Likewise, when a function's sole purpose is to operate on some object (i.e. struct) that struct should typically be the first parameter (writeWAVHeader).

  7. If you're going to go the camelCase route (which I don't know if I would in C, but that's 100% subjective) I would stick with strict camel casing (writeWavHeader). It's less visually jarring, it's easier to type, and anyone familiar with WAV will understand.

    • Same with RIFF_marker
  8. Include your own headers first, and then other headers. If your wav.h had a hidden dependency on a file, it could get hidden by the source file including it. It wouldn't be until you tried to include it without including that hidden dependency that you'd get a sudden mysterious undeclared symbol error.

  9. retrieveWAVHeader isn't defined. It also seems to be an unnecessary version of (WaveHeader*) ptr

  10. If the return of recordWAV is a ALSA constant (since it's one of the error codes), you should use whatever ALSA's success constant is. I can't imagine that it wouldn't be 0, but consistency with the other possible returns would be nice.

share|improve this answer

In C, I'd rather accept a pointer to a struct to be initialized than return newly allocated heap memory. It's clear that the caller owns the memory. The caller has the flexibility to pass you a pointer to a struct on its stack.

void initWAVHeader(WaveHeader *hdr, uint32_t sample_rate, uint16_t bit_depth, uint16_t channels);

The same goes for the rest of your functions. I'd make WaveHeader * the first parameter to all of the functions to give your library a sense of design consistency.

To fill in the struct, consider keeping a global static prototype. Instead of setting each field piecemeal, memcpy() the entire struct, then fill in the parts you need to change.


To write the header, you should be able to write the entire struct at once, as long as the struct is correctly packed in memory. Putting __attribute__((__packed__)) at the end of your struct declaration should ensure that GCC will not add padding between the members.


I take issue with this style:

/* Signed 16-bit little-endian format */
if (hdr->bits_per_sample == 16) err = snd_pcm_hw_params_set_format(handle, params, SND_PCM_FORMAT_S16_LE);
else err = snd_pcm_hw_params_set_format(handle, params, SND_PCM_FORMAT_U8);
if (err)
{
    fprintf(stderr, "Error setting format: %s\n", snd_strerror(err));
    snd_pcm_close(handle);
    return err;
}

The problems I see are:

  1. The if line is too long for my taste. The body should have been on a separate line.
  2. Too much repetition (of the snd_pcm_hw_params_set_format() call), and yet it's not immediately obvious what the code branches have in common.
  3. In C code where you have to check the error code on every library call, I like to make it a habit to never call a library function outside of an if condition. It's an idiom — if (OK != (err = library_call(…))) { handle_error(); }.

So, I'd write it this way (with an extra layer of parentheses to squelch compiler warnings about assigning inside an if):

/* Signed 16-bit little-endian format or Unsigned 8-bit? */
enum snd_pcm_format_t fmt = (hdr->bits_per_sample == 16) ?
    SND_PCM_FORMAT_S16_LE : SND_PCM_FORMAT_U8;
if ((err = snd_pcm_hw_params_set_format(handle, params, fmt))) {
    fprintf(stderr, "Error setting format: %s\n", snd_strerror(err));
    snd_pcm_close(handle);
    return err;
}
share|improve this answer

Other reviewers have already given you very good advice. Here are a few thoughts of mine to add:

Your recordWAV is supposed to receive a header that has been setup with genericWAVHeader. That function takes the desired sample rate and applies it to the header. However, within recordWAV, the functions that setup the PCM handle parameters determine the real sample rate (your call to snd_pcm_hw_params_set_rate_near modifies the suggested sample rate) and so the rate encoded in the header passed to recordWAV can be incorrect.

With your current arrangement there is no way to fix this. Which brings me to...

I think the public interface is wrong.

  • The header should not be public. The caller needs to know nothing about the header as it is an implementation detail of the WAV file.

  • There is no reason to ask the function to open a file as well as all of the other things it must do. Just pass in a file descriptor (or a FILE*)

So I would change the function prototype to:

int record_wav(int fd,
               const char *device,
               uint32_t sample_rate,
               uint16_t bit_depth,
               uint16_t channels,
               uint32_t duration);

The record function is much too big and the error handling is repeated much too often. I would prefer to see a simple function like this:

int record_wav(int fd, ...)
{
    snd_pcm_t *h;
    int err = open_pcm_channel(&h, ...);
    if (err) {
        fprintf(stderr, "Error opening channel: %s\n", snd_strerror(err));
        return err;
    }
    err = write_header(fd, &h, ...);
    if (!err) {
        err = write_data(fd, &h, ...);
    }
    if (err) {
        fprintf(stderr, "Error: %s\n", snd_strerror(err));
    }
    snd_pcm_drain(h);
    snd_pcm_close(h);
    return err;
}

In this, open_pcm_channel() opens the channel and configures it according to the call parameters to record_wav, write_header is the only thing to know about the header and write_data is the only thing to know about obtaining and writing data and it creates and frees the necessary buffer locally.

In open_pcm_channel() above, the snd_pcm_hw_params_t *params; is allocated, used and and freed within that function (not that you currently don't seem to free it). The chain of calls to 'snd' functions can be something like:

snd_pcm_hw_params_alloca(&params);
snd_pcm_hw_params_any(handle, params);
err = snd_pcm_hw_params_set_access(handle, params, SND_PCM_ACCESS_RW_INTERLEAVED);
if (!err) {
    err = snd_pcm_hw_params_set_format(handle, params, format);
}
if (!err) {
    err = snd_pcm_hw_params_set_channels(handle, params, hdr->number_of_channels);
}
if (!err) {
    err = snd_pcm_hw_params_set_rate_near(handle, params, &sampleRate, &dir);
}
if (!err) {
    err = snd_pcm_hw_params_set_period_size_near(handle, params, &frames, &dir);
}
if (!err) {
    err = snd_pcm_hw_params(handle, params);
}
if (!err) {
    err = snd_pcm_hw_params_get_period_size(params, &frames, &dir);
}
if (!err) {
    err = snd_pcm_hw_params_get_period_time(params, &sampleRate, &dir);
}

or even

snd_pcm_hw_params_alloca(&p);
snd_pcm_hw_params_any(h, p);
if ((err = snd_pcm_hw_p_set_access(h, p, SND_PCM_ACCESS_RW_INTERLEAVED))
    || ((err = snd_pcm_hw_params_set_format(h, p, format)))
    || ((err = snd_pcm_hw_params_set_channels(h, p, hdr->number_of_channels)))
    || ((err = snd_pcm_hw_params_set_rate_near(h, p, &sampleRate, &dir)))
    || ((err = snd_pcm_hw_params_set_period_size_near(h, p, &frames, &dir)))
    || ((err = snd_pcm_hw_params(h, p)))
    || ((err = snd_pcm_hw_params_get_period_size(p, &frames, &dir)))
    || ((err = snd_pcm_hw_params_get_period_time(p, &sampleRate, &dir)))) {
    // handle error
}

Note that the call to snd_pcm_hw_params_get_period_time seems redundant and its call parameter sampleRate is misnamed (function gets that time not the rate). Also the call to snd_pcm_hw_params_get_period_size seems redundant because snd_pcm_hw_params_set_period_size_near returned the size used in its call parameter.

Some other points:

The WAV file format uses little-endian numbers in the header. If you are running on a big-endian CPU your functions will not write the header correctly.

You have no check for errors on opening or writing the file.

Your params is not checked for allocation error and is not freed.

share|improve this answer
    
That long chain could be err = snd_pcm_func1() || snd_pcm_func2() || snd_pcm_func_last() –  200_success Jan 18 at 23:30
    
@200_success IMO that would lose the int value, and leave a boolean in err. –  ChrisW Jan 19 at 0:57
    
@200_success - not if you want the final value of err to equal the value returned by a failing function. In your example err will hold 0 or 1. –  William Morris Jan 19 at 0:57
    
Oh, right. I was thinking of the || operator in e.g. Perl or JavaScript, which gives the first truthy value. –  200_success Jan 19 at 2:56

Portability

  • Right now you have a pretty large dependency staring right at you on the first line.

    #include <alsa/asoundlib.h>
    

    With that one declaration, I now know that your "library" is tailored to a specific sound architecture. Try running your code on Windows or a Mac, and your library won't cut it.

    Instead, you may need to write a wrapper for a great library named PortAudio. Just read the first paragraph on their website if you don't believe me in thinking that one of the best C libraries out there.

    PortAudio is a free, cross-platform, open-source, audio I/O library. It lets you write simple audio programs in 'C' or C++ that will compile and run on many platforms including Windows, Macintosh OS X, and Unix (OSS/ALSA). It is intended to promote the exchange of audio software between developers on different platforms. Many applications use PortAudio for Audio I/O.

    I think your choice is clear. Ditch the DIY method for dealing with a ton of sound architecture API's, and create a wrapper for the PortAudio library. You won't find yourself disappointed.

  • On top of the sound architecture dependency, you then assign another dependency to a specific device.

    const char *device = "plughw:1,0"; // USB microphone
    

    How is this a library? Luckily, PortAudio also can do all of the hard work for you here as well, with the function Pa_GetDefaultInputDevice().

Standards

  • You probably shouldn't use the write() function anymore since they aren't standardized; instead prefer fwrite().

    C99 & C11 §7.19

    Many implementations of the C runtime environment, most notably the UNIX operating system, provide, aside from the standard I/O library’s fopen, fclose, fread, fwrite, and fseek, a set of unbuffered I/O services, open, close, read, write, and lseek. The C89 Committee decided not to standardize the latter set of functions.

    In addition, buffered I/O is always faster than unbuffered. There are cases where you might want to use unbuffered, such as whenever you want to ensure that the output has been written before continuing. But in this case you will want to use fwrite().

Library Design

  • Your whole library was built to write out audio to a .wav file, and it works. But what if I want to write out a .wav file the way Apple writes them? (yes, they are different from the way Windows writes them for some reason) Or what if I want a .flac file? I see you have written a .wav to .flac converter, but converting between formats takes an unnecessary amount of time.

    As a programmer, you should always be looking for ways to make your program processes more dynamic and efficient (optimally, at the same time). Storing the audio directly into a .flac file would be more efficient, for example, than converting an audio file to the specific codec. So how can we do this?

    Enter in libsndfile. This library can read and write almost every form of audio codec you can think of. It's more dynamic than your current implementation, and it is easier to switch between the output audio file types.

Syntax/Style

  • You should almost always be initializing variables when you declare them.

    WaveHeader *hdr  = malloc(sizeof(*hdr));
    
  • You switch between camelCase and snake_case. You need to be consistent.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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