Custom soundstream implementation throws error in msvcr120.dll

[stubbz]

I'm having an issue with a custom soundstream that is described here http://stackoverflow.com/questions/25354609/custom-sfml-soundstream-is-throwing-memory-access-exception

It appears that the queue that I am trying to place results into somehow becomes corrupted and is unable to access certain portions of memory.

[zsbzsb]

Assuming you are using VS2013 as a compiler, where did you get SFML from?

[stubbz]

I am using VS2013 -- I compiled it myself using cmake and the sfml source in the git repository.

I was able to get it to stop throwing the exception... but the way I did it makes no sense to me.  (Check the edit on the stack overflow post)

[binary1248]

http://sfml-dev.org/tutorials/2.1/audio-streams.php#threading-issues

You should probably read that, carefully, and understand the requirements and safety issues related to onGetData().

Besides that, your code will leak like a boss. new without corresponding delete? That's a recipe for disaster . SFML never takes ownership of any of your memory like some other libraries do. It was designed with RAII in mind from the ground up.

[stubbz]

I'll toss a mutex in and see if that clears things up.. It's bizarre that I was able to stop the crashes just by adding a member to the class though..

AHHHH!!! I noticed I had a leak, but couldn't think of where it could be happening... thanks for bringing that up.. I figured that SFML would clean up after me.  Thanks!


EDIT:
I added mutexes in where necessary, but I'm still running into the same sort of errors.
although now it's unable to read the memory location of the mutex member.

Would it matter that the members of the SoundStream (as well as the soundstream) are allocated on the stack?

[binary1248]

Showing us your new code would always help...

[stubbz]

Sorry, didn't wanna blast you with my code and say please fix it ..


the stream / main

#include <SFML/Graphics.hpp>
#include <SFML/Audio.hpp>
#include "Signals.h"
#include "Waveforms.h"
#include "Wavetable.h"
#include <array>
#include <cmath>
#include "DspMath.h"
#include <iostream>
#include "Synth.h"
#include "SynthFactory.h"
#include "Spectrometer.h"
#include <functional>
#include <queue>
using namespace std;
using namespace synth;

#include <concurrent_queue.h>

class SynthStream : public sf::SoundStream
{
public:

        SynthStream(synth::Synth& synth, Spectrometer& spectrometer) :
                _synth((synth)),
                _spectrometer(spectrometer)
        {
                this->setLoop(false);
                this->initialize(1, 44100);
        }
private:
        //queue<Signal> _results;
        Synth& _synth;
        Spectrometer& _spectrometer;
        virtual bool onGetData(Chunk& data)
        {
                Signal signal = _synth.getSignal(4096);
               
                const SignalVector& signalVector = signal.getSignalVector();
                unsigned int length = signalVector.size();
                sf::Int16* samples = new sf::Int16[length];
                for (int sample = 0; sample < length; sample++)
                {
                        double value = signalVector[sample];
                        samples[sample] = 10000* value;
                        if (sample % 10 == 0)
                                _spectrometer.addValue(value);
                }
                data.samples = samples;
                data.sampleCount = length;

                return true;
        }
        virtual void onSeek(sf::Time timeOffset)
        {
        }

};



