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
}
c lock-free atomic audio
add a comment |
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
}
c lock-free atomic audio
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
add a comment |
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
}
c lock-free atomic audio
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
c lock-free atomic audio
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
add a comment |
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
add a comment |
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
withgcc
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 neverfree()
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, butuint32_t
is the type that's defined by the C standard.
uint32_t sampleRate = 48000;
Define
i
within yourfor
loops, not outside.(C99)
for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)
typedef struct
s 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 ofprintf()
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. Theatexit()
function registers a function to be called at normal program termination. Though if you decide to use this, you may want to renameonExit()
to something such ascleanup()
or something similar.
int main(void)
{
...
atexit(cleanup);
return 0;
}
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. Thefree(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
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
withgcc
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 neverfree()
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, butuint32_t
is the type that's defined by the C standard.
uint32_t sampleRate = 48000;
Define
i
within yourfor
loops, not outside.(C99)
for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)
typedef struct
s 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 ofprintf()
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. Theatexit()
function registers a function to be called at normal program termination. Though if you decide to use this, you may want to renameonExit()
to something such ascleanup()
or something similar.
int main(void)
{
...
atexit(cleanup);
return 0;
}
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. Thefree(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
add a comment |
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
withgcc
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 neverfree()
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, butuint32_t
is the type that's defined by the C standard.
uint32_t sampleRate = 48000;
Define
i
within yourfor
loops, not outside.(C99)
for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)
typedef struct
s 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 ofprintf()
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. Theatexit()
function registers a function to be called at normal program termination. Though if you decide to use this, you may want to renameonExit()
to something such ascleanup()
or something similar.
int main(void)
{
...
atexit(cleanup);
return 0;
}
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. Thefree(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
add a comment |
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
withgcc
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 neverfree()
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, butuint32_t
is the type that's defined by the C standard.
uint32_t sampleRate = 48000;
Define
i
within yourfor
loops, not outside.(C99)
for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)
typedef struct
s 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 ofprintf()
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. Theatexit()
function registers a function to be called at normal program termination. Though if you decide to use this, you may want to renameonExit()
to something such ascleanup()
or something similar.
int main(void)
{
...
atexit(cleanup);
return 0;
}
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
withgcc
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 neverfree()
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, butuint32_t
is the type that's defined by the C standard.
uint32_t sampleRate = 48000;
Define
i
within yourfor
loops, not outside.(C99)
for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)
typedef struct
s 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 ofprintf()
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. Theatexit()
function registers a function to be called at normal program termination. Though if you decide to use this, you may want to renameonExit()
to something such ascleanup()
or something similar.
int main(void)
{
...
atexit(cleanup);
return 0;
}
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. Thefree(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
add a comment |
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. Thefree(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
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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
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