Thread: Critique my .wav writer code

  1. #1
    Citizen of Awesometown the_jackass's Avatar
    Join Date
    Oct 2014
    Location
    Awesometown
    Posts
    269

    Critique my .wav writer code

    Okay this is the .wav writing code I'll use for my useless little experiment:

    Code:
    #include <iostream>
    #include <fstream>
    #include <cmath>
    #include <vector>
    
    const double PI=3.1415926535897932384626433832795;
    
    typedef  uint8_t byte;
    typedef  uint16_t word;
    typedef  uint32_t dword;
    
    using namespace std;
    
    struct WaveFile {
          struct Riff {
              dword id,size,format;
          }riff;
    
          struct Fmt {
              dword id,size;
              word audioFormat,numChannels;
              dword sampleRate,byteRate;
              word blockAlign,bitsPerSample;
          }fmt;
    
          struct DataChunk {
            dword id,size;
          }dataChunk;
    
          vector<byte> data;
    
          WaveFile(dword sampleRate=8000,word bitsPerSample=8) {
              //cout<<"riff = "<<sizeof(riff)<<"\n"<<"fmt = "<<sizeof(fmt)<<"\ndataChunk = "<<sizeof(dataChunk)<<"\n";
    
                riff.id=0x46464952; // "RIFF"
                riff.format=0x45564157; // "WAVE"
    
                fmt={0};
                fmt.id=0x20746d66; // "fmt "
                fmt.size=16; // for PCM
                fmt.audioFormat=1; // PCM
                fmt.numChannels=1; // Mono
                fmt.sampleRate=sampleRate;
                fmt.byteRate=sampleRate*fmt.numChannels*bitsPerSample/8;
                fmt.blockAlign=fmt.numChannels*bitsPerSample/8;
                fmt.bitsPerSample=bitsPerSample;
    
                dataChunk={0};
                dataChunk.id=0x61746164; // "data"
          }
    
          void write(const char *fileName) {
              // first fill in the remaining fields
                dataChunk.size=data.size();
                riff.size=36+dataChunk.size;
    
              fstream f(fileName,ios::out|ios::binary);
                f.write(reinterpret_cast<char*>(&riff),sizeof(riff));
                f.write(reinterpret_cast<char*>(&fmt),sizeof(fmt));
                f.write(reinterpret_cast<char*>(&dataChunk),sizeof(dataChunk));
    
                f.write(reinterpret_cast<char*>(&data[0]),data.size());
                f.close();
          }
    
          // assumes bitsPerSample=8
          // fills with a simple sine wave of fixed frequency
          void testFillData() {
                double W=2*PI*100;
                double totalTime=2.0; // seconds
                double numSamples=totalTime*fmt.sampleRate;
    
                data.clear();
    
                //cout<<"sampleRate = "<<fmt.sampleRate<<", byteRate = "<<fmt.byteRate<<"\n";
    
                for(double i=0.0;i<fmt.byteRate*totalTime;i+=1) {
                double t=i/fmt.byteRate;
                      double sample=127*sin(W*t);
                      if(sample<-128.0) sample=-128.0;
                      else if(sample>127.0) sample=127.0;
                data.push_back(sample);
                }
          }
    };
    
    int main() {
        WaveFile wav;
        wav.testFillData();
        wav.write("sine_wave.wav");
        return 0;
    }
    Now my primary questions are:

    Is my method of writing the data to the file correct in that it doesent cause any struct packing and endianness issues (assuming same endianness of the machine that compiled my program and the wav format requirement)?

    Does the code do what I think it should do? The output sound seems too high for 100 Hz.

    Is anything else wrong/can be improved in the code?
    "Highbrow philosophical truth: Everybody is an ape in monkeytown" --Oscar Wilde

  2. #2
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    I would suggest at least that you don't try to serialize structs by writing them raw to a file. You never know what padding a compiler might sneak in. I would suggest you serialize the members one-by-one and use appropriate fixed-width types to declare variables (e.g. int16_t, uint32_t, etc).
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  3. #3
    Registered User
    Join Date
    May 2010
    Posts
    4,498
    Is my method of writing the data to the file correct in that it doesent cause any struct packing and endianness issues (assuming same endianness of the machine that compiled my program and the wav format requirement)?
    Probably not. This is what Clang says about your code, note the padding warning. The best way to insure you don't have padding issues is to write each item separately.

    ||=== Build: Debug in testcpp (compiler: LLVM Clang Compiler) ===|
    main.cpp|38|warning: missing field 'size' initializer [-Wmissing-field-initializers]|
    main.cpp|48|warning: missing field 'size' initializer [-Wmissing-field-initializers]|
    main.cpp|62|warning: implicit conversion changes signedness: 'size_type' (aka 'unsigned long') to 'streamsize' (aka 'long') [-Wsign-conversion]|
    main.cpp|54|warning: implicit conversion loses integer precision: 'size_type' (aka 'unsigned long') to 'dword' (aka 'unsigned int') [-Wshorten-64-to-32]|
    main.cpp|82|warning: implicit conversion turns floating-point number into integer: 'double' to 'value_type' (aka 'unsigned char') [-Wfloat-conversion]|
    main.cpp|71|warning: unused variable 'numSamples' [-Wunused-variable]|
    main.cpp|30|warning: padding struct 'WaveFile' with 4 bytes to align 'data' [-Wpadded]|
    ||=== Build finished: 0 error(s), 7 warning(s) (0 minute(s), 1 second(s)) ===|
    You should also note the implicit conversion issues.

    Jim

  4. #4
    Registered User
    Join Date
    Jun 2015
    Posts
    1,643
    Inventing new names for uint8_t, etc., just forces the reader to remember them for no real gain in this instance. The sizes aren't going to change and what better names then the ones they already have?. You also need to include <cstdint> if you're going to use those types. Don't rely on random includes in other header files.

    Your spacing is all over the place. A consistent 4-space indent will never do you wrong.

    Your value for PI is overly long. Doubles have nowhere near that precision. They hold 15 to 17 decimal digits:
    Code:
    const double PI1 = 3.1415926535897932;
    You may just want to calculate it:
    Code:
     const double PI = 2 * asin(1);

    You've lucked out (or not, depending on your pov) with your structs in that they probably don't have any padding (individually, at least). 16-bit objects occur in pairs along with 32-bit objects. Looks like a snug fit. However, I doubt that it's guaranteed. I usually write an input and output function for each struct that needs to be serialized to a binary file. In those funcs I read/write each member individually.
    Code:
    #include <iostream>
    #include <cmath>
    
    const double PI = 2 * std::asin(1);
    const double PI1 = 3.1415926535897932;
    const double PI2 = 3.1415926535897932384626433832795;
    
    int main() {
        // These all print exactly the same thing.
        // Digits after the 16th are pretty much meaningless.
        std::cout.precision(30);
        std::cout << PI << '\n';
        std::cout << PI1 << '\n';
        std::cout << PI2 << '\n';
    
        // Here's a nifty trick:
        uint32_t id = *(uint32_t*)"RIFF";
        uint32_t format = *(uint32_t*)"WAVE";
        std::cout << std::hex << id << '\n' << format << '\n';
    
        return 0;
    }

  5. #5
    Registered User
    Join Date
    Jun 2015
    Posts
    1,643
    Quote Originally Posted by jimblumberg View Post
    main.cpp|62|warning: implicit conversion changes signedness: 'size_type' (aka 'unsigned long') to 'streamsize' (aka 'long') [-Wsign-conversion]|
    Why does it complain about that one but not the three lines before it (where sizeof is used)? Don't they have the same problem? Isn't the type of sizeof also unsigned? Or is sizeof special?

  6. #6
    Registered User
    Join Date
    May 2010
    Posts
    4,498
    Why does it complain about that one but not the three lines before it (where sizeof is used)? Don't they have the same problem? Isn't the type of sizeof also unsigned?
    That's a good question, sizeof should return a size_t and std::vector::size() should also be returning a size_t. Perhaps it's because sizeof is a compile time constant? See this link for a similar discussion which seems come to this conclusion.

    By the way g++ version 6.1.0 also warns about that conversion (when you add the proper warning flag).


    Jim

  7. #7
    Citizen of Awesometown the_jackass's Avatar
    Join Date
    Oct 2014
    Location
    Awesometown
    Posts
    269
    Quote Originally Posted by algorism View Post
    Inventing new names for uint8_t, etc., just forces the reader to remember them for no real gain in this instance. The sizes aren't going to change and what better names then the ones they already have?.
    Well I find byte, word and dword easier to type than uint8_t, uint16_t and uint32_t.


    I believe the conversions between long, size_t and unsigned int wont cause any issues since my code would never deal with huge files that are larger than 2gb etc.

    I have also changed the file writing code (and incoporated algorism's trick regarding chunk ids).

    Oh and as regards the spacing/indentation, I dont know how it came to be...I dont add any extra spaces by myself just completely rely on CodeBlocks for this.

    Here's the new code:

    Code:
    #include <iostream>
    #include <fstream>
    #include <cmath>
    #include <cstdint>
    #include <vector>
    
    const double PI = 3.1415926535897932;
    
    typedef  uint8_t byte;
    typedef  uint16_t word;
    typedef  uint32_t dword;
    
    using namespace std;
    
    void writeByte(fstream &f,byte v) { f.put(v); }
    
    void writeWord(fstream &f,word v) {
          f.put(v&0xff);
          f.put((v>>8)&0xff);
    }
    
    void writeDword(fstream &f,dword v) {
          f.put(v&0xff);
          f.put((v>>8)&0xff);
          f.put((v>>16)&0xff);
          f.put((v>>24)&0xff);
    }
    
    struct WaveFile {
          struct Riff {
              dword id,size,format;
          }riff;
    
          struct Fmt {
              dword id,size;
              word audioFormat,numChannels;
              dword sampleRate,byteRate;
              word blockAlign,bitsPerSample;
          }fmt;
    
          struct DataChunk {
            dword id,size;
          }dataChunk;
    
          vector<byte> data;
    
          WaveFile(dword sampleRate=8000,word bitsPerSample=8) {
                riff.id=*(dword*)"RIFF"; // 0x46464952; // "RIFF"
                riff.format=*(dword*)"WAVE"; // 0x45564157; // "WAVE"
    
                fmt={0};
                fmt.id=*(dword*)"fmt "; // 0x20746d66; // "fmt "
                fmt.size=16; // for PCM
                fmt.audioFormat=1; // PCM
                fmt.numChannels=1; // Mono
                fmt.sampleRate=sampleRate;
                fmt.byteRate=sampleRate*fmt.numChannels*bitsPerSample/8;
                fmt.blockAlign=fmt.numChannels*bitsPerSample/8;
                fmt.bitsPerSample=bitsPerSample;
    
                dataChunk={0};
                dataChunk.id=*(dword*)"data"; // 0x61746164; // "data"
          }
    
          void write(const char *fileName) {
              // first fill in the remaining fields
                dataChunk.size=data.size();
                riff.size=36+dataChunk.size;
    
              fstream f(fileName,ios::out|ios::binary);
    
                writeDword(f,riff.id);
                writeDword(f,riff.size);
                writeDword(f,riff.format);
    
                writeDword(f,fmt.id);
                writeDword(f,fmt.size);
                writeWord(f,fmt.audioFormat);
                writeWord(f,fmt.numChannels);
                writeDword(f,fmt.sampleRate);
                writeDword(f,fmt.byteRate);
                writeWord(f,fmt.blockAlign);
                writeWord(f,fmt.bitsPerSample);
    
                writeDword(f,dataChunk.id);
                writeDword(f,dataChunk.size);
                f.write(reinterpret_cast<char*>(&data[0]),static_cast<dword>(data.size()));
    
                f.close();
          }
    
          // assumes bitsPerSample=8
          // fills with a simple sine wave of fixed frequency
          void testFillData() {
                double W=2*PI*100;
                double totalTime=2.0; // seconds
    
                data.clear();
    
                for(double i=0.0;i<fmt.byteRate*totalTime;i+=1) {
                double t=i/fmt.byteRate;
                      double sample=127*sin(W*t);
                      if(sample<-128.0) sample=-128.0;
                      else if(sample>127.0) sample=127.0;
                data.push_back(static_cast<byte>(sample));
                }
          }
    };
    
    int main() {
        WaveFile wav;
        wav.testFillData();
        wav.write("sine_wave.wav");
        return 0;
    }
    Last edited by the_jackass; 12-03-2016 at 03:10 AM.
    "Highbrow philosophical truth: Everybody is an ape in monkeytown" --Oscar Wilde

  8. #8
    Lurking whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    9,612
    I really dislike that you are depending on a hack like *(dword*)"data" for labels. There is no guarantee that numbers like 0x61746164 will persist between builds. The strings themselves should be small enough. If they aren't PLEASE just hash them.

  9. #9
    Citizen of Awesometown the_jackass's Avatar
    Join Date
    Oct 2014
    Location
    Awesometown
    Posts
    269
    I guess I should return to directly using the hex values (like 0x46464952 for "RIFF").
    "Highbrow philosophical truth: Everybody is an ape in monkeytown" --Oscar Wilde

  10. #10
    Lurking whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    9,612
    If that's what you want you can certainly do that.
    Code:
    #define RIFF_FMT 0x46464952
    Constants mean you care.

  11. #11
    Old Took
    Join Date
    Nov 2016
    Location
    Londonistan
    Posts
    121
    But constants should never be #define'd. Yuck!

    This is the C++ forum.

    I don't see the casting as being an issue personally although I think using reinterpret_cast<> rather than C-style casts would be slightly more readable.

  12. #12
    Registered User
    Join Date
    May 2010
    Posts
    4,498
    Using std::ostream.put() to write your "byte" is probably not the best choice, since that function uses a char for the parameter.

    Also Code::Blocks has a menu item to reformat code, in case something happens and the formatting gets messed up.

    Jim
    Last edited by jimblumberg; 12-03-2016 at 09:20 AM.

  13. #13
    Registered User
    Join Date
    May 2010
    Posts
    4,498
    But constants should never be #define'd. Yuck!
    True but there's nothing stopping you from using const qualified variables. Casts, should only be used as a last resort.

    Jim

  14. #14
    Registered User
    Join Date
    Jun 2015
    Posts
    1,643
    Quote Originally Posted by whiteflags View Post
    I really dislike that you are depending on a hack like *(dword*)"data" for labels.
    I believe you are mistaken. *(uint32_t*)"RIFF" is not a hack. It does exactly what is wanted and it does it correctly on every platform, unlike 0x46464952, which will be correct on a little-endian system but incorrect on a big-endian system. The "hack" will be correct on both.

    I'm not sure what this means:
    There is no guarantee that numbers like 0x61746164 will persist between builds.

    Quote Originally Posted by the_jackass
    Well I find byte, word and dword easier to type than uint8_t, uint16_t and uint32_t.
    I see. I didn't realize you were writing the program only for yourself and were especially concerned with reducing your typing. My point is that other people will be able to read your code more easily if you don't use invented names for objects which already have perfectly good names.

    BTW, I'm just making a point. I'm not saying to change them since it hardly matters in such a small program. However, I find "word" and "dword" to be totally arbitrary names. In what sense is a 16-bit object a "word"? What millenium is this?

    And if you want to save some keystrokes and also be kind to your readers, why not:
    Code:
    typedef uint8_t  u8;
    typedef uint16_t u16;
    typedef uint32_t u32;
    (Although most people would consider these names too short for global symbols.)
    Last edited by algorism; 12-03-2016 at 10:01 AM.

  15. #15
    Registered User
    Join Date
    Jun 2015
    Posts
    1,643
    Quote Originally Posted by jimblumberg View Post
    That's a good question, sizeof should return a size_t and std::vector::size() should also be returning a size_t. Perhaps it's because sizeof is a compile time constant? See this link for a similar discussion which seems come to this conclusion.
    That explanation makes sense. Since sizeof is evaluated at compile time the compiler can check that the conversion is benign. Not so with the vector size.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Critique my code
    By Aslaville in forum C Programming
    Replies: 2
    Last Post: 05-09-2015, 03:38 AM
  2. Please critique my source code - Craps Game
    By bos1234 in forum C Programming
    Replies: 8
    Last Post: 09-20-2013, 01:22 PM
  3. C code critique
    By mnd22 in forum C Programming
    Replies: 8
    Last Post: 12-23-2011, 10:05 AM
  4. Critique my code?
    By bigbadbowlindud in forum C++ Programming
    Replies: 20
    Last Post: 06-24-2004, 12:54 PM
  5. could ya'll critique this code
    By linuxdude in forum C Programming
    Replies: 0
    Last Post: 02-19-2004, 09:55 PM

Tags for this Thread