int main()
{
        const int width = 800;
        const int height = 200;

        sf::RenderWindow window(sf::VideoMode(width, height), "StubbzDSP.CPP.Visualization");

        unsigned int sampleRate = 44100;

        const synth::Wavetable wavetable = synth::wavetable::getWavetable();
        const synth::filters::FilterTable filterTable = synth::filters::getFilterTable();
        synth::DefaultSynthFactory synthFactory;
        synth::Synth synth = synthFactory.createSynth(sampleRate, wavetable, filterTable);

        unsigned int rootNote = 69 - 24;
        synth.enableNote(rootNote, 1);
        synth.enableNote(rootNote + 7, 1);
        //synth.enableNote(rootNote - 5, 1);

        double scale = (height) / 4;

        sf::View view(sf::FloatRect(0, 0, width, height));
       

        view.move(0, -scale * 2);
        window.setView(view);
        window.setVerticalSyncEnabled(true);

       
        Spectrometer spectrometer(width, height, 5);

        SynthStream stream(synth, spectrometer);
        stream.play();

        std::queue<Signal> signals;
        unsigned int c = 72 - 12;
        unordered_map<sf::Keyboard::Key, unsigned int> keyMap {
                        { sf::Keyboard::A, c },      //c
                                        { sf::Keyboard::W, c + 1 },  //c#
                        { sf::Keyboard::S, c + 2 },  //d
                                        { sf::Keyboard::E, c + 3 },  //d#
                        { sf::Keyboard::D, c + 4 },  //e
                        { sf::Keyboard::F, c + 5 },  //f
                                        { sf::Keyboard::T, c + 6 },  //f#
                        { sf::Keyboard::G, c + 7 },  //g
                                        { sf::Keyboard::Y, c + 8 },  //g#
                        { sf::Keyboard::H, c + 9 },  //a
                                        { sf::Keyboard::U, c + 10 }, //a#
                        { sf::Keyboard::J, c + 11 }, //b
                        { sf::Keyboard::K, c + 12 }, //c
                                        { sf::Keyboard::O, c + 13 }, //c#
                        { sf::Keyboard::L, c + 14 }, //d
                                        { sf::Keyboard::P, c + 14 }, //d#
        };

        double value = 0;

        while (stream.getStatus() == SynthStream::Playing && window.isOpen())
        {

                        sf::Event event;
                        while (window.pollEvent(event))
                        {
                                if (event.type == sf::Event::Closed)
                                        window.close();

                                if (event.type == sf::Event::KeyPressed)
                                {
                                        auto key = event.key.code;
                                        auto mapping = keyMap.find(key);
                                        if (mapping != keyMap.end()){
                                                //synth.disableNote(mapping->second);
                                                synth.enableNote(mapping->second, 1);
                                        } else
                                        {
                                                if (key == sf::Keyboard::Up)
                                                {
                                                        value += .2;
                                                        if (value > 1)
                                                                value = 1;
                                                        synthFactory._lfo1Freq.setValue(value);
                                                } else if (key == sf::Keyboard::Down)
                                                {
                                                        value -= .2;
                                                        if (value < 0)
                                                                value = 0;
                                                        synthFactory._lfo1Freq.setValue(value);
                                                }
                                        }
                                }
                                if (event.type == sf::Event::KeyReleased)
                                {
                                        auto key = event.key.code;
                                        auto mapping = keyMap.find(key);
                                        if (mapping != keyMap.end()){
                                                synth.disableNote(mapping->second);
                                        }
                                }

                        }

                        window.clear(sf::Color::White);
                        spectrometer.draw(window);
                        spectrometer.draw(window);
                        window.display();
        }

       

        return 0;
}
 

Synth

#pragma once

#include "SignalGenerator.h"
#include <unordered_map>
#include "Note.h"
#include <mutex>

namespace synth{
        class Synth
        {
        public:
                Synth(unique_ptr<SignalGenerator> signalGeneratorFactory, unsigned int sampleRate);
                Synth(Synth&& other);
                Synth& operator=(Synth&& other) = delete;
                Synth(const Synth& other) = delete;
                Synth& operator= (const Synth& other) = delete;
                ~Synth();

                void Synth::enableNote(char midiNoteId, double velocity);
                void Synth::disableNote(char midiNoteId);
                Signal getSignal(unsigned int signalLength);

        private:
                unique_ptr<SignalGenerator> _signalGenerator;
                unordered_multimap<char, Note*> _notes;
                unsigned int _sampleRate;
                mutex _noteLock;
        };
}


#include "Synth.h"
#include "DspMath.h"
#include "SignalFunctions.h"
#include <algorithm>
#include <iostream>

using namespace synth;

Synth::Synth(unique_ptr<SignalGenerator> signalGenerator, unsigned int sampleRate)
        : _signalGenerator(move(signalGenerator)), _sampleRate(sampleRate){}
Synth::Synth(Synth&& other)
        : _signalGenerator(move(other._signalGenerator)), _sampleRate(other._sampleRate), _notes(move(_notes)){}

Synth::~Synth(){
        //TODO make sure that all the _notes are being deleted from "this->_notes"
}

void Synth::enableNote(char midiNoteId, double velocity){
        _noteLock.lock();

        auto existingNotes = _notes.equal_range(midiNoteId);

        //TODO update velocity
        for (auto it = existingNotes.first; it != existingNotes.second; ++it){
        }

        //TODO this current logic only allows one note of an id
        if (existingNotes.first == existingNotes.second){
                double frequency = dspMath::convertMidiNoteToFrequency(midiNoteId);
                _notes.emplace(std::pair<char, Note*>(midiNoteId, new Note(midiNoteId, frequency, velocity)));
        }

        _noteLock.unlock();
}

