Thread: 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?

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

Originally Posted by algorism

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;
}

I guess I should return to directly using the hex values (like 0x46464952 for "RIFF").

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

Originally Posted by whiteflags

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.

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