Play some sine waves with SDL2











up vote
11
down vote

favorite












Runs smoothly, however valgrind is showing "possibly lost: 544 bytes in 2 blocks".
This is my first time doing a lot of things here so I might be making multiple mistakes.



Please let me know if anything needs to be fixed, or if I should just do something completely different for whatever reason.



/* protosynth
*
* Throughout the source code, "frequency" refers to a Hz value,
* and "pitch" refers to a numeric musical note value with 0 representing C0, 12 for C1, etc..
*
* compiled with:
* gcc -Wall protosynth.c -o protosynth `sdl2-config --cflags --libs` -lm
*/

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include "SDL.h"

const double ChromaticRatio = 1.059463094359295264562;
const double Tao = 6.283185307179586476925;

Uint32 sampleRate = 48000;
Uint32 frameRate = 60;
Uint32 floatStreamLength = 1024;// must be a power of two, decrease to allow for a lower syncCompensationFactor to allow for lower latency, increase to reduce risk of underrun
Uint32 samplesPerFrame; // = sampleRate/frameRate;
Uint32 msPerFrame; // = 1000/frameRate;
double practicallySilent = 0.001;

Uint32 audioBufferLength = 48000;// must be a multiple of samplesPerFrame (auto adjusted upwards if not)
float *audioBuffer;

SDL_atomic_t audioCallbackLeftOff;
Sint32 audioMainLeftOff;
Uint8 audioMainAccumulator;

SDL_AudioDeviceID AudioDevice;
SDL_AudioSpec audioSpec;

SDL_Event event;
SDL_bool running = SDL_TRUE;


typedef struct {
float *waveform;
Uint32 waveformLength;
double volume; // multiplied
double pan; // 0 to 1: all the way left to all the way right
double frequency; // Hz
double phase; // 0 to 1
} voice;

/* _
| |
___ _ __ ___ __ _| | __
/ __| '_ / _ / _` | |/ /
__ |_) | __/ (_| | <
|___/ .__/ ___|__,_|_|_
| |
|_|
*/
void speak(voice *v) {
float sample;
Uint32 sourceIndex;
double phaseIncrement = v->frequency/sampleRate;
Uint32 i;
if (v->volume > practicallySilent) {
for (i=0; (i+1)<samplesPerFrame; i+=2) {

v->phase += phaseIncrement;
if (v->phase > 1) v->phase -= 1;

sourceIndex = v->phase*v->waveformLength;
sample = v->waveform[sourceIndex]*v->volume;

audioBuffer[audioMainLeftOff+i] += sample*(1-v->pan); //left channel
audioBuffer[audioMainLeftOff+i+1] += sample*v->pan; //right channel
}
}
else {
for (i=0; i<samplesPerFrame; i+=1)
audioBuffer[audioMainLeftOff+i] = 0;
}
audioMainAccumulator++;
}

double getFrequency(double pitch) {
return pow(ChromaticRatio, pitch-57)*440;
}
int getWaveformLength(double pitch) {
return sampleRate / getFrequency(pitch)+0.5f;
}

void buildSineWave(float *data, Uint32 length) {
Uint32 i;
for (i=0; i < length; i++)
data[i] = sin( i*(Tao/length) );
}

void logSpec(SDL_AudioSpec *as) {
printf(
" freq______%5dn"
" format____%5dn"
" channels__%5dn"
" silence___%5dn"
" samples___%5dn"
" size______%5dnn",
(int) as->freq,
(int) as->format,
(int) as->channels,
(int) as->silence,
(int) as->samples,
(int) as->size
);
}

void logVoice(voice *v) {
printf(
" waveformLength__%dn"
" volume__________%fn"
" pan_____________%fn"
" frequency_______%fn"
" phase___________%fn",
v->waveformLength,
v->volume,
v->pan,
v->frequency,
v->phase
);
}

void logWavedata(float *floatStream, Uint32 floatStreamLength, Uint32 increment) {
printf("nnwaveform data:nn");
Uint32 i=0;
for (i=0; i<floatStreamLength; i+=increment)
printf("%4d:%2.16fn", i, floatStream[i]);
printf("nn");
}

/* _ _ _____ _ _ _ _
| (_) / ____| | | | | | |
__ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
/ _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
| (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
__,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
*/
void audioCallback(void *unused, Uint8 *byteStream, int byteStreamLength) {
float* floatStream = (float*) byteStream;

Sint32 localAudioCallbackLeftOff = SDL_AtomicGet(&audioCallbackLeftOff);

Uint32 i;
for (i=0; i<floatStreamLength; i++) {

floatStream[i] = audioBuffer[localAudioCallbackLeftOff];

localAudioCallbackLeftOff++;
if ( localAudioCallbackLeftOff == audioBufferLength )
localAudioCallbackLeftOff = 0;
}
//printf("localAudioCallbackLeftOff__%5dn", localAudioCallbackLeftOff);

SDL_AtomicSet(&audioCallbackLeftOff, localAudioCallbackLeftOff);
}

/*_ _ _
(_) (_) |
_ _ __ _| |_
| | '_ | | __|
| | | | | | |_
|_|_| |_|_|__|
*/
int init() {
SDL_Init(SDL_INIT_AUDIO | SDL_INIT_TIMER);

SDL_AudioSpec want;

SDL_zero(want);// btw, I have no idea what this is...

want.freq = sampleRate;
want.format = AUDIO_F32;
want.channels = 2;
want.samples = floatStreamLength;
want.callback = audioCallback;

AudioDevice = SDL_OpenAudioDevice(NULL, 0, &want, &audioSpec, SDL_AUDIO_ALLOW_FORMAT_CHANGE);

if (AudioDevice == 0) {
printf("nFailed to open audio: %sn", SDL_GetError());
return 1;
}

printf("want:n");
logSpec(&want);
printf("audioSpec:n");
logSpec(&audioSpec);

if (audioSpec.format != want.format) {
printf("nCouldn't get Float32 audio format.n");
return 2;
}

sampleRate = audioSpec.freq;
floatStreamLength = audioSpec.size/4;
samplesPerFrame = sampleRate/frameRate;
msPerFrame = 1000/frameRate;
audioMainLeftOff = samplesPerFrame*8;
SDL_AtomicSet(&audioCallbackLeftOff, 0);

if (audioBufferLength % samplesPerFrame)
audioBufferLength += samplesPerFrame-(audioBufferLength % samplesPerFrame);
audioBuffer = malloc( sizeof(float)*audioBufferLength );

return 0;
}
int onExit() {
SDL_CloseAudioDevice(AudioDevice);
//free(audioBuffer);//not necessary?
SDL_Quit();
return 0;
}

/* _
(_)
_ __ ___ __ _ _ _ __
| '_ ` _ / _` | | '_
| | | | | | (_| | | | | |
|_| |_| |_|__,_|_|_| |_|
*/
int main(int argc, char *argv) {

float syncCompensationFactor = 0.0016;// decrease to reduce risk of collision, increase to lower latency
Sint32 mainAudioLead;
Uint32 i;

voice testVoiceA;
voice testVoiceB;
voice testVoiceC;
testVoiceA.volume = 1;
testVoiceB.volume = 1;
testVoiceC.volume = 1;
testVoiceA.pan = 0.5;
testVoiceB.pan = 0;
testVoiceC.pan = 1;
testVoiceA.phase = 0;
testVoiceB.phase = 0;
testVoiceC.phase = 0;
testVoiceA.frequency = getFrequency(45);// A3
testVoiceB.frequency = getFrequency(49);// C#4
testVoiceC.frequency = getFrequency(52);// E4
Uint16 C0waveformLength = getWaveformLength(0);
testVoiceA.waveformLength = C0waveformLength;
testVoiceB.waveformLength = C0waveformLength;
testVoiceC.waveformLength = C0waveformLength;
float sineWave[C0waveformLength];
buildSineWave(sineWave, C0waveformLength);
testVoiceA.waveform = sineWave;
testVoiceB.waveform = sineWave;
testVoiceC.waveform = sineWave;

//logVoice(&testVoiceA);
//logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);

if ( init() ) return 1;

SDL_Delay(42);// let the tubes warm up

SDL_PauseAudioDevice(AudioDevice, 0);// unpause audio.

while (running) {
while( SDL_PollEvent( &event ) != 0 ) {
if( event.type == SDL_QUIT ) {
running = SDL_FALSE;
}
}
for (i=0; i<samplesPerFrame; i++) audioBuffer[audioMainLeftOff+i] = 0;

//printf("audioMainLeftOff___________%5dn", audioMainLeftOff);

speak(&testVoiceA);
speak(&testVoiceB);
speak(&testVoiceC);

if (audioMainAccumulator > 1) {
for (i=0; i<samplesPerFrame; i++) {
audioBuffer[audioMainLeftOff+i] /= audioMainAccumulator;
}
}
audioMainAccumulator = 0;

audioMainLeftOff += samplesPerFrame;
if (audioMainLeftOff == audioBufferLength) audioMainLeftOff = 0;

mainAudioLead = audioMainLeftOff - SDL_AtomicGet(&audioCallbackLeftOff);
if (mainAudioLead < 0) mainAudioLead += audioBufferLength;
//printf("mainAudioLead:%5dn", mainAudioLead);
if (mainAudioLead < floatStreamLength) printf("An audio collision may have occured!n");
SDL_Delay( mainAudioLead*syncCompensationFactor );
}
onExit();
return 0;
}


