10
\$\begingroup\$

See the previous and initial iteration. See the next iteration.

Now I have refactored the code according to many suggestions made in the first iteration.

What's new:

  1. I attempted to deal with endianness. I have zero chance of running this on a big-endian machine. If you do, could you add a minicomment whether it works/not?
  2. Now I use a struct holding all metadata for WAV files, and write it to a file in one invocation to fwrite.
  3. fopen(fileName, "w") is now fopen(fileName, "wb") as the file is binary.
  4. if statement conditions are shorter.
  5. WriteWavePCM checks whether file output works/not.
  6. A couple of really minor changes.

What this program does: it generate some sort of rhytmical music and writes it to a WAV file that can be played, to my knowledge, using VLC or QuickTime Player.

See what I have:

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

/****************************************************
* From http://stackoverflow.com/questions/12791864/ *
*      c-program-to-check-little-vs-big-endian      *
****************************************************/
static bool is_little_endian() 
{
    volatile uint32_t i = 0x01234567;
    return (*((uint8_t*)(&i))) == 0x67;
}

static uint16_t little_endian_uint16_t(uint16_t num) 
{
    if (is_little_endian()) 
    {
        return num;
    }

    return (((0xff00 & num) >> 8) | ((0xff & num) << 8));
}

static uint32_t little_endian_uint32_t(uint32_t num) {
    if (is_little_endian()) 
    {
        return num;
    }

    return ((((0xff000000 & num) >> 24) | 
             ((0xff & num) << 24)       | 
             ((0xff0000 & num) >> 8))   | 
             ((0xff00 & num) << 8));
}

#pragma pack(0)
typedef struct {

    /***************
    * RIFF header. *
    ***************/
    char fChunkID[4];
    uint32_t fChunkSize;
    char fFormat[4];

    /******************
    * "fmt" subchunk. *
    ******************/
    char fSubchunk1ID[4];
    uint32_t fSubchunk1Size;
    uint16_t fAudioFormat;
    uint16_t fNumChannels;
    uint32_t fSampleRate;
    uint32_t fByteRate;
    uint16_t fBlockAlign;
    uint16_t fBitsPerSample;

    /*******************
    * "data" subchunk. *
    *******************/
    char fSubchunk2ID[4];
    uint32_t fSubchunk2Size;
} file_format_header;


