Generate sinusoid as Wave file

Question

This is a little program to show how a WAVe-File Generator could be written. It use a Function-Object "SinFunction" to calculate the value of any point of the amplitude in time. This object and two floats are given to the constructor of the generator object. The two floats are the frequency and the duration of the created signal.

You could give it other Function-Objects to produce the function of any expression you can write in C++. Or you could give SinFunction different frequency and/or duration values.

#include <iostream> /* Write File */
#include <fstream>
#include <string> /* File Name Parameter */
#include <math.h> /* sin */
using namespace std;
class SinFunction{
public:
 float operator() (float x){
 return sin(x);
 }
};
template <class Funk>
class WaveFileWriter{
private:
void longWrite(long x, ofstream &myfile)
{
 long i = x;
 int i1 = i % 256;
 i = i - i1;
 i = i / 256;
 int i2 = i % 256;
 i = i - i2;
 i = i / 256;
 int i3 = i % 256;
 i = i - i1;
 i = i / 256;
 int i4 = i;
 unsigned char a = (unsigned char)i1;
 unsigned char b = (unsigned char)i2;
 unsigned char c = (unsigned char)i3;
 unsigned char d = (unsigned char)i4;
 myfile << a;
 myfile << b;
 myfile << c;
 myfile << d;
}
void integerWrite(unsigned int x, ofstream &myfile)
{
 unsigned int i = x;
 int i1 = i % 256;
 i = i - i1;
 i = i / 256;
 unsigned char a = (unsigned char)i1;
 unsigned char b = (unsigned char)i;
 myfile << a;
 myfile << b;
}
float Frequency; // Frequency of playing Tone
float Duration ; // Duration on playing Tone
Funk Funktion;
public:
WaveFileWriter(string FileName, float Frequenz, float Duratione) : Frequency{Frequenz}, Duration{Duratione} {
 ofstream myfile;
 // Definition on File Name: "MeinTest.wav"
 myfile.open (FileName.c_str(), ios::out | ios::binary);
 // empty Char declaration
 char h = 0;
 // +++++++ Header ++++++
 // ----- WAV-Header ----
 // Chunk Name "RIFF"
 char RIFF[] = "RIFF";
 myfile << RIFF;
 // Chunk Lange
 longWrite(220000, myfile);
 // RIFF-Typ "WAVE"
 char WAVE[] = "WAVE";
 myfile << WAVE;
 // ----- FMT CHUNK ------
 // CHUNK-NAME 'FMT
 char FMT[] = "fmt ";
 myfile << FMT;
 // CHUNK Länge 10H
 longWrite(0x10, myfile);
 // Audio-Format
 integerWrite(1, myfile);
 // Kanalzahl
 integerWrite(2, myfile);
 // Samplerate (Hz) = 44 000
 long SampelsPerSecond = 44100;
 longWrite(SampelsPerSecond, myfile);
 // Bytes pro Sekunde = 44 000
 longWrite(176000, myfile);
 // Bytes pro Sampel
 integerWrite(4, myfile);
 // Bits pro Sampel
 integerWrite(16, myfile);
 // CHUNK-Name 'data'
 char data[] = "data";
 myfile << data;
 // CHUNK Lange
 int Time = 5;
 long DataLeng = (long)SampelsPerSecond * Time;
 longWrite(DataLeng, myfile);
 // +++++ DATA ++++
 float result;
 float value;
 unsigned int f;
 for(float m = 0; m <= DataLeng; m++)
 {
 // 1. Chanel
 result = Funktion(M_PI*2*(m/SampelsPerSecond)*Frequency);
 value = (result * 32767 ) + 32767;
 f = (unsigned int)value;
 integerWrite(f, myfile);
 //2. Chanel
 result = Funktion(M_PI*2*(m/SampelsPerSecond)*Frequency);
 value = (result * 32767 ) + 32767;
 f = (unsigned int)value;
 integerWrite(f, myfile);
 // myfile << (char)value;
 }
 // File is writen so close
 myfile.close();
}
};
int main( int argc, const char* argv[] )
{
 WaveFileWriter<SinFunction> File1{"MyTest.wav", 1000, 5};
 return 0;
}

I'm looking for ideas to refactor it, to make it perfect.

• 1201ProgramAlarm: Why are you writing the WAV header piece-by-piece, rather than declaring a structure?

Answer 1

Dont't use using namespace std;

---

Use consistent formatting:

Your vertical whitespace and indentation are inconsistent. That makes your code a little difficult to read.

---

Use of access modifiers