EDIT:



After doing some more research on my valgrind errors, I came to the conclusion that there wasn't much I could do other than suppress them. Here is my suppression file:



{
<from SDL_TimerInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:SDL_SYS_CreateThread
fun:SDL_CreateThread
fun:SDL_TimerInit
fun:SDL_InitSubSystem
fun:init
fun:main
}
{
<from SDL_AudioInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:pa_thread_new
fun:pa_threaded_mainloop_start
fun:pa_simple_new
fun:PULSEAUDIO_Init
fun:SDL_AudioInit
fun:SDL_InitSubSystem
fun:init
fun:main
}









share|improve this question




















  • 3




    ASCII art comments? That just repeat the names of the functions? Argh, my eyes...
    – Ant
    Feb 6 '14 at 18:11












  • haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them.
    – Duovarious
    Feb 6 '14 at 18:58















up vote
11
down vote

favorite












Runs smoothly, however valgrind is showing "possibly lost: 544 bytes in 2 blocks".
This is my first time doing a lot of things here so I might be making multiple mistakes.



Please let me know if anything needs to be fixed, or if I should just do something completely different for whatever reason.



/* protosynth
*
* Throughout the source code, "frequency" refers to a Hz value,
* and "pitch" refers to a numeric musical note value with 0 representing C0, 12 for C1, etc..
*
* compiled with:
* gcc -Wall protosynth.c -o protosynth `sdl2-config --cflags --libs` -lm
*/

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include "SDL.h"

const double ChromaticRatio = 1.059463094359295264562;
const double Tao = 6.283185307179586476925;

Uint32 sampleRate = 48000;
Uint32 frameRate = 60;
Uint32 floatStreamLength = 1024;// must be a power of two, decrease to allow for a lower syncCompensationFactor to allow for lower latency, increase to reduce risk of underrun
Uint32 samplesPerFrame; // = sampleRate/frameRate;
Uint32 msPerFrame; // = 1000/frameRate;
double practicallySilent = 0.001;

Uint32 audioBufferLength = 48000;// must be a multiple of samplesPerFrame (auto adjusted upwards if not)
float *audioBuffer;

SDL_atomic_t audioCallbackLeftOff;
Sint32 audioMainLeftOff;
Uint8 audioMainAccumulator;

SDL_AudioDeviceID AudioDevice;
SDL_AudioSpec audioSpec;

SDL_Event event;
SDL_bool running = SDL_TRUE;


typedef struct {
float *waveform;
Uint32 waveformLength;
double volume; // multiplied
double pan; // 0 to 1: all the way left to all the way right
double frequency; // Hz
double phase; // 0 to 1
} voice;

/* _
| |
___ _ __ ___ __ _| | __
/ __| '_ / _ / _` | |/ /
__ |_) | __/ (_| | <
|___/ .__/ ___|__,_|_|_
| |
|_|
*/
void speak(voice *v) {
float sample;
Uint32 sourceIndex;
double phaseIncrement = v->frequency/sampleRate;
Uint32 i;
if (v->volume > practicallySilent) {
for (i=0; (i+1)<samplesPerFrame; i+=2) {

v->phase += phaseIncrement;
if (v->phase > 1) v->phase -= 1;

sourceIndex = v->phase*v->waveformLength;
sample = v->waveform[sourceIndex]*v->volume;

audioBuffer[audioMainLeftOff+i] += sample*(1-v->pan); //left channel
audioBuffer[audioMainLeftOff+i+1] += sample*v->pan; //right channel
}
}
else {
for (i=0; i<samplesPerFrame; i+=1)
audioBuffer[audioMainLeftOff+i] = 0;
}
audioMainAccumulator++;
}

double getFrequency(double pitch) {
return pow(ChromaticRatio, pitch-57)*440;
}
int getWaveformLength(double pitch) {
return sampleRate / getFrequency(pitch)+0.5f;
}

void buildSineWave(float *data, Uint32 length) {
Uint32 i;
for (i=0; i < length; i++)
data[i] = sin( i*(Tao/length) );
}

void logSpec(SDL_AudioSpec *as) {
printf(
" freq______%5dn"
" format____%5dn"
" channels__%5dn"
" silence___%5dn"
" samples___%5dn"
" size______%5dnn",
(int) as->freq,
(int) as->format,
(int) as->channels,
(int) as->silence,
(int) as->samples,
(int) as->size
);
}

void logVoice(voice *v) {
printf(
" waveformLength__%dn"
" volume__________%fn"
" pan_____________%fn"
" frequency_______%fn"
" phase___________%fn",
v->waveformLength,
v->volume,
v->pan,
v->frequency,
v->phase
);
}

void logWavedata(float *floatStream, Uint32 floatStreamLength, Uint32 increment) {
printf("nnwaveform data:nn");
Uint32 i=0;
for (i=0; i<floatStreamLength; i+=increment)
printf("%4d:%2.16fn", i, floatStream[i]);
printf("nn");
}

/* _ _ _____ _ _ _ _
| (_) / ____| | | | | | |
__ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
/ _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
| (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
__,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
*/
void audioCallback(void *unused, Uint8 *byteStream, int byteStreamLength) {
float* floatStream = (float*) byteStream;

Sint32 localAudioCallbackLeftOff = SDL_AtomicGet(&audioCallbackLeftOff);

Uint32 i;
for (i=0; i<floatStreamLength; i++) {

floatStream[i] = audioBuffer[localAudioCallbackLeftOff];

localAudioCallbackLeftOff++;
if ( localAudioCallbackLeftOff == audioBufferLength )
localAudioCallbackLeftOff = 0;
}
//printf("localAudioCallbackLeftOff__%5dn", localAudioCallbackLeftOff);

SDL_AtomicSet(&audioCallbackLeftOff, localAudioCallbackLeftOff);
}

/*_ _ _
(_) (_) |
_ _ __ _| |_
| | '_ | | __|
| | | | | | |_
|_|_| |_|_|__|
*/
int init() {
SDL_Init(SDL_INIT_AUDIO | SDL_INIT_TIMER);

SDL_AudioSpec want;

SDL_zero(want);// btw, I have no idea what this is...

want.freq = sampleRate;
want.format = AUDIO_F32;
want.channels = 2;
want.samples = floatStreamLength;
want.callback = audioCallback;

AudioDevice = SDL_OpenAudioDevice(NULL, 0, &want, &audioSpec, SDL_AUDIO_ALLOW_FORMAT_CHANGE);

if (AudioDevice == 0) {
printf("nFailed to open audio: %sn", SDL_GetError());
return 1;
}

printf("want:n");
logSpec(&want);
printf("audioSpec:n");
logSpec(&audioSpec);

if (audioSpec.format != want.format) {
printf("nCouldn't get Float32 audio format.n");
return 2;
}

sampleRate = audioSpec.freq;
floatStreamLength = audioSpec.size/4;
samplesPerFrame = sampleRate/frameRate;
msPerFrame = 1000/frameRate;
audioMainLeftOff = samplesPerFrame*8;
SDL_AtomicSet(&audioCallbackLeftOff, 0);

if (audioBufferLength % samplesPerFrame)
audioBufferLength += samplesPerFrame-(audioBufferLength % samplesPerFrame);
audioBuffer = malloc( sizeof(float)*audioBufferLength );

return 0;
}
int onExit() {
SDL_CloseAudioDevice(AudioDevice);
//free(audioBuffer);//not necessary?
SDL_Quit();
return 0;
}