bool WriteWavePCM(uint16_t* sound, size_t pairAmount, char* fileName)
{
    if (sound == NULL || fileName == NULL) 
    {
        return false;
    }

    FILE* fout = fopen(fileName, "wb");

    if (fout == NULL) 
    {
        return false;
    }

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

    /*****************************************************************
    * Can't make the following local static, as we need to check the *
    * endianness.                                                    *
    *****************************************************************/
    uint16_t N_CHANNELS                   = little_endian_uint16_t(2);
    uint32_t fSubchunk1Size               = little_endian_uint32_t(16);
    uint16_t fAudioFormat                 = little_endian_uint16_t(1);
    uint16_t fBitsPerSample               = little_endian_uint16_t(16);
    uint32_t fRIFFChunkDescriptorLength   = little_endian_uint32_t(12);
    uint32_t fFmtSubChunkDescriptorLength = little_endian_uint32_t(24);

    file_format_header hdr;

    /**************************************
    * Load the magic file format strings. *
    **************************************/
    for (size_t i = 0; i < 4; ++i) 
    {
        hdr.fChunkID[i]     = fChunkID[i];
        hdr.fFormat[i]      = fFormat[i];
        hdr.fSubchunk1ID[i] = fSubchunk1ID[i];
        hdr.fSubchunk2ID[i] = fSubchunk2ID[i];
    }

    /********************************
    * WriteWavePCM() configuration: *
    * - 2 channels,                 *
    * - frequency 44100 Hz.         *
    ********************************/
    uint32_t SAMPLE_RATE   = little_endian_uint32_t(44100);
    uint16_t BITS_PER_BYTE = little_endian_uint16_t(8);

    const uint32_t fByteRate = 
        little_endian_uint32_t(SAMPLE_RATE * N_CHANNELS * fBitsPerSample / 
                               BITS_PER_BYTE);

    const uint16_t fBlockAlign =
        little_endian_uint16_t(N_CHANNELS * fBitsPerSample / BITS_PER_BYTE);

    const uint32_t fSubchunk2Size = 
        little_endian_uint32_t(pairAmount * N_CHANNELS * fBitsPerSample / 
                               BITS_PER_BYTE);

    const uint32_t fChunkSize = 
        little_endian_uint32_t(fRIFFChunkDescriptorLength + 
                               fFmtSubChunkDescriptorLength + fSubchunk2Size);

    hdr.fAudioFormat   = fAudioFormat;
    hdr.fBitsPerSample = fBitsPerSample;
    hdr.fBlockAlign    = fBlockAlign;
    hdr.fByteRate      = fByteRate;
    hdr.fChunkSize     = fChunkSize;
    hdr.fNumChannels   = N_CHANNELS;
    hdr.fSampleRate    = SAMPLE_RATE;
    hdr.fSubchunk1Size = fSubchunk1Size;
    hdr.fSubchunk2Size = fSubchunk2Size;

    /******************************** 
    * Write the file format header. *
    ********************************/
    size_t ws = fwrite(&hdr, sizeof(hdr), 1, fout);

    if (ws != 1) 
    {
        fclose(fout);
        return false;
    }

    /************************ 
    * Write the sound data. *
    ************************/
    ws = fwrite(sound, sizeof(uint16_t), pairAmount * N_CHANNELS, fout);
    fclose(fout);
    return ws == pairAmount * N_CHANNELS;
}