void Synth::disableNote(char midiNoteId){
        //TODO instead of erasing _notes, mark their statuses as being completed
        //auto existingNotes = _notes.equal_range(midiNoteId);

        _noteLock.lock();

        _notes.erase(midiNoteId);

        _noteLock.unlock();

}


Signal Synth::getSignal(unsigned int signalLength){

        _noteLock.lock();


        vector<const shared_ptr<Signal>> signals;

        for (auto it = _notes.begin(); it != _notes.end(); ++it){

                Note& note = *it->second;

                SignalConfiguration& configuration = note.getConfiguration(signalLength, _sampleRate);
                unique_ptr<SignalConfiguration> nextConfiguration = unique_ptr<SignalConfiguration>(new SignalConfiguration(note.getFrequency(), signalLength, _sampleRate, note.isNoteOff()));
                SignalCache signalCache;
                shared_ptr<Signal> signal = _signalGenerator->getSignal(configuration, *nextConfiguration, signalCache);
                note.setConfiguration(move(nextConfiguration));
                signals.push_back(signal);
                if (signal->isComplete()){
                        this->_notes.erase(it);
                }
        }
        _noteLock.unlock();


        return signals::mixSignals(signals, signalLength);
}

 

spectrometer

#include <SFML/Graphics/Drawable.hpp>
#include <SFML/Graphics/VertexArray.hpp>
#include <SFML/Graphics/RenderTarget.hpp>
#include <SFML/Graphics/RenderWindow.hpp>
#include <SFML/System/Thread.hpp>
#include <mutex>

class Spectrometer //: public sf::Drawable
{
public:
        Spectrometer(unsigned int width, unsigned int height, double thickness);
        ~Spectrometer();
        Spectrometer(Spectrometer&& other);
        Spectrometer& operator=(Spectrometer&& other) = delete;
        Spectrometer(const Spectrometer& other) = delete;
        Spectrometer& operator= (const Spectrometer& other) = delete;
        void addValue(double value);
        sf::VertexArray _wave;
        void draw(sf::RenderWindow& window) ;

private:
        void setValue(unsigned x, double value);
        unsigned int _width;
        unsigned int _height;
        double _yScale;
        double _thickness;
        unsigned int _currentX;

         std::mutex _lock;
};


Spectrometer::~Spectrometer()
{
}

Spectrometer::Spectrometer(Spectrometer&& other)
        : _width(other._width),
        _height(other._height),
        _yScale(other._yScale),
        _wave(std::move(other._wave)),
        _thickness(other._thickness),
        _currentX(other._currentX)
{
}

Spectrometer::Spectrometer(unsigned width, unsigned height, double thickness)
        : _width(width),
        _height(height),
        _yScale(height / 2),
        _wave(sf::VertexArray(sf::TrianglesStrip, width * 2)),
        _thickness(thickness/2),
        _currentX(0)
{
        for (unsigned int x = 0; x < width; x++)
        {
                this->setValue(x, 0);
        }
}

void Spectrometer::draw(sf::RenderWindow& window)
{
        _lock.lock();

        window.draw(_wave);


        sf::RectangleShape line(sf::Vector2f(_thickness * 2, _height));
        line.setPosition(_currentX, -_yScale);
        line.setFillColor(sf::Color::Black);
        window.draw(line);

        _lock.unlock();


}

void Spectrometer::addValue(double value)
{
        _lock.lock();

        if (_currentX * 2 + 1 > _wave.getVertexCount() - 1)
        {
                _currentX = 0;
        }
        this->setValue(_currentX, value);
        _currentX++;

        _lock.unlock();

}

void Spectrometer::setValue(unsigned int x, double value)
{
        auto color = sf::Color::Black;
        if (x + 1 <= _wave.getVertexCount() - 1)
        {
                double scaledValue = value * _yScale * .5;
                _wave[2 * x] = sf::Vertex(sf::Vector2f(x, scaledValue + _thickness), color);
                _wave[2 * x + 1] = sf::Vertex(sf::Vector2f(x, scaledValue - _thickness), color);
        }
}



 