/* _
(_)
_ __ ___ __ _ _ _ __
| '_ ` _ / _` | | '_
| | | | | | (_| | | | | |
|_| |_| |_|__,_|_|_| |_|
*/
int main(int argc, char *argv) {

float syncCompensationFactor = 0.0016;// decrease to reduce risk of collision, increase to lower latency
Sint32 mainAudioLead;
Uint32 i;

voice testVoiceA;
voice testVoiceB;
voice testVoiceC;
testVoiceA.volume = 1;
testVoiceB.volume = 1;
testVoiceC.volume = 1;
testVoiceA.pan = 0.5;
testVoiceB.pan = 0;
testVoiceC.pan = 1;
testVoiceA.phase = 0;
testVoiceB.phase = 0;
testVoiceC.phase = 0;
testVoiceA.frequency = getFrequency(45);// A3
testVoiceB.frequency = getFrequency(49);// C#4
testVoiceC.frequency = getFrequency(52);// E4
Uint16 C0waveformLength = getWaveformLength(0);
testVoiceA.waveformLength = C0waveformLength;
testVoiceB.waveformLength = C0waveformLength;
testVoiceC.waveformLength = C0waveformLength;
float sineWave[C0waveformLength];
buildSineWave(sineWave, C0waveformLength);
testVoiceA.waveform = sineWave;
testVoiceB.waveform = sineWave;
testVoiceC.waveform = sineWave;

//logVoice(&testVoiceA);
//logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);

if ( init() ) return 1;

SDL_Delay(42);// let the tubes warm up

SDL_PauseAudioDevice(AudioDevice, 0);// unpause audio.

while (running) {
while( SDL_PollEvent( &event ) != 0 ) {
if( event.type == SDL_QUIT ) {
running = SDL_FALSE;
}
}
for (i=0; i<samplesPerFrame; i++) audioBuffer[audioMainLeftOff+i] = 0;

//printf("audioMainLeftOff___________%5dn", audioMainLeftOff);

speak(&testVoiceA);
speak(&testVoiceB);
speak(&testVoiceC);

if (audioMainAccumulator > 1) {
for (i=0; i<samplesPerFrame; i++) {
audioBuffer[audioMainLeftOff+i] /= audioMainAccumulator;
}
}
audioMainAccumulator = 0;

audioMainLeftOff += samplesPerFrame;
if (audioMainLeftOff == audioBufferLength) audioMainLeftOff = 0;

mainAudioLead = audioMainLeftOff - SDL_AtomicGet(&audioCallbackLeftOff);
if (mainAudioLead < 0) mainAudioLead += audioBufferLength;
//printf("mainAudioLead:%5dn", mainAudioLead);
if (mainAudioLead < floatStreamLength) printf("An audio collision may have occured!n");
SDL_Delay( mainAudioLead*syncCompensationFactor );
}
onExit();
return 0;
}


EDIT:



After doing some more research on my valgrind errors, I came to the conclusion that there wasn't much I could do other than suppress them. Here is my suppression file:



{
<from SDL_TimerInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:SDL_SYS_CreateThread
fun:SDL_CreateThread
fun:SDL_TimerInit
fun:SDL_InitSubSystem
fun:init
fun:main
}
{
<from SDL_AudioInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:pa_thread_new
fun:pa_threaded_mainloop_start
fun:pa_simple_new
fun:PULSEAUDIO_Init
fun:SDL_AudioInit
fun:SDL_InitSubSystem
fun:init
fun:main
}









share|improve this question




















  • 3




    ASCII art comments? That just repeat the names of the functions? Argh, my eyes...
    – Ant
    Feb 6 '14 at 18:11












  • haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them.
    – Duovarious
    Feb 6 '14 at 18:58













up vote
11
down vote

favorite









up vote
11
down vote

favorite











Runs smoothly, however valgrind is showing "possibly lost: 544 bytes in 2 blocks".
This is my first time doing a lot of things here so I might be making multiple mistakes.



Please let me know if anything needs to be fixed, or if I should just do something completely different for whatever reason.



/* protosynth
*
* Throughout the source code, "frequency" refers to a Hz value,
* and "pitch" refers to a numeric musical note value with 0 representing C0, 12 for C1, etc..
*
* compiled with:
* gcc -Wall protosynth.c -o protosynth `sdl2-config --cflags --libs` -lm
*/

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include "SDL.h"

const double ChromaticRatio = 1.059463094359295264562;
const double Tao = 6.283185307179586476925;

Uint32 sampleRate = 48000;
Uint32 frameRate = 60;
Uint32 floatStreamLength = 1024;// must be a power of two, decrease to allow for a lower syncCompensationFactor to allow for lower latency, increase to reduce risk of underrun
Uint32 samplesPerFrame; // = sampleRate/frameRate;
Uint32 msPerFrame; // = 1000/frameRate;
double practicallySilent = 0.001;

Uint32 audioBufferLength = 48000;// must be a multiple of samplesPerFrame (auto adjusted upwards if not)
float *audioBuffer;

SDL_atomic_t audioCallbackLeftOff;
Sint32 audioMainLeftOff;
Uint8 audioMainAccumulator;

SDL_AudioDeviceID AudioDevice;
SDL_AudioSpec audioSpec;

SDL_Event event;
SDL_bool running = SDL_TRUE;


typedef struct {
float *waveform;
Uint32 waveformLength;
double volume; // multiplied
double pan; // 0 to 1: all the way left to all the way right
double frequency; // Hz
double phase; // 0 to 1
} voice;

/* _
| |
___ _ __ ___ __ _| | __
/ __| '_ / _ / _` | |/ /
__ |_) | __/ (_| | <
|___/ .__/ ___|__,_|_|_
| |
|_|
*/
void speak(voice *v) {
float sample;
Uint32 sourceIndex;
double phaseIncrement = v->frequency/sampleRate;
Uint32 i;
if (v->volume > practicallySilent) {
for (i=0; (i+1)<samplesPerFrame; i+=2) {

v->phase += phaseIncrement;
if (v->phase > 1) v->phase -= 1;

sourceIndex = v->phase*v->waveformLength;
sample = v->waveform[sourceIndex]*v->volume;

audioBuffer[audioMainLeftOff+i] += sample*(1-v->pan); //left channel
audioBuffer[audioMainLeftOff+i+1] += sample*v->pan; //right channel
}
}
else {
for (i=0; i<samplesPerFrame; i+=1)
audioBuffer[audioMainLeftOff+i] = 0;
}
audioMainAccumulator++;
}

double getFrequency(double pitch) {
return pow(ChromaticRatio, pitch-57)*440;
}
int getWaveformLength(double pitch) {
return sampleRate / getFrequency(pitch)+0.5f;
}

void buildSineWave(float *data, Uint32 length) {
Uint32 i;
for (i=0; i < length; i++)
data[i] = sin( i*(Tao/length) );
}

void logSpec(SDL_AudioSpec *as) {
printf(
" freq______%5dn"
" format____%5dn"
" channels__%5dn"
" silence___%5dn"
" samples___%5dn"
" size______%5dnn",
(int) as->freq,
(int) as->format,
(int) as->channels,
(int) as->silence,
(int) as->samples,
(int) as->size
);
}

void logVoice(voice *v) {
printf(
" waveformLength__%dn"
" volume__________%fn"
" pan_____________%fn"
" frequency_______%fn"
" phase___________%fn",
v->waveformLength,
v->volume,
v->pan,
v->frequency,
v->phase
);
}

void logWavedata(float *floatStream, Uint32 floatStreamLength, Uint32 increment) {
printf("nnwaveform data:nn");
Uint32 i=0;
for (i=0; i<floatStreamLength; i+=increment)
printf("%4d:%2.16fn", i, floatStream[i]);
printf("nn");
}

/* _ _ _____ _ _ _ _
| (_) / ____| | | | | | |
__ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
/ _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
| (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
__,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
*/
void audioCallback(void *unused, Uint8 *byteStream, int byteStreamLength) {
float* floatStream = (float*) byteStream;

Sint32 localAudioCallbackLeftOff = SDL_AtomicGet(&audioCallbackLeftOff);

Uint32 i;
for (i=0; i<floatStreamLength; i++) {

floatStream[i] = audioBuffer[localAudioCallbackLeftOff];

localAudioCallbackLeftOff++;
if ( localAudioCallbackLeftOff == audioBufferLength )
localAudioCallbackLeftOff = 0;
}
//printf("localAudioCallbackLeftOff__%5dn", localAudioCallbackLeftOff);

SDL_AtomicSet(&audioCallbackLeftOff, localAudioCallbackLeftOff);
}