int main(int argc, char* argv[]){
    /************************************
    * Around 23 seconds of pure techno! *
    ************************************/
    static const unsigned int N_SAMPLE_PAIRS = 1048576;

    uint16_t N_CHANNELS = little_endian_uint16_t(2);
    uint16_t* sound = malloc(sizeof(uint16_t) * N_SAMPLE_PAIRS * N_CHANNELS);

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

    uint16_t j;

    for (size_t i = 0, j = 0; i < N_SAMPLE_PAIRS * N_CHANNELS; i += 2, j++)
    {
        uint16_t datum1 = 
               little_endian_uint16_t(450 * ((j >> 9 | j >> 7 | j >> 2) % 128));

        uint16_t datum2 = 
              little_endian_uint16_t(450 * ((j >> 11 | j >> 8 | j >> 3) % 128));

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

    char* file_name = argc > 1 ? argv[1] : "Default.wav";
    bool 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;
}

Anything else to improve?

\$\endgroup\$
  • \$\begingroup\$ main.cpp:182:8: error: cannot initialize a variable of type 'char *' with an rvalue of type 'const char *'. Fix: const char *. \$\endgroup\$ – theoden Sep 22 '15 at 12:00
  • \$\begingroup\$ @theoden I have changed my source some time ago. Can you tell the name of the function and the row within that function? (CLang compiles with no errors/warnings.) \$\endgroup\$ – coderodde Sep 22 '15 at 12:04
  • \$\begingroup\$ @theoden If you meant the line char* file_name = argc > 1 ? ..., try changing to const char* file_name ? ... \$\endgroup\$ – coderodde Sep 22 '15 at 12:07
  • \$\begingroup\$ my clang doesn't (darwin 13.4.0). Yes, exactly. but that's not an answer, just mentioned portability issues: main.cpp:162:12: error: cannot initialize a variable of type 'uint16_t *' (aka 'unsigned short *') with an rvalue of type 'void *' uint16_t* sound = malloc(sizeof(uint16_t) * N_SAMPLE_PAIRS * N_CHANNELS); main.cpp:182:8: error: cannot initialize a variable of type 'char *' with an rvalue of type 'const char *' char* file_name = argc > 1 ? argv[1] : "Default.wav"; 2 errors generated. \$\endgroup\$ – theoden Sep 22 '15 at 12:09
  • \$\begingroup\$ I have darwin14.1.0. \$\endgroup\$ – coderodde Sep 22 '15 at 12:12
2
\$\begingroup\$
  1. Function accepts any number of arguments, but should accept none.

    static bool is_little_endian() 
    static bool is_little_endian(void) 
    
  2. Pedantic: int/unsigned may be as small as 16 bits. A problem for left shifts.

    ((0xffL & num) << 24)       | 
    ((0xff00L & num) << 8));
    // or   
    ((UINT32_C(0xff) & num) << 24)       | 
    ((UINT32_C(0xff00) & num) << 8));
    
  3. Minor: () not needed, sizeof(hdr) --> sizeof hdr

  4. Style: I favor the style of sizeof *pointer rather than sizeof pointer_type. Nice to begin with sizeof() as that insures multiplication is done at least as size_t.

    uint16_t* sound = malloc(sizeof(uint16_t) * N_SAMPLE_PAIRS * N_CHANNELS);
    uint16_t* sound = malloc(sizeof *sound * N_SAMPLE_PAIRS * N_CHANNELS);
    
  5. Like #4

    // fwrite(sound, sizeof(uint16_t), pairAmount * N_CHANNELS, fout)
    fwrite(sound, sizeof *sound, pairAmount * N_CHANNELS, fout)
    
  6. Design thought: Error messages on stderr

    // puts("Something seems to have gone wrong.");
    fputs("Something seems to have gone wrong.\n", stderr);
    
  7. What are the magic numbers 450, 9, 7, 2, 128?

    little_endian_uint16_t(450 * ((j >> 9 | j >> 7 | j >> 2) % 128));`
    
  8. Format inconsistency. This implies you are not using a format tool before committing code. Recommend using automated formating before review.

    bool WriteWavePCM(uint16_t* sound, size_t pairAmount, char* fileName)
    {
    ...
    int main(int argc, char* argv[]){
    
  9. is_little_endian() is a weak little endian check as it tests only 1 byte. The opposite of little endian is not big endian as there are more than 2 (although the others are rare). Recommend ntoh()

  10. Use const char* fileName so code may call WriteWavePCM(sound, N_SAMPLE_PAIRS, "Default.wav");

\$\endgroup\$
  • \$\begingroup\$ Nice one. However, a minor comment on 7: those magic numbers are responsible for generating music, and I have no idea how they affect anything. \$\endgroup\$ – coderodde Sep 23 '15 at 6:18
  • 1
    \$\begingroup\$ @coderodde Possible a comment to that effect even if it is // magic numbers are responsible for generating music - details unknown \$\endgroup\$ – chux Sep 23 '15 at 6:19
  • \$\begingroup\$ When coding physical quantities: I like posting the units near the code like (44100 /* Hz */) or as a define #define SAMPLE_FREQUENCY (44100 /* Hz */) ... little_endian_uint32_t(SAMPLE_FREQUENCY). Anyways - time for my beauty sleep. ZZzzzz. Maybe more later zzzzzzzz \$\endgroup\$ – chux Sep 23 '15 at 6:25
  • \$\begingroup\$ I've seen such algorithms before and they may as well be magic :) It'a like a fractal that is a whole piece of music in a few chars of C code and some really do sound varied, musical, structured as if designed, and stunning. You could make the analogy with those numbers as being like the time location, frequency and duration of notes in a conventional piece, I'm not sure how to concly describe that they are that sort of data in a comment. \$\endgroup\$ – alan2here Aug 5 '16 at 17:11
2
\$\begingroup\$

The program had a few problems on a big-endian machine (PowerMac G4, which is nice to have for exactly this sort of situation!) Once you convert a number to the opposite endian, you don't want to use it in any arithmetic calculations - so just wait until you put then in the .wav header to convert them.

--- original.c  2015-09-22 20:14:08.121610013 -0500
+++ test.c  2015-09-22 20:16:10.729507166 -0500
@@ -91,12 +91,12 @@
     * Can't make the following local static, as we need to check the *
     * endianness.                                                    *
     *****************************************************************/
-    uint16_t N_CHANNELS                   = little_endian_uint16_t(2);
-    uint32_t fSubchunk1Size               = little_endian_uint32_t(16);
-    uint16_t fAudioFormat                 = little_endian_uint16_t(1);
-    uint16_t fBitsPerSample               = little_endian_uint16_t(16);
-    uint32_t fRIFFChunkDescriptorLength   = little_endian_uint32_t(12);
-    uint32_t fFmtSubChunkDescriptorLength = little_endian_uint32_t(24);
+    uint16_t N_CHANNELS                   = 2;
+    uint32_t fSubchunk1Size               = 16;
+    uint16_t fAudioFormat                 = 1;
+    uint16_t fBitsPerSample               = 16;
+    uint32_t fRIFFChunkDescriptorLength   = 12;
+    uint32_t fFmtSubChunkDescriptorLength = 24;

     file_format_header hdr;

@@ -116,33 +116,33 @@
     * - 2 channels,                 *
     * - frequency 44100 Hz.         *
     ********************************/
-    uint32_t SAMPLE_RATE   = little_endian_uint32_t(44100);
-    uint16_t BITS_PER_BYTE = little_endian_uint16_t(8);
+    uint32_t SAMPLE_RATE   = 44100;
+    uint16_t BITS_PER_BYTE = 8;

     const uint32_t fByteRate = 
-        little_endian_uint32_t(SAMPLE_RATE * N_CHANNELS * fBitsPerSample / 
+        (SAMPLE_RATE * N_CHANNELS * fBitsPerSample / 
                                BITS_PER_BYTE);

     const uint16_t fBlockAlign =
-        little_endian_uint16_t(N_CHANNELS * fBitsPerSample / BITS_PER_BYTE);
+        (N_CHANNELS * fBitsPerSample / BITS_PER_BYTE);

     const uint32_t fSubchunk2Size = 
-        little_endian_uint32_t(pairAmount * N_CHANNELS * fBitsPerSample / 
+        (pairAmount * N_CHANNELS * fBitsPerSample / 
                                BITS_PER_BYTE);

     const uint32_t fChunkSize = 
-        little_endian_uint32_t(fRIFFChunkDescriptorLength + 
+        (fRIFFChunkDescriptorLength + 
                                fFmtSubChunkDescriptorLength + fSubchunk2Size);

-    hdr.fAudioFormat   = fAudioFormat;
-    hdr.fBitsPerSample = fBitsPerSample;
-    hdr.fBlockAlign    = fBlockAlign;
-    hdr.fByteRate      = fByteRate;
-    hdr.fChunkSize     = fChunkSize;
-    hdr.fNumChannels   = N_CHANNELS;
-    hdr.fSampleRate    = SAMPLE_RATE;
-    hdr.fSubchunk1Size = fSubchunk1Size;
-    hdr.fSubchunk2Size = fSubchunk2Size;
+    hdr.fAudioFormat   = little_endian_uint16_t(fAudioFormat);
+    hdr.fBitsPerSample = little_endian_uint16_t(fBitsPerSample);
+    hdr.fBlockAlign    = little_endian_uint16_t(fBlockAlign);
+    hdr.fByteRate      = little_endian_uint32_t(fByteRate);
+    hdr.fChunkSize     = little_endian_uint32_t(fChunkSize);
+    hdr.fNumChannels   = little_endian_uint16_t(N_CHANNELS);
+    hdr.fSampleRate    = little_endian_uint32_t(SAMPLE_RATE);
+    hdr.fSubchunk1Size = little_endian_uint32_t(fSubchunk1Size);
+    hdr.fSubchunk2Size = little_endian_uint32_t(fSubchunk2Size);

     /******************************** 
     * Write the file format header. *
@@ -169,7 +169,7 @@
     ************************************/
     static const unsigned int N_SAMPLE_PAIRS = 1048576;

-    uint16_t N_CHANNELS = little_endian_uint16_t(2);
+    uint16_t N_CHANNELS = 2;
     uint16_t* sound = malloc(sizeof(uint16_t) * N_SAMPLE_PAIRS * N_CHANNELS);

     if (!sound)
\$\endgroup\$
  • \$\begingroup\$ Was you able to play the .wav file on G4 in the first place? If so, did it sound the same way as on Intel? \$\endgroup\$ – coderodde Sep 23 '15 at 6:21
  • \$\begingroup\$ There was some problem with the sound card driver under Ubuntu 12.04 that I didn't feel like fixing. But I copied the .wav file to another computer and it did sound (and look) the same. \$\endgroup\$ – libertyernie Sep 23 '15 at 14:17
  • \$\begingroup\$ I don't understand. Did you manage to compile the source and play the file on a big-endian hardware? \$\endgroup\$ – coderodde Sep 23 '15 at 14:47
  • 1
    \$\begingroup\$ I compiled the source on big-endian and it generated the correct .wav file. The only reason I couldn't play it on that machine was because the sound card was not working, so I copied the .wav file to another computer to play it. I did open the file in Audacity on the PowerMac and the waveforms looked correct. \$\endgroup\$ – libertyernie Sep 23 '15 at 16:15
1
\$\begingroup\$

Each time you convert to little endian, you test if the machine is little endian. Looks a bit excessive; this test shall be done once and forever.

Options are

  • Making a second pass:

    fill sound array in native byte order;
    if (!is_little_endian())
        swap byte order
    
  • Using function pointers

    little_endian_uint16 = is_little_endian()
        ? nop
        : byte_swapper;
    
\$\endgroup\$
  • \$\begingroup\$ Actually, 99% of the time you know if the system where your code is going to run is LE or BE, so more often than not a simple compile-time switch is enough. \$\endgroup\$ – glampert Sep 22 '15 at 19:09
  • \$\begingroup\$ @glampert It is a viable solution for source-based distribution. Sometimes you want to distribute binaries. \$\endgroup\$ – vnp Sep 22 '15 at 19:23
1
\$\begingroup\$

It looks good. I can only suggest a couple minor changes.

  • Your use of #pragma pack(0) is not correct. A value of zero is not supported by most compilers that have the pack directive. According to MSDN, valid values for VS are 1, 2, 4, 8, and 16. I couldn't find the documentation for GCC nor have tested, but regardless, if you want to disable automatic padding added by the compiler, then the pack value should be 1. Also, it is good practice to restore the default padding afterwards, so that other structures declared further down are not affected. To do that, use the push/pop commands:

    #pragma pack(push, 1)
      // Structures defined here will not have any padding.
    #pragma pack(pop)
      // Back to compiler defaults.
    

    It is worth mentioning that #pragma pack is not Standard C, but it is widely supported on modern compilers. If you fiddle with some older versions of GCC, you might need to resort to the __attribute__((packed)) Gnu extension.

  • Strict C requires that you add a void to the parameter list of functions taking no arguments, so is_little_endian is missing it:

    static bool is_little_endian(void) {
                                 ^^^^
    
  • You might consider replacing the runtime checks for Little/Big Endian architecture by static compile-time checks using a compiler switch (#ifdefs). My experience is that you more often than not know the byte-order of the system where your program is going to run, so there's no need to pay for extra runtime checking if this can be resolved during compilation. This is also the case for programs distributed in source-code form. There's no standard macro defining the byte order of the system where you're compiling, but most platforms do define some global macro with this info, do a net search if you're interested. I for one find it easier to just require the user/programmer to specify that info to the build engine when compiling the source code, if it is necessary.

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