It is unnecessary to specify private as an access modifier at the top of your class as they are private by default. However it is common to list public members and functions first because anyone (including you) reading your code is going to be more interested in the interface for the class.

---

Omit return 0;

main() will return on its own at the end of its execution. It is considered standard practice not to return at the end of main() and the compiler will generate the appropriate return as needed.

---

Let RAII do its job

{
ofstream myfile;
// Definition on File Name: "MeinTest.wav"
myfile.open (FileName.c_str(), ios::out | ios::binary);
... // other code
// File is writen so close
myfile.close();
}

The very next line after you close your file explicitly the scope ends. Your file will close itself on going out of scope and perform all necessary operations (like flushing and memory cleanup) A huge part of writing quality C++ classes is proper RAII cleanup so that you and I don't need to clean up after ourselves. Let your objects go out of scope instead of doing it explicity.

---

Only write meaningful comments

Most of your comments (like those in the previous example) only serve to state what is already clearly stated by the code itself. As a rule of thumb you should always assume that anyone reading your code is capable of reading code. myFile.close() should be readily apparent as to what it does. Only comment when something isn't easy to understand. (also consider refactoring or renaming when you find the need for a comment.)

---

Don't use All-cap variable names

char RIFF[] = "RIFF";
char WAVE[] = "WAVE";

All cap variables are conventionally reserved for macros. prefer instead:

char Riff[] = "RIFF";
char Wave[] = "WAVE";

or

char riff[] = "RIFF";
char wave[] = "WAVE";

Incidentally I am a bit uncertain about the current use case but I am unsure why you are using C-style char array[] (and not char* array[]) or a C++ string? is there a reason you can't use an std::string?

---

Use C++ casts

unsigned char a = (unsigned char)i1;
unsigned char b = (unsigned char)i2;
unsigned char c = (unsigned char)i3;
unsigned char d = (unsigned char)i4;

C++ static_cast<> is safer than C-style casts as it is checked at compile time. Also you should use C++ conventions when writing C++ code.

---

Concatenate your operations

long i = x;
int i1 = i % 256;
i = i - i1;
i = i / 256;
int i2 = i % 256;
i = i - i2;
i = i / 256;
int i3 = i % 256;
i = i - i1;
i = i / 256;
int i4 = i;
unsigned char a = (unsigned char)i1;
unsigned char b = (unsigned char)i2;
unsigned char c = (unsigned char)i3;
unsigned char d = (unsigned char)i4;

can be re-written:

int x1 = x % 256;
x = (x - x1) / 256;
int x2 = x % 256;
x = (x - x2) / 256;
int x3 = x % 256;
x = (x - x1) / 256; // did you mean to use x3 here? (well i3 in your code)
int x4 = x;
unsigned char a = static_cast<unsigned char>(x1);
unsigned char b = static_cast<unsigned char>(x2);
unsigned char c = static_cast<unsigned char>(x3);
unsigned char d = static_cast<unsigned char>(x4);