[binary1248]

You added mutexes... but not to the right place.

The issue is that onGetData() can be called at any time, so even when your other classes are currently modifying their data. onGetData() ends up inspecting those other classes while they are still at work being modified and you end up having problems. You have to make sure that onGetData() does not do what it does at the same time as any of the data it depends on is being modified.

[stubbz]

I'm not sure I understand.. are you saying that it is more important to protect access to the reference to the class versus the state that the class carries?  Currently I'm wrapping any changes in state in a mutex.

What I'm saying is... I'm doing this...

void Class:doIt(){
   lock.lock();
   //change state
   lock.unlock();
}

 

Should I be doing...

void main(){
   lock.lock();
   obj.doIt();
   lock.unlock();
}

 

I'm not sure I fully understand at a low level how data races are resulting in crashes..  I understand a race condition happening and a wrong value being stored, but not the case where a member variable is inaccessable.  Is it due to a race condition in how the member variables are accessed/addressed in memory?

[Nexus]

Race conditions can lead to inconsistent values and wrong program logic. Thus, your assumptions and preconditions in the code suddenly don't apply anymore, and anything can happen, including crashes (e.g. if you dereference a bad pointer).

By the way, mutexes should be used with RAII. That way, they are still unlocked if you return early or throw an exception in the function.

void Class::doIt()
{
    sf::Lock lock(mutex);
    ... // do stuff
} // automatic unlock here

And main() returns int, not void!

[stubbz]

So I've distilled my code down to this.

class SynthStream : public sf::SoundStream
{
public:

        SynthStream()
        {
                this->setLoop(false);
                this->initialize(1, 44100);
        }
private:

        std::mutex _mutex;
        //sf::Mutex _mutex;
        virtual bool onGetData(Chunk& data)
        {
                std::lock_guard<std::mutex> lock(_mutex);
                //sf::Lock lock(_mutex);
               

                return true;
        }
        virtual void onSeek(sf::Time timeOffset)
        {
        }

};



int main()
{
        const int width = 800;
        const int height = 200;

        sf::RenderWindow window(sf::VideoMode(width, height), "StubbzDSP.CPP.Visualization");


        SynthStream stream;
        stream.play();


        while (stream.getStatus() == SynthStream::Playing && window.isOpen())
        {
                        window.clear(sf::Color::White);
                        //spectrometer.draw(window);
                        window.display();
        }

       

        return 0;
}

And it's still throwing memory access errors (specifically on the _mutex member), I'm thinking perhaps it's my compilation of the SFML, one of the .dll's that I have included, or project settings.

[binary1248]

The only thing I could think of is some incompatibility between the std threading facility and SFML's own threads. If the standard library you use implements threads using a different family of operating system functions than the one SFML uses, it might cause abnormal behaviour. Try to refrain from using any of the std threading classes with sf::SoundStream and see if it helps.

[stubbz]

I tried using an sfml mutex to no avail... I'm thinking it's something to do with my build of sfml.

[Jesper Juhl]

I'm not seeing any obvious causes for your problem from a quick reading of the code.

Some wild shot in the dark guesses and things to try:

- maybe you are mixing release and debug builds?
- maybe you are messing up compiler settings related to threading ("/MT , /MD and friends" - maybe only for your own code or SFML)?
- if you are doing release builds, try a debug build or vice versa, just to see if a new hint pops up.
- you could try doing a build with either the clang or gcc compiler and enable their "thread sanitizer" features and see if it catches anything (while you are at it, try "address sanitizer" as well).
- you could try cranking up your compilers warning level and see if it points out anything relevant.
- you could try a Linux build just to see if it behaves differently.
- if you do a Linux build you could run it under Valgrind's "helgrind" tool which can catch some threading bugs.
- you could run a static code analysis tool on your code and see if it catches anything (like; clang-analyze, cppcheck, PVS studio, coverity prevent, msvc /Analyze, cppcat, flexelint).
- if linking static, try dynamic and vice versa.
- since it's a threading bug you could try to see if it goes away on a single core system (either by pin'ing your app to a single core, booting the OS in a mode where only a single core is used (doable on Linux, not sure about Windows) or running it on actual single-core hardware) and observe if that makes the bug go away (just to get an extra data point/hint).

As I said; just some random wild guesses. But it's what I'd try if I had some weird bug and reading the code didn't give any clues...