Writing computer generated music to a .wav file in C

Question

See the next iteration.

I wrote this simple C program for writing computer generated music to a WAV file over three years ago. I refactored it a little, but it does not look good to me. Any suggestions?

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

/******************************
*  Magic file format strings. *
******************************/
const char fChunkID[]     = {'R', 'I', 'F', 'F'};
const char fFormat[]      = {'W', 'A', 'V', 'E'};
const char fSubchunk1ID[] = {'f', 'm', 't', ' '};
const char fSubchunk2ID[] = {'d', 'a', 't', 'a'};

/********************************
* WriteWavePCM() configuration: *
* - 2 channels,                 *
* - frequency 44100 Hz.         *
********************************/
const unsigned short N_CHANNELS = 2;
const unsigned int SAMPLE_RATE = 44100;
const unsigned short BITS_PER_BYTE = 8;

bool WriteWavePCM(short* sound, size_t pairAmount, char* fileName){
    const static unsigned int fSubchunk1Size = 16;
    const static unsigned short fAudioFormat = 1;
    const static unsigned short fBitsPerSample = 16;

    unsigned int fByteRate = SAMPLE_RATE * N_CHANNELS *
                             fBitsPerSample / BITS_PER_BYTE;

    unsigned short fBlockAlign = N_CHANNELS * fBitsPerSample / BITS_PER_BYTE;
    unsigned int fSubchunk2Size;
    unsigned int fChunkSize;

    FILE* fout;
    size_t ws;

    if (!sound || !fileName || !(fout = fopen( fileName, "w" ))) return false;

    fSubchunk2Size = pairAmount * N_CHANNELS * fBitsPerSample / BITS_PER_BYTE;
    fChunkSize = 36 + fSubchunk2Size;

    // Writing the RIFF header:
    fwrite(&fChunkID, 1, sizeof(fChunkID),      fout);
    fwrite(&fChunkSize,  sizeof(fChunkSize), 1, fout);
    fwrite(&fFormat, 1,  sizeof(fFormat),       fout);

    // "fmt" chunk:
    fwrite(&fSubchunk1ID, 1, sizeof(fSubchunk1ID),      fout);
    fwrite(&fSubchunk1Size,  sizeof(fSubchunk1Size), 1, fout);
    fwrite(&fAudioFormat,    sizeof(fAudioFormat),   1, fout);
    fwrite(&N_CHANNELS,      sizeof(N_CHANNELS),     1, fout);
    fwrite(&SAMPLE_RATE,     sizeof(SAMPLE_RATE),    1, fout);
    fwrite(&fByteRate,       sizeof(fByteRate),      1, fout);
    fwrite(&fBlockAlign,     sizeof(fBlockAlign),    1, fout);
    fwrite(&fBitsPerSample,  sizeof(fBitsPerSample), 1, fout);

    /* "data" chunk: */
    fwrite(&fSubchunk2ID, 1, sizeof(fSubchunk2ID),      fout);
    fwrite(&fSubchunk2Size,  sizeof(fSubchunk2Size), 1, fout);

    /* sound data: */
    ws = fwrite(sound, sizeof(short), pairAmount * N_CHANNELS, fout);
    fclose(fout);
    return true;
}

/************************************
* Around 23 seconds of pure techno! *
************************************/
const unsigned int N_SAMPLE_PAIRS = 1048576;

int main(int argc, char* argv[]){
    short* sound;
    int i;
    int j;
    bool status;
    char* file_name;

    sound = (short*) malloc( sizeof(short) * N_SAMPLE_PAIRS * N_CHANNELS );

    if (!sound)
    {
        puts("Could not allocate space for the sound data.");
        return (EXIT_FAILURE);
    }

    for (i = 0, j = 0; i < N_SAMPLE_PAIRS * N_CHANNELS; i += 2, j++ )
    {
        short datum1 = 450 * ((j >> 9 | j >> 7 | j >> 2) % 128);
        short datum2 = 450 * ((j >> 11 | j >> 8 | j >> 3) % 128);

        sound[i]     = datum1; // One channel.
        sound[i + 1] = datum2; // Another channel.
    }

    file_name = argc > 1 ? argv[1] : "Default.wav";
    status = WriteWavePCM(sound, N_SAMPLE_PAIRS, file_name);
    free(sound);

    if (status)
    {
        printf("Discotheque is ready in \"%s\"\n", file_name);
    }
    else
    {
        puts( "Something seems to have gone wrong." );
        return (EXIT_FAILURE);
    }

    return 0;
}

(What comes to playing the generated file, VLC and QuickTime Player are capable to play it; not sure about other media players.)

