42
\$\begingroup\$

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

\$\endgroup\$
  • 2
    \$\begingroup\$ What does it sound like? \$\endgroup\$ – Rowan Sep 22 '15 at 7:18
  • \$\begingroup\$ Some sort of techno. I don't know why, but the generated music maintains a constant rhythm. \$\endgroup\$ – coderodde Sep 22 '15 at 7:19
17
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ Is there a chance that the struct will be "padded" (holes between two consecutive fields)? If so, it will break things? \$\endgroup\$ – coderodde Sep 22 '15 at 5:54
  • \$\begingroup\$ Also, how can I deal with endianness? google does not help much. \$\endgroup\$ – coderodde Sep 22 '15 at 6:27
  • \$\begingroup\$ @coderodde You do need to be careful about struct padding but normally these types of headers are packed "correctly" without gaps. You can use #pragma pack if they are not. For endianness, you need to fill in your structure either normally when on a littleendian machine or in reverse byte order when on a bigendian machine. If you include <endian.h> or <bswap.h> there are macros that help you do so. \$\endgroup\$ – JS1 Sep 22 '15 at 6:53
15
\$\begingroup\$

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.

\$\endgroup\$
11
\$\begingroup\$

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++)
    
\$\endgroup\$
9
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I don't know what the magic constants do. They affect the "music" being generated, but I don't know either how each of them affect anything. \$\endgroup\$ – coderodde Sep 22 '15 at 6:05
  • \$\begingroup\$ "I wrote this simple C program for writing computer generated music to a WAV file over three years ago" vs. "I don't know what the magic constants do. They affect the "music" being generated, but I don't know either how each of them affect anything" - this strikes me as odd, but if nothing else rather ably demonstrates how important comments are... \$\endgroup\$ – Eight-Bit Guru Sep 22 '15 at 15:33
  • 1
    \$\begingroup\$ In addition to what @Eight-BitGuru said, I can tell you what at least 2 of them do. The math: 450 * (anything % 128) is going to quantize the results and scale them. (anything % 128) limits the values to being between 0 and 128. Since you've got 16-bit samples, and 128 values fit in 7-bits, it's going to be very quiet, so you need to scale it to be between 0 and 2^16. Normally you'd multiply by 512, but using 450 gives you some headroom if you want to mix it. Of course by scaling an 8-bit value to 16-bit, you're also leaving spaces (hence the quantization). \$\endgroup\$ – user1118321 Sep 22 '15 at 16:11
8
\$\begingroup\$

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.

\$\endgroup\$

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.