/*_ _ _
(_) (_) |
_ _ __ _| |_
| | '_ | | __|
| | | | | | |_
|_|_| |_|_|__|
*/
int init() {
SDL_Init(SDL_INIT_AUDIO | SDL_INIT_TIMER);

SDL_AudioSpec want;

SDL_zero(want);// btw, I have no idea what this is...

want.freq = sampleRate;
want.format = AUDIO_F32;
want.channels = 2;
want.samples = floatStreamLength;
want.callback = audioCallback;

AudioDevice = SDL_OpenAudioDevice(NULL, 0, &want, &audioSpec, SDL_AUDIO_ALLOW_FORMAT_CHANGE);

if (AudioDevice == 0) {
printf("nFailed to open audio: %sn", SDL_GetError());
return 1;
}

printf("want:n");
logSpec(&want);
printf("audioSpec:n");
logSpec(&audioSpec);

if (audioSpec.format != want.format) {
printf("nCouldn't get Float32 audio format.n");
return 2;
}

sampleRate = audioSpec.freq;
floatStreamLength = audioSpec.size/4;
samplesPerFrame = sampleRate/frameRate;
msPerFrame = 1000/frameRate;
audioMainLeftOff = samplesPerFrame*8;
SDL_AtomicSet(&audioCallbackLeftOff, 0);

if (audioBufferLength % samplesPerFrame)
audioBufferLength += samplesPerFrame-(audioBufferLength % samplesPerFrame);
audioBuffer = malloc( sizeof(float)*audioBufferLength );

return 0;
}
int onExit() {
SDL_CloseAudioDevice(AudioDevice);
//free(audioBuffer);//not necessary?
SDL_Quit();
return 0;
}

/* _
(_)
_ __ ___ __ _ _ _ __
| '_ ` _ / _` | | '_
| | | | | | (_| | | | | |
|_| |_| |_|__,_|_|_| |_|
*/
int main(int argc, char *argv) {

float syncCompensationFactor = 0.0016;// decrease to reduce risk of collision, increase to lower latency
Sint32 mainAudioLead;
Uint32 i;

voice testVoiceA;
voice testVoiceB;
voice testVoiceC;
testVoiceA.volume = 1;
testVoiceB.volume = 1;
testVoiceC.volume = 1;
testVoiceA.pan = 0.5;
testVoiceB.pan = 0;
testVoiceC.pan = 1;
testVoiceA.phase = 0;
testVoiceB.phase = 0;
testVoiceC.phase = 0;
testVoiceA.frequency = getFrequency(45);// A3
testVoiceB.frequency = getFrequency(49);// C#4
testVoiceC.frequency = getFrequency(52);// E4
Uint16 C0waveformLength = getWaveformLength(0);
testVoiceA.waveformLength = C0waveformLength;
testVoiceB.waveformLength = C0waveformLength;
testVoiceC.waveformLength = C0waveformLength;
float sineWave[C0waveformLength];
buildSineWave(sineWave, C0waveformLength);
testVoiceA.waveform = sineWave;
testVoiceB.waveform = sineWave;
testVoiceC.waveform = sineWave;

//logVoice(&testVoiceA);
//logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);

if ( init() ) return 1;

SDL_Delay(42);// let the tubes warm up

SDL_PauseAudioDevice(AudioDevice, 0);// unpause audio.

while (running) {
while( SDL_PollEvent( &event ) != 0 ) {
if( event.type == SDL_QUIT ) {
running = SDL_FALSE;
}
}
for (i=0; i<samplesPerFrame; i++) audioBuffer[audioMainLeftOff+i] = 0;

//printf("audioMainLeftOff___________%5dn", audioMainLeftOff);

speak(&testVoiceA);
speak(&testVoiceB);
speak(&testVoiceC);

if (audioMainAccumulator > 1) {
for (i=0; i<samplesPerFrame; i++) {
audioBuffer[audioMainLeftOff+i] /= audioMainAccumulator;
}
}
audioMainAccumulator = 0;

audioMainLeftOff += samplesPerFrame;
if (audioMainLeftOff == audioBufferLength) audioMainLeftOff = 0;

mainAudioLead = audioMainLeftOff - SDL_AtomicGet(&audioCallbackLeftOff);
if (mainAudioLead < 0) mainAudioLead += audioBufferLength;
//printf("mainAudioLead:%5dn", mainAudioLead);
if (mainAudioLead < floatStreamLength) printf("An audio collision may have occured!n");
SDL_Delay( mainAudioLead*syncCompensationFactor );
}
onExit();
return 0;
}


EDIT:



After doing some more research on my valgrind errors, I came to the conclusion that there wasn't much I could do other than suppress them. Here is my suppression file:



{
<from SDL_TimerInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:SDL_SYS_CreateThread
fun:SDL_CreateThread
fun:SDL_TimerInit
fun:SDL_InitSubSystem
fun:init
fun:main
}
{
<from SDL_AudioInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:pa_thread_new
fun:pa_threaded_mainloop_start
fun:pa_simple_new
fun:PULSEAUDIO_Init
fun:SDL_AudioInit
fun:SDL_InitSubSystem
fun:init
fun:main
}









share|improve this question















Runs smoothly, however valgrind is showing "possibly lost: 544 bytes in 2 blocks".
This is my first time doing a lot of things here so I might be making multiple mistakes.



Please let me know if anything needs to be fixed, or if I should just do something completely different for whatever reason.



/* protosynth
*
* Throughout the source code, "frequency" refers to a Hz value,
* and "pitch" refers to a numeric musical note value with 0 representing C0, 12 for C1, etc..
*
* compiled with:
* gcc -Wall protosynth.c -o protosynth `sdl2-config --cflags --libs` -lm
*/

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include "SDL.h"

const double ChromaticRatio = 1.059463094359295264562;
const double Tao = 6.283185307179586476925;

Uint32 sampleRate = 48000;
Uint32 frameRate = 60;
Uint32 floatStreamLength = 1024;// must be a power of two, decrease to allow for a lower syncCompensationFactor to allow for lower latency, increase to reduce risk of underrun
Uint32 samplesPerFrame; // = sampleRate/frameRate;
Uint32 msPerFrame; // = 1000/frameRate;
double practicallySilent = 0.001;

Uint32 audioBufferLength = 48000;// must be a multiple of samplesPerFrame (auto adjusted upwards if not)
float *audioBuffer;

SDL_atomic_t audioCallbackLeftOff;
Sint32 audioMainLeftOff;
Uint8 audioMainAccumulator;

SDL_AudioDeviceID AudioDevice;
SDL_AudioSpec audioSpec;

SDL_Event event;
SDL_bool running = SDL_TRUE;


typedef struct {
float *waveform;
Uint32 waveformLength;
double volume; // multiplied
double pan; // 0 to 1: all the way left to all the way right
double frequency; // Hz
double phase; // 0 to 1
} voice;

/* _
| |
___ _ __ ___ __ _| | __
/ __| '_ / _ / _` | |/ /
__ |_) | __/ (_| | <
|___/ .__/ ___|__,_|_|_
| |
|_|
*/
void speak(voice *v) {
float sample;
Uint32 sourceIndex;
double phaseIncrement = v->frequency/sampleRate;
Uint32 i;
if (v->volume > practicallySilent) {
for (i=0; (i+1)<samplesPerFrame; i+=2) {

v->phase += phaseIncrement;
if (v->phase > 1) v->phase -= 1;

sourceIndex = v->phase*v->waveformLength;
sample = v->waveform[sourceIndex]*v->volume;

audioBuffer[audioMainLeftOff+i] += sample*(1-v->pan); //left channel
audioBuffer[audioMainLeftOff+i+1] += sample*v->pan; //right channel
}
}
else {
for (i=0; i<samplesPerFrame; i+=1)
audioBuffer[audioMainLeftOff+i] = 0;
}
audioMainAccumulator++;
}

double getFrequency(double pitch) {
return pow(ChromaticRatio, pitch-57)*440;
}
int getWaveformLength(double pitch) {
return sampleRate / getFrequency(pitch)+0.5f;
}

void buildSineWave(float *data, Uint32 length) {
Uint32 i;
for (i=0; i < length; i++)
data[i] = sin( i*(Tao/length) );
}

void logSpec(SDL_AudioSpec *as) {
printf(
" freq______%5dn"
" format____%5dn"
" channels__%5dn"
" silence___%5dn"
" samples___%5dn"
" size______%5dnn",
(int) as->freq,
(int) as->format,
(int) as->channels,
(int) as->silence,
(int) as->samples,
(int) as->size
);
}

void logVoice(voice *v) {
printf(
" waveformLength__%dn"
" volume__________%fn"
" pan_____________%fn"
" frequency_______%fn"
" phase___________%fn",
v->waveformLength,
v->volume,
v->pan,
v->frequency,
v->phase
);
}

