Recording audio in C

Please note there are newer revisions of this code, one here, and one here for continuous audio recording.

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)

wav.h

#include 

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
#include
#include
#include
#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);

A few things jump out at me immediately:

-
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.

-
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

-
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 standard library functions output errors to stderr. It would be absurd :).

-
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!)

-
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

-
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).

-
If you're going to go the camelCase route (which I don't know if I would in C, but that's 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

-
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.

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

-
If the return of recordWAV is an 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.

StackExchange Code Review Q#39521, answer score: 21