notice it was also unnecessary to copy x into a second variable. you can use it directly (it's already a copy)

---

Omit unused headers

You aren't using <iostream>. to that end you can omit it.

---

Compare values of the same type

for(float m = 0; m <= DataLeng; m++)

That line threw me off for a moment. I've never seen a float as a comparison within a for loop. Especially since DataLeng is an int. Your compiler should have warned you about this. To that end. . . :

---

Use max compiler warnings

Make sure you have max compiler warnings on. Consider warnings as error too. This will catch a lot of issues for you and help you clean your code before you even get to us. use -W4 on VS or -Wall on gcc/clang and -Werror. (If you use a different compiler you will have to look up how to better utilize warnings.)

---

Prefer prefix over postfix

Answer 2

I see a number of things that may help you improve your program.

Fix the bug

Right now, the program purports to generate a sine wave, but when I first played it, it was clear that it contained all kinds of harmonics. When I examined the waveform using Audacity, the problem was very clear. Here's the waveform:

badSine

The problem is extreme clipping. In particular, the software is adding an offset which is neither needed nor wanted:

value = (result * 32767 ) + 32767;

If we modify that line to eliminate the offset:

value = result * 32767;

The resulting waveform is correct:

goodSine

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use <cmath> instead of <math.h>

The difference between the two forms is that the former defines things within the std:: namespace versus into the global namespace. Language lawyers have lots of fun with this, but for daily use I'd recommend using <cmath>. See this SO question for details.

Use the appropriate data type

It seems that your code is making the assumption that a long is 4 bytes and an int is 2 bytes. That's not true on my machine or across different compilers. If you need specifically 2-byte or 4-byte quantities (which this code does), it's better to use the specific size types uint32_t and uint16_t as defined in <cstdint>.

Simplify your code

The longWrite code has a number of things I'd suggest changing. I've already mentioned one, but here's another. Because the parameter x is passed by value, you are free to do what you want with the copy. That simplifies the code a lot:

void longWrite(uint32_t x, std::ofstream &myfile)
{
 myfile << static_cast<uint8_t>(x & 0xff);
 x >>= 8;
 myfile << static_cast<uint8_t>(x & 0xff);
 x >>= 8;
 myfile << static_cast<uint8_t>(x & 0xff);
 x >>= 8;
 myfile << static_cast<uint8_t>(x & 0xff);
}

Use an object where appropriate

Unless the environment in which this code is running is extremely constricted in terms ofmemory space, (an embedded system, for example), it is probably better to use an object (either a class or struct, to represent the output file format. For example, see this C code.

Use const or constexpr where practical

The code uses a number of named constants, which could and should be declared as const or constexpr. For example, the various text constants could be declared this way:

 constexpr char RIFF[] = "RIFF";
 myfile << RIFF;

However, as mentioned before, as a practical matter, this would be part of a structure instead.

Eliminate "magic numbers"

Instead of hard-coding the constants 220000 and 176000 in the code, it would be better to use a const or constexpr and name them.

Initialize variables at declaration

The code currently contains these lines:

ofstream myfile;
myfile.open (FileName.c_str(), ios::out | ios::binary);

These could be condensed much more succintly and clearly as this:

 std::ofstream myfile{FileName};

Note that a std::ofstream can take a std::string filename and that its default flags are already the ones you had specified manually.

Use the passed parameters

At the moment the code takes but then ignores the passed Duratione value using a hardcoded Time variable instead.

Think of the user

The use of a template here is rather suspect. It will require essentially a duplicate compiled version for every function the user might want. A more flexible approach would be to pass the function as a parameter. That would remove the template and change the declaration of the WaveFileWrite function to this:

WaveFileWriter(const std::string& FileName, 
 float Frequency, 
 float Duration, 
 float (*Funktion)(float));

However, there's little reason to store anything in an object in this case, so I'd be inclined to turn it into a function or function object instead. I'd also allow for passing in diferent functions for left and right channel. Here's the function I wrote:

bool WaveFileWriter(const std::string& FileName, float Frequency, float Duration, float (*left)(float), float (*right)(float)) {
 std::ofstream myfile{FileName};
 constexpr uint16_t fullscale{32767};
 const double twoPiF{M_PI*2*Frequency};
 Header hdr{Duration};
 myfile << hdr;
 for (unsigned t = 0; t <= hdr.subchunk2Size; ++t)
 {
 integerWrite(fullscale * left(twoPiF * t / hdr.sampleRate), myfile);
 integerWrite(fullscale * right(twoPiF * t / hdr.sampleRate), myfile);
 }
 return !myfile.fail();
}

Here's an example of use:

float decaybell(float t) {
 return std::exp(-0.001*t) * std::cos(t);
}
float harmonic(float t) {
 return 2.0/M_PI * (std::cos(t) + std::cos(3*t)/3 + std::cos(5*t)/5);
}
float square(float t) {
 return 2.0/M_PI * (std::sin(t) + std::sin(3*t)/3 + std::sin(5*t)/5);
}
float silent(float t) {
 return 0;
}
int main()
{
 WaveFileWriter("MyTest.wav", 440, 5, decaybell, square);
}

Answer 3

SinFunction and the templating of WaveFileWriter are overkill. All we need is a simple std::function<float(float)>.

I'd argue that the knowledge of frequency doesn't belong in the writer - it's a property of the signal we're generating, not of the file format. This will be important when you want to produce multiple notes within the file, for example.

Why are we duplicating the signal to two channels, rather than writing a monaural file that's half the size? Even if there's some valid reason to write a stereo file, there's no need to compute the contents twice, unless you expect users to pass a non-pure function.

When dealing with file formats with fixed-width integer types, it's natural to use C++'s fixed-width integer types with them - I suggest writing a wrapper for a standard stream that implements operator<<(std::uint16_t) and similar instead of the longWrite() and friends here that write data that might be wider or narrower than the type that's passed. Also, take more care using % with signed types.

Why use the signature for main() that accepts arguments, when we then ignore the arguments and use a hard-coded filename instead? Let's allow the user to pass the filename as argument; that make the program much more flexible (even better if we default to writing to standard output, so we can use it to feed a command pipeline).