void logWavedata(float *floatStream, Uint32 floatStreamLength, Uint32 increment) {
printf("nnwaveform data:nn");
Uint32 i=0;
for (i=0; i<floatStreamLength; i+=increment)
printf("%4d:%2.16fn", i, floatStream[i]);
printf("nn");
}

/* _ _ _____ _ _ _ _
| (_) / ____| | | | | | |
__ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
/ _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
| (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
__,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
*/
void audioCallback(void *unused, Uint8 *byteStream, int byteStreamLength) {
float* floatStream = (float*) byteStream;

Sint32 localAudioCallbackLeftOff = SDL_AtomicGet(&audioCallbackLeftOff);

Uint32 i;
for (i=0; i<floatStreamLength; i++) {

floatStream[i] = audioBuffer[localAudioCallbackLeftOff];

localAudioCallbackLeftOff++;
if ( localAudioCallbackLeftOff == audioBufferLength )
localAudioCallbackLeftOff = 0;
}
//printf("localAudioCallbackLeftOff__%5dn", localAudioCallbackLeftOff);

SDL_AtomicSet(&audioCallbackLeftOff, localAudioCallbackLeftOff);
}

/*_ _ _
(_) (_) |
_ _ __ _| |_
| | '_ | | __|
| | | | | | |_
|_|_| |_|_|__|
*/
int init() {
SDL_Init(SDL_INIT_AUDIO | SDL_INIT_TIMER);

SDL_AudioSpec want;

SDL_zero(want);// btw, I have no idea what this is...

want.freq = sampleRate;
want.format = AUDIO_F32;
want.channels = 2;
want.samples = floatStreamLength;
want.callback = audioCallback;

AudioDevice = SDL_OpenAudioDevice(NULL, 0, &want, &audioSpec, SDL_AUDIO_ALLOW_FORMAT_CHANGE);

if (AudioDevice == 0) {
printf("nFailed to open audio: %sn", SDL_GetError());
return 1;
}

printf("want:n");
logSpec(&want);
printf("audioSpec:n");
logSpec(&audioSpec);

if (audioSpec.format != want.format) {
printf("nCouldn't get Float32 audio format.n");
return 2;
}

sampleRate = audioSpec.freq;
floatStreamLength = audioSpec.size/4;
samplesPerFrame = sampleRate/frameRate;
msPerFrame = 1000/frameRate;
audioMainLeftOff = samplesPerFrame*8;
SDL_AtomicSet(&audioCallbackLeftOff, 0);

if (audioBufferLength % samplesPerFrame)
audioBufferLength += samplesPerFrame-(audioBufferLength % samplesPerFrame);
audioBuffer = malloc( sizeof(float)*audioBufferLength );

return 0;
}
int onExit() {
SDL_CloseAudioDevice(AudioDevice);
//free(audioBuffer);//not necessary?
SDL_Quit();
return 0;
}

/* _
(_)
_ __ ___ __ _ _ _ __
| '_ ` _ / _` | | '_
| | | | | | (_| | | | | |
|_| |_| |_|__,_|_|_| |_|
*/
int main(int argc, char *argv) {

float syncCompensationFactor = 0.0016;// decrease to reduce risk of collision, increase to lower latency
Sint32 mainAudioLead;
Uint32 i;

voice testVoiceA;
voice testVoiceB;
voice testVoiceC;
testVoiceA.volume = 1;
testVoiceB.volume = 1;
testVoiceC.volume = 1;
testVoiceA.pan = 0.5;
testVoiceB.pan = 0;
testVoiceC.pan = 1;
testVoiceA.phase = 0;
testVoiceB.phase = 0;
testVoiceC.phase = 0;
testVoiceA.frequency = getFrequency(45);// A3
testVoiceB.frequency = getFrequency(49);// C#4
testVoiceC.frequency = getFrequency(52);// E4
Uint16 C0waveformLength = getWaveformLength(0);
testVoiceA.waveformLength = C0waveformLength;
testVoiceB.waveformLength = C0waveformLength;
testVoiceC.waveformLength = C0waveformLength;
float sineWave[C0waveformLength];
buildSineWave(sineWave, C0waveformLength);
testVoiceA.waveform = sineWave;
testVoiceB.waveform = sineWave;
testVoiceC.waveform = sineWave;

//logVoice(&testVoiceA);
//logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);

if ( init() ) return 1;

SDL_Delay(42);// let the tubes warm up

SDL_PauseAudioDevice(AudioDevice, 0);// unpause audio.

while (running) {
while( SDL_PollEvent( &event ) != 0 ) {
if( event.type == SDL_QUIT ) {
running = SDL_FALSE;
}
}
for (i=0; i<samplesPerFrame; i++) audioBuffer[audioMainLeftOff+i] = 0;

//printf("audioMainLeftOff___________%5dn", audioMainLeftOff);

speak(&testVoiceA);
speak(&testVoiceB);
speak(&testVoiceC);

if (audioMainAccumulator > 1) {
for (i=0; i<samplesPerFrame; i++) {
audioBuffer[audioMainLeftOff+i] /= audioMainAccumulator;
}
}
audioMainAccumulator = 0;

audioMainLeftOff += samplesPerFrame;
if (audioMainLeftOff == audioBufferLength) audioMainLeftOff = 0;

mainAudioLead = audioMainLeftOff - SDL_AtomicGet(&audioCallbackLeftOff);
if (mainAudioLead < 0) mainAudioLead += audioBufferLength;
//printf("mainAudioLead:%5dn", mainAudioLead);
if (mainAudioLead < floatStreamLength) printf("An audio collision may have occured!n");
SDL_Delay( mainAudioLead*syncCompensationFactor );
}
onExit();
return 0;
}


EDIT:



After doing some more research on my valgrind errors, I came to the conclusion that there wasn't much I could do other than suppress them. Here is my suppression file:



{
<from SDL_TimerInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:SDL_SYS_CreateThread
fun:SDL_CreateThread
fun:SDL_TimerInit
fun:SDL_InitSubSystem
fun:init
fun:main
}
{
<from SDL_AudioInit>
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
fun:allocate_dtv
fun:_dl_allocate_tls
fun:pthread_create@@GLIBC_2.2.5
fun:pa_thread_new
fun:pa_threaded_mainloop_start
fun:pa_simple_new
fun:PULSEAUDIO_Init
fun:SDL_AudioInit
fun:SDL_InitSubSystem
fun:init
fun:main
}






c lock-free atomic audio






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Mar 18 '14 at 7:39

























asked Feb 6 '14 at 17:32









Duovarious

5817




5817








  • 3




    ASCII art comments? That just repeat the names of the functions? Argh, my eyes...
    – Ant
    Feb 6 '14 at 18:11












  • haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them.
    – Duovarious
    Feb 6 '14 at 18:58














  • 3




    ASCII art comments? That just repeat the names of the functions? Argh, my eyes...
    – Ant
    Feb 6 '14 at 18:11












  • haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them.
    – Duovarious
    Feb 6 '14 at 18:58








3




3




ASCII art comments? That just repeat the names of the functions? Argh, my eyes...
– Ant
Feb 6 '14 at 18:11






ASCII art comments? That just repeat the names of the functions? Argh, my eyes...
– Ant
Feb 6 '14 at 18:11














haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them.
– Duovarious
Feb 6 '14 at 18:58




haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them.
– Duovarious
Feb 6 '14 at 18:58










1 Answer
1






active

oldest

votes

















up vote
10
down vote



accepted










Things you did well




  • Nicely formatted, easy to read.


  • Use of typedef with structures.



Things you could improve



Preprocessor:





  • Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored).



    #include <SDL/SDL.h>


    In the C standard, §6.10.2, paragraphs 2 to 4 state:






    • A preprocessing directive of the form



      #include <h-char-sequence> new-line


      searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the < and > delimiters, and causes the replacement of that directive by the entire contents of the header. How the places are specified or the header identified is implementation-defined.




    • A preprocessing directive of the form



      #include "q-char-sequence" new-line


      causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read



      #include <h-char-sequence> new-line


      with the identical contained sequence (including > characters, if any) from the original
      directive.




    • A preprocessing directive of the form



      #include pp-tokens new-line


      (that does not match one of the two previous forms) is permitted. The preprocessing tokens after include in the directive are processed just as in normal text. (Each identifier currently defined as a macro name is replaced by its replacement list of preprocessing tokens.) The directive resulting after all replacements shall match one of the two previous forms. The method by which a sequence of preprocessing tokens between a < and a > preprocessing token pair or a pair of " characters is combined into a single header name preprocessing token is implementation-defined.




    Definitions:




    • h-char: any member of the source character set except the new-line character and >


    • q-char: any member of the source character set except the new-line character and "





    Don't forget to include -lSDL with gcc to link your code to the SDL library.