Answer 1

Endianness

Your program works on little endian machines only. If you care for it to work on any machine, you would need to fix up all of the fwrite() calls to output specifically in little endian format.

Use struct for header

I would suggest creating a struct for the file header. That way, you could fill in all the fields (in an endian correct manner) and then do one single fwrite(). From the code it looks like you can combine all the fields into a single header, but if it makes more sense you could have multiple headers (RIFF header, format header, data header, etc).

Answer 2

fopen should have a b in the second parameter. This primarily prevents windows from replacing any \n byte with \r\n which would corrupt the file.

fopen( fileName, "wb" )

Make the order of the fwrite parameters consistent first the sizeof and then the number of elements:

// Writing the RIFF header:
fwrite(&fChunkID,    sizeof(fChunkID),   1, fout);
fwrite(&fChunkSize,  sizeof(fChunkSize), 1, fout);
fwrite(&fFormat,     sizeof(fFormat),    1, fout);

Speaking of sizes the size of the standard primitives are not guaranteed to be exactly the same on all platforms (long being a particular pain). Instead use the integer types from stdint.h.

Answer 3

It actually doesn't look too bad. Some minor nitpicks

1. You shouldn't cast the return value of malloc.

2. fByteRate and fBlockAlign can be const since they're being computed from const values only.

3. I'm not entirely sure why pretty much all variables in WriteWavePCM are prefixed with f. I suppose this is meant to indicate that they are related to data being written to a file. Since most code in this method is related to writing things to a file this seems redundant to me and a bit distracting but YMMV.

4. A magic constant is being used here fChunkSize = 36 + fSubchunk2Size; - replace with named constant (calculated or defined - depends where the 36 comes from).

5. fBitsPerSample is defined to 16 which assumes that short is 16bits (since it's related to writing the short* sound data into the file). This may not be the case on all platforms. So fBitsPerSample should either be sizeof(*sound) or being passed in.

6. You use inconsistent bracing style:

   int main(int argc, char* argv[]){
   

   vs

   if (!sound)
   {
   

7. You have defined N_CHANNELS but the main loop is hard coded to increment by 2, so this

   for (i = 0, j = 0; i < N_SAMPLE_PAIRS * N_CHANNELS; i += 2, j++ )
   

   should be

   for (i = 0, j = 0; i < N_SAMPLE_PAIRS * N_CHANNELS; i += N_CHANNELS, j++)
   

Answer 4

In addition to the excellent advice given in the other answers, I'd recommend completely overhauling this loop:

for (i = 0, j = 0; i < N_SAMPLE_PAIRS * N_CHANNELS; i += 2, j++ )
{
    short datum1 = 450 * ((j >> 9 | j >> 7 | j >> 2) % 128);
    short datum2 = 450 * ((j >> 11 | j >> 8 | j >> 3) % 128);

    sound[i]     = datum1; // One channel.
    sound[i + 1] = datum2; // Another channel.
}

You have 9 magical constants in there. They should have sensible names to describe what they do.

Whenever you see this:

    sound[i]     = datum1; // One channel.
    sound[i + 1] = datum2; // Another channel.

it's an indication that you really wanted a struct. You should define a structure that contains 2 pieces of data instead of an array where you have to know that every other piece of data is one channel. Something like:

struct AudioSample {
    int16_t leftChannel;
    int16_t rightChannel;
};

Or whatever's appropriate for your use case. That would also allow you to write the loop with only a single control variable, since i and j would now be equal.

Answer 5

No reason to pack a lot of conditions in the same if:

if (!sound || !fileName || !(fout = fopen( fileName, "w" ))) 

Could very well be:

if (sound == NULL || fileName == NULL) {
    return false;
} 

FILE * fout = fopen(fileName, "wb");
if (fout == NULL) {
    return false
} 

Personal preference, but like to avoid assignments inside conditional, they should have very good visibility. I also prefer to compare against NULL instead of the nearly invisible ! when it comes to pointers, again to maximize visibility.

You also perform those checks, which might terminate the function, after several variable assignments, which are lost work if the parameters are null or the file can't be opened. Not a big deal, but nevertheless, suboptimal. C99 allows you to declare variables anywhere, so can move those checks to the top of the function.

---

You assign to ws in here:

ws = fwrite(sound, sizeof(short), pairAmount * N_CHANNELS, fout);

But that variable is never used. I suppose that was meant for error checking initially? Well, you do no error checking, which is very optimistic. fwrite might fail.

---

I like to keep the global scope as clear as possible, so I'd move all constants that are only being used inside the write wav function to the function scope. You can make them static constants to ensure they are not reallocated on the stack every time.