Variables/Initialization:





  • Tao is simple 2π, as you have defined in your code.




    const double Tao = 6.283185307179586476925;



    However, π is a mathematically defined constant in math.h. Since you are already using that header, you should utilize the predefined constant.



    const double TAO = 2 * M_PI;



Memory:





  • You allocate memory to audioBuffer(), but then never free() it,




    audioBuffer = malloc( sizeof(float)*audioBufferLength );
    // free(audioBuffer); //not necessary?



    This would be my guess as to what valgrind is whining about. You should always have freed all memory that you have allocated before you exit your program; we want to avoid memory leaks.



    Now to adress your comment as to whether or not that line of code is necessary, since you are exiting your program anyways. It depends on the operating system. The majority of modern (and all major) operating systems will free memory not freed by the program when it ends.



    Relying on this is bad practice and it is better to free() it explicitly. The issue isn't just that your code looks bad. You may decide you want to integrate your small program into a larger, long running one. Then a while later you have to spend hours tracking down memory leaks.



    Relying on a feature of an operating system also makes the code less portable.




Syntax/Styling:





  • Right now you are using Uint32 to represent an unsigned 32 bit integer, but uint32_t is the type that's defined by the C standard.



    uint32_t sampleRate = 48000;



  • Define i within your for loops, not outside.(C99)



    for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)



  • typedef structs typically have a capitalized name by standard conventions.



    typedef struct 
    {
    float *waveform;
    Uint32 waveformLength;
    double volume; // multiplied
    double pan; // 0 to 1: all the way left to all the way right
    double frequency; // Hz
    double phase; // 0 to 1
    } Voice;



  • Declare all of your parameters as void when you don't take in any arguments.



    int init(void)



  • You aren't using the parameters specified in main().




    int main(int argc, char *argv)



    Declare them as void if you aren't going to use them.



    int main(void)



  • Use puts() instead of printf() when you aren't formatting your output.




    printf("nnwaveform data:nn");



    puts("Waveform data: ");


  • Remove != 0 in some of your conditional tests for maximum C-ness.



Comments:





  • Your ASCII art comments... hurt my eyes.




    /*                _ _        _____      _ _ _                _    
    | (_) / ____| | | | | | |
    __ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
    / _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
    | (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
    __,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
    */



    You said in the comments that: "Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them."



    Let me suggest another alternative that you can keep around: documenting your code with Doxygen. Replacing your ASCII art comments with documentation of your methods will make it easier to navigate, and serve the very important purpose of stating why/how you programmed something a certain way.



    I've taken an example from one of my previous questions to use here.



    /**
    * @fn static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    * @brief Fills token type and boundaries.
    * @param token
    * @param type
    * @param start
    * @param end
    */
    static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    {
    token->type = type;
    token->start = start;
    token->end = end;
    token->size = 0;
    }



  • Remove old commented out code.




    //logVoice(&testVoiceA);
    //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);



    It serves almost no purpose, and makes your code look cluttered.



  • Besides your ASCII art comments and your old commented out code, you have only a few other comments throughout your source code. See this blog post here as to why and how you should comment throughout your code.



Exiting:





  • You have a function dedicated to termination, and you call it right before you close down your program.




    int onExit() {
    SDL_CloseAudioDevice(AudioDevice);
    //free(audioBuffer);//not necessary?
    SDL_Quit();
    return 0;
    }



    I think you could make great use of the atexit() function in your code. The atexit() function registers a function to be called at normal program termination. Though if you decide to use this, you may want to rename onExit() to something such as cleanup() or something similar.



    int main(void)
    {
    ...
    atexit(cleanup);
    return 0;
    }







share|improve this answer



















  • 3




    However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
    – Corbin
    Mar 18 '14 at 4:31










  • Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
    – Duovarious
    Mar 18 '14 at 7:43













Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f41086%2fplay-some-sine-waves-with-sdl2%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
10
down vote



accepted










Things you did well




  • Nicely formatted, easy to read.


  • Use of typedef with structures.



Things you could improve



Preprocessor:





  • Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored).



    #include <SDL/SDL.h>


    In the C standard, §6.10.2, paragraphs 2 to 4 state:






    • A preprocessing directive of the form



      #include <h-char-sequence> new-line


      searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the < and > delimiters, and causes the replacement of that directive by the entire contents of the header. How the places are specified or the header identified is implementation-defined.




    • A preprocessing directive of the form



      #include "q-char-sequence" new-line


      causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read



      #include <h-char-sequence> new-line


      with the identical contained sequence (including > characters, if any) from the original
      directive.




    • A preprocessing directive of the form



      #include pp-tokens new-line


      (that does not match one of the two previous forms) is permitted. The preprocessing tokens after include in the directive are processed just as in normal text. (Each identifier currently defined as a macro name is replaced by its replacement list of preprocessing tokens.) The directive resulting after all replacements shall match one of the two previous forms. The method by which a sequence of preprocessing tokens between a < and a > preprocessing token pair or a pair of " characters is combined into a single header name preprocessing token is implementation-defined.




    Definitions:




    • h-char: any member of the source character set except the new-line character and >


    • q-char: any member of the source character set except the new-line character and "





    Don't forget to include -lSDL with gcc to link your code to the SDL library.




Variables/Initialization:





  • Tao is simple 2π, as you have defined in your code.




    const double Tao = 6.283185307179586476925;



    However, π is a mathematically defined constant in math.h. Since you are already using that header, you should utilize the predefined constant.



    const double TAO = 2 * M_PI;



Memory:





  • You allocate memory to audioBuffer(), but then never free() it,




    audioBuffer = malloc( sizeof(float)*audioBufferLength );
    // free(audioBuffer); //not necessary?



    This would be my guess as to what valgrind is whining about. You should always have freed all memory that you have allocated before you exit your program; we want to avoid memory leaks.



    Now to adress your comment as to whether or not that line of code is necessary, since you are exiting your program anyways. It depends on the operating system. The majority of modern (and all major) operating systems will free memory not freed by the program when it ends.



    Relying on this is bad practice and it is better to free() it explicitly. The issue isn't just that your code looks bad. You may decide you want to integrate your small program into a larger, long running one. Then a while later you have to spend hours tracking down memory leaks.



    Relying on a feature of an operating system also makes the code less portable.




Syntax/Styling:





  • Right now you are using Uint32 to represent an unsigned 32 bit integer, but uint32_t is the type that's defined by the C standard.



    uint32_t sampleRate = 48000;



  • Define i within your for loops, not outside.(C99)



    for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)



  • typedef structs typically have a capitalized name by standard conventions.



    typedef struct 
    {
    float *waveform;
    Uint32 waveformLength;
    double volume; // multiplied
    double pan; // 0 to 1: all the way left to all the way right
    double frequency; // Hz
    double phase; // 0 to 1
    } Voice;



  • Declare all of your parameters as void when you don't take in any arguments.



    int init(void)



  • You aren't using the parameters specified in main().




    int main(int argc, char *argv)



    Declare them as void if you aren't going to use them.



    int main(void)



  • Use puts() instead of printf() when you aren't formatting your output.




    printf("nnwaveform data:nn");



    puts("Waveform data: ");


  • Remove != 0 in some of your conditional tests for maximum C-ness.



Comments:





  • Your ASCII art comments... hurt my eyes.




    /*                _ _        _____      _ _ _                _    
    | (_) / ____| | | | | | |
    __ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
    / _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
    | (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
    __,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
    */



    You said in the comments that: "Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them."



    Let me suggest another alternative that you can keep around: documenting your code with Doxygen. Replacing your ASCII art comments with documentation of your methods will make it easier to navigate, and serve the very important purpose of stating why/how you programmed something a certain way.



    I've taken an example from one of my previous questions to use here.



    /**
    * @fn static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    * @brief Fills token type and boundaries.
    * @param token
    * @param type
    * @param start
    * @param end
    */
    static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    {
    token->type = type;
    token->start = start;
    token->end = end;
    token->size = 0;
    }



  • Remove old commented out code.




    //logVoice(&testVoiceA);
    //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);



    It serves almost no purpose, and makes your code look cluttered.



  • Besides your ASCII art comments and your old commented out code, you have only a few other comments throughout your source code. See this blog post here as to why and how you should comment throughout your code.



Exiting:





  • You have a function dedicated to termination, and you call it right before you close down your program.




    int onExit() {
    SDL_CloseAudioDevice(AudioDevice);
    //free(audioBuffer);//not necessary?
    SDL_Quit();
    return 0;
    }



    I think you could make great use of the atexit() function in your code. The atexit() function registers a function to be called at normal program termination. Though if you decide to use this, you may want to rename onExit() to something such as cleanup() or something similar.



    int main(void)
    {
    ...
    atexit(cleanup);
    return 0;
    }







share|improve this answer



















  • 3




    However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
    – Corbin
    Mar 18 '14 at 4:31










  • Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
    – Duovarious
    Mar 18 '14 at 7:43

















up vote
10
down vote



accepted










Things you did well




  • Nicely formatted, easy to read.


  • Use of typedef with structures.



Things you could improve



Preprocessor:





  • Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored).



    #include <SDL/SDL.h>


    In the C standard, §6.10.2, paragraphs 2 to 4 state:






    • A preprocessing directive of the form



      #include <h-char-sequence> new-line


      searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the < and > delimiters, and causes the replacement of that directive by the entire contents of the header. How the places are specified or the header identified is implementation-defined.




    • A preprocessing directive of the form



      #include "q-char-sequence" new-line


      causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read



      #include <h-char-sequence> new-line


      with the identical contained sequence (including > characters, if any) from the original
      directive.




    • A preprocessing directive of the form



      #include pp-tokens new-line


      (that does not match one of the two previous forms) is permitted. The preprocessing tokens after include in the directive are processed just as in normal text. (Each identifier currently defined as a macro name is replaced by its replacement list of preprocessing tokens.) The directive resulting after all replacements shall match one of the two previous forms. The method by which a sequence of preprocessing tokens between a < and a > preprocessing token pair or a pair of " characters is combined into a single header name preprocessing token is implementation-defined.




    Definitions:




    • h-char: any member of the source character set except the new-line character and >


    • q-char: any member of the source character set except the new-line character and "





    Don't forget to include -lSDL with gcc to link your code to the SDL library.




Variables/Initialization:





  • Tao is simple 2π, as you have defined in your code.




    const double Tao = 6.283185307179586476925;



    However, π is a mathematically defined constant in math.h. Since you are already using that header, you should utilize the predefined constant.



    const double TAO = 2 * M_PI;



Memory:





  • You allocate memory to audioBuffer(), but then never free() it,




    audioBuffer = malloc( sizeof(float)*audioBufferLength );
    // free(audioBuffer); //not necessary?



    This would be my guess as to what valgrind is whining about. You should always have freed all memory that you have allocated before you exit your program; we want to avoid memory leaks.



    Now to adress your comment as to whether or not that line of code is necessary, since you are exiting your program anyways. It depends on the operating system. The majority of modern (and all major) operating systems will free memory not freed by the program when it ends.



    Relying on this is bad practice and it is better to free() it explicitly. The issue isn't just that your code looks bad. You may decide you want to integrate your small program into a larger, long running one. Then a while later you have to spend hours tracking down memory leaks.



    Relying on a feature of an operating system also makes the code less portable.




Syntax/Styling:





  • Right now you are using Uint32 to represent an unsigned 32 bit integer, but uint32_t is the type that's defined by the C standard.



    uint32_t sampleRate = 48000;



  • Define i within your for loops, not outside.(C99)



    for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)



  • typedef structs typically have a capitalized name by standard conventions.



    typedef struct 
    {
    float *waveform;
    Uint32 waveformLength;
    double volume; // multiplied
    double pan; // 0 to 1: all the way left to all the way right
    double frequency; // Hz
    double phase; // 0 to 1
    } Voice;



  • Declare all of your parameters as void when you don't take in any arguments.



    int init(void)



  • You aren't using the parameters specified in main().




    int main(int argc, char *argv)



    Declare them as void if you aren't going to use them.



    int main(void)



  • Use puts() instead of printf() when you aren't formatting your output.




    printf("nnwaveform data:nn");



    puts("Waveform data: ");


  • Remove != 0 in some of your conditional tests for maximum C-ness.



Comments:





  • Your ASCII art comments... hurt my eyes.




    /*                _ _        _____      _ _ _                _    
    | (_) / ____| | | | | | |
    __ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
    / _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
    | (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
    __,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
    */



    You said in the comments that: "Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them."



    Let me suggest another alternative that you can keep around: documenting your code with Doxygen. Replacing your ASCII art comments with documentation of your methods will make it easier to navigate, and serve the very important purpose of stating why/how you programmed something a certain way.



    I've taken an example from one of my previous questions to use here.



    /**
    * @fn static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    * @brief Fills token type and boundaries.
    * @param token
    * @param type
    * @param start
    * @param end
    */
    static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    {
    token->type = type;
    token->start = start;
    token->end = end;
    token->size = 0;
    }



  • Remove old commented out code.




    //logVoice(&testVoiceA);
    //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);



    It serves almost no purpose, and makes your code look cluttered.



  • Besides your ASCII art comments and your old commented out code, you have only a few other comments throughout your source code. See this blog post here as to why and how you should comment throughout your code.



Exiting:





  • You have a function dedicated to termination, and you call it right before you close down your program.




    int onExit() {
    SDL_CloseAudioDevice(AudioDevice);
    //free(audioBuffer);//not necessary?
    SDL_Quit();
    return 0;
    }



    I think you could make great use of the atexit() function in your code. The atexit() function registers a function to be called at normal program termination. Though if you decide to use this, you may want to rename onExit() to something such as cleanup() or something similar.



    int main(void)
    {
    ...
    atexit(cleanup);
    return 0;
    }







share|improve this answer



















  • 3




    However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
    – Corbin
    Mar 18 '14 at 4:31










  • Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
    – Duovarious
    Mar 18 '14 at 7:43















up vote
10
down vote



accepted







up vote
10
down vote



accepted






Things you did well




  • Nicely formatted, easy to read.


  • Use of typedef with structures.



Things you could improve



Preprocessor:





  • Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored).



    #include <SDL/SDL.h>


    In the C standard, §6.10.2, paragraphs 2 to 4 state:






    • A preprocessing directive of the form



      #include <h-char-sequence> new-line


      searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the < and > delimiters, and causes the replacement of that directive by the entire contents of the header. How the places are specified or the header identified is implementation-defined.




    • A preprocessing directive of the form



      #include "q-char-sequence" new-line


      causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read



      #include <h-char-sequence> new-line


      with the identical contained sequence (including > characters, if any) from the original
      directive.




    • A preprocessing directive of the form



      #include pp-tokens new-line


      (that does not match one of the two previous forms) is permitted. The preprocessing tokens after include in the directive are processed just as in normal text. (Each identifier currently defined as a macro name is replaced by its replacement list of preprocessing tokens.) The directive resulting after all replacements shall match one of the two previous forms. The method by which a sequence of preprocessing tokens between a < and a > preprocessing token pair or a pair of " characters is combined into a single header name preprocessing token is implementation-defined.




    Definitions:




    • h-char: any member of the source character set except the new-line character and >


    • q-char: any member of the source character set except the new-line character and "





    Don't forget to include -lSDL with gcc to link your code to the SDL library.




Variables/Initialization:





  • Tao is simple 2π, as you have defined in your code.




    const double Tao = 6.283185307179586476925;



    However, π is a mathematically defined constant in math.h. Since you are already using that header, you should utilize the predefined constant.



    const double TAO = 2 * M_PI;



Memory:





  • You allocate memory to audioBuffer(), but then never free() it,




    audioBuffer = malloc( sizeof(float)*audioBufferLength );
    // free(audioBuffer); //not necessary?



    This would be my guess as to what valgrind is whining about. You should always have freed all memory that you have allocated before you exit your program; we want to avoid memory leaks.



    Now to adress your comment as to whether or not that line of code is necessary, since you are exiting your program anyways. It depends on the operating system. The majority of modern (and all major) operating systems will free memory not freed by the program when it ends.



    Relying on this is bad practice and it is better to free() it explicitly. The issue isn't just that your code looks bad. You may decide you want to integrate your small program into a larger, long running one. Then a while later you have to spend hours tracking down memory leaks.



    Relying on a feature of an operating system also makes the code less portable.




Syntax/Styling:





  • Right now you are using Uint32 to represent an unsigned 32 bit integer, but uint32_t is the type that's defined by the C standard.



    uint32_t sampleRate = 48000;



  • Define i within your for loops, not outside.(C99)



    for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)



  • typedef structs typically have a capitalized name by standard conventions.



    typedef struct 
    {
    float *waveform;
    Uint32 waveformLength;
    double volume; // multiplied
    double pan; // 0 to 1: all the way left to all the way right
    double frequency; // Hz
    double phase; // 0 to 1
    } Voice;



  • Declare all of your parameters as void when you don't take in any arguments.



    int init(void)



  • You aren't using the parameters specified in main().




    int main(int argc, char *argv)



    Declare them as void if you aren't going to use them.



    int main(void)



  • Use puts() instead of printf() when you aren't formatting your output.




    printf("nnwaveform data:nn");



    puts("Waveform data: ");


  • Remove != 0 in some of your conditional tests for maximum C-ness.



Comments:





  • Your ASCII art comments... hurt my eyes.




    /*                _ _        _____      _ _ _                _    
    | (_) / ____| | | | | | |
    __ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
    / _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
    | (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
    __,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
    */



    You said in the comments that: "Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them."



    Let me suggest another alternative that you can keep around: documenting your code with Doxygen. Replacing your ASCII art comments with documentation of your methods will make it easier to navigate, and serve the very important purpose of stating why/how you programmed something a certain way.



    I've taken an example from one of my previous questions to use here.



    /**
    * @fn static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    * @brief Fills token type and boundaries.
    * @param token
    * @param type
    * @param start
    * @param end
    */
    static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    {
    token->type = type;
    token->start = start;
    token->end = end;
    token->size = 0;
    }



  • Remove old commented out code.




    //logVoice(&testVoiceA);
    //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);



    It serves almost no purpose, and makes your code look cluttered.



  • Besides your ASCII art comments and your old commented out code, you have only a few other comments throughout your source code. See this blog post here as to why and how you should comment throughout your code.



Exiting:





  • You have a function dedicated to termination, and you call it right before you close down your program.




    int onExit() {
    SDL_CloseAudioDevice(AudioDevice);
    //free(audioBuffer);//not necessary?
    SDL_Quit();
    return 0;
    }



    I think you could make great use of the atexit() function in your code. The atexit() function registers a function to be called at normal program termination. Though if you decide to use this, you may want to rename onExit() to something such as cleanup() or something similar.



    int main(void)
    {
    ...
    atexit(cleanup);
    return 0;
    }







share|improve this answer














Things you did well




  • Nicely formatted, easy to read.


  • Use of typedef with structures.



Things you could improve



Preprocessor:





  • Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored).



    #include <SDL/SDL.h>


    In the C standard, §6.10.2, paragraphs 2 to 4 state:






    • A preprocessing directive of the form



      #include <h-char-sequence> new-line


      searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the < and > delimiters, and causes the replacement of that directive by the entire contents of the header. How the places are specified or the header identified is implementation-defined.




    • A preprocessing directive of the form



      #include "q-char-sequence" new-line


      causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read



      #include <h-char-sequence> new-line


      with the identical contained sequence (including > characters, if any) from the original
      directive.




    • A preprocessing directive of the form



      #include pp-tokens new-line


      (that does not match one of the two previous forms) is permitted. The preprocessing tokens after include in the directive are processed just as in normal text. (Each identifier currently defined as a macro name is replaced by its replacement list of preprocessing tokens.) The directive resulting after all replacements shall match one of the two previous forms. The method by which a sequence of preprocessing tokens between a < and a > preprocessing token pair or a pair of " characters is combined into a single header name preprocessing token is implementation-defined.




    Definitions:




    • h-char: any member of the source character set except the new-line character and >


    • q-char: any member of the source character set except the new-line character and "





    Don't forget to include -lSDL with gcc to link your code to the SDL library.




Variables/Initialization:





  • Tao is simple 2π, as you have defined in your code.




    const double Tao = 6.283185307179586476925;



    However, π is a mathematically defined constant in math.h. Since you are already using that header, you should utilize the predefined constant.



    const double TAO = 2 * M_PI;



Memory:





  • You allocate memory to audioBuffer(), but then never free() it,




    audioBuffer = malloc( sizeof(float)*audioBufferLength );
    // free(audioBuffer); //not necessary?



    This would be my guess as to what valgrind is whining about. You should always have freed all memory that you have allocated before you exit your program; we want to avoid memory leaks.



    Now to adress your comment as to whether or not that line of code is necessary, since you are exiting your program anyways. It depends on the operating system. The majority of modern (and all major) operating systems will free memory not freed by the program when it ends.



    Relying on this is bad practice and it is better to free() it explicitly. The issue isn't just that your code looks bad. You may decide you want to integrate your small program into a larger, long running one. Then a while later you have to spend hours tracking down memory leaks.



    Relying on a feature of an operating system also makes the code less portable.




Syntax/Styling:





  • Right now you are using Uint32 to represent an unsigned 32 bit integer, but uint32_t is the type that's defined by the C standard.



    uint32_t sampleRate = 48000;



  • Define i within your for loops, not outside.(C99)



    for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)



  • typedef structs typically have a capitalized name by standard conventions.



    typedef struct 
    {
    float *waveform;
    Uint32 waveformLength;
    double volume; // multiplied
    double pan; // 0 to 1: all the way left to all the way right
    double frequency; // Hz
    double phase; // 0 to 1
    } Voice;



  • Declare all of your parameters as void when you don't take in any arguments.



    int init(void)



  • You aren't using the parameters specified in main().




    int main(int argc, char *argv)



    Declare them as void if you aren't going to use them.



    int main(void)



  • Use puts() instead of printf() when you aren't formatting your output.




    printf("nnwaveform data:nn");



    puts("Waveform data: ");


  • Remove != 0 in some of your conditional tests for maximum C-ness.



Comments:





  • Your ASCII art comments... hurt my eyes.




    /*                _ _        _____      _ _ _                _    
    | (_) / ____| | | | | | |
    __ _ _ _ __| |_ ___ | | __ _| | | |__ __ _ ___| | __
    / _` | | | |/ _` | |/ _ | | / _` | | | '_ / _` |/ __| |/ /
    | (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__| <
    __,_|__,_|__,_|_|___/ _______,_|_|_|_.__/ __,_|___|_|_
    */



    You said in the comments that: "Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them."



    Let me suggest another alternative that you can keep around: documenting your code with Doxygen. Replacing your ASCII art comments with documentation of your methods will make it easier to navigate, and serve the very important purpose of stating why/how you programmed something a certain way.



    I've taken an example from one of my previous questions to use here.



    /**
    * @fn static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    * @brief Fills token type and boundaries.
    * @param token
    * @param type
    * @param start
    * @param end
    */
    static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    {
    token->type = type;
    token->start = start;
    token->end = end;
    token->size = 0;
    }



  • Remove old commented out code.




    //logVoice(&testVoiceA);
    //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);



    It serves almost no purpose, and makes your code look cluttered.



  • Besides your ASCII art comments and your old commented out code, you have only a few other comments throughout your source code. See this blog post here as to why and how you should comment throughout your code.



Exiting:





  • You have a function dedicated to termination, and you call it right before you close down your program.




    int onExit() {
    SDL_CloseAudioDevice(AudioDevice);
    //free(audioBuffer);//not necessary?
    SDL_Quit();
    return 0;
    }



    I think you could make great use of the atexit() function in your code. The atexit() function registers a function to be called at normal program termination. Though if you decide to use this, you may want to rename onExit() to something such as cleanup() or something similar.



    int main(void)
    {
    ...
    atexit(cleanup);
    return 0;
    }








share|improve this answer














share|improve this answer



share|improve this answer








edited 15 mins ago









albert

1171




1171










answered Mar 18 '14 at 2:05









syb0rg

16.6k797179




16.6k797179








  • 3




    However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
    – Corbin
    Mar 18 '14 at 4:31










  • Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
    – Duovarious
    Mar 18 '14 at 7:43
















  • 3




    However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
    – Corbin
    Mar 18 '14 at 4:31










  • Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
    – Duovarious
    Mar 18 '14 at 7:43










3




3




However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
– Corbin
Mar 18 '14 at 4:31




However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined.
– Corbin
Mar 18 '14 at 4:31












Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
– Duovarious
Mar 18 '14 at 7:43






Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did.
– Duovarious
Mar 18 '14 at 7:43




















draft saved

draft discarded




















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.





Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


Please pay close attention to the following guidance:


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f41086%2fplay-some-sine-waves-with-sdl2%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Quarter-circle Tiles

build a pushdown automaton that recognizes the reverse language of a given pushdown automaton?

Mont Emei