New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

audiobuffer reload fix number 2 #390

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@kd7tck
Contributor

kd7tck commented May 9, 2013

A simpler fix, I spent the last day looking over the AL lib and realized the algenbuffer method has an auto flush mechanism built in.

This is for #354.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 10, 2013

Member

the algenbuffer method has an auto flush mechanism built in

What does that mean, and how does this new code solve the reload problem?

By the way, it doesn't look clean. Are you relying on properly documented features?

Member

LaurentGomila commented May 10, 2013

the algenbuffer method has an auto flush mechanism built in

What does that mean, and how does this new code solve the reload problem?

By the way, it doesn't look clean. Are you relying on properly documented features?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 10, 2013

Member

Wouldn't this just leave the previous buffer(s) intact without freeing them? The official documentation (http://connect.creativelabs.com/openal/Documentation/OpenAL_Programmers_Guide.pdf) doesn't mention any flushing of old ones right now (they're probably cleaned up once you tear down the context; but not when calling alGenBuffers()).

Description
This function generates one or more buffers, which contain audio data (see
alBufferData). References to buffers are ALuint values, which are used wherever a
buffer reference is needed (in calls such as alDeleteBuffers, alSourcei,
alSourceQueueBuffers, and alSourceUnqueueBuffers).
Remarks
f the requested number of buffers cannot be created, an error will be generated which
can be detected with alGetError. If an error occurs, no buffers will be generated. If n
equals zero, alGenBuffers does nothing and does not return an error.

Member

MarioLiebisch commented May 10, 2013

Wouldn't this just leave the previous buffer(s) intact without freeing them? The official documentation (http://connect.creativelabs.com/openal/Documentation/OpenAL_Programmers_Guide.pdf) doesn't mention any flushing of old ones right now (they're probably cleaned up once you tear down the context; but not when calling alGenBuffers()).

Description
This function generates one or more buffers, which contain audio data (see
alBufferData). References to buffers are ALuint values, which are used wherever a
buffer reference is needed (in calls such as alDeleteBuffers, alSourcei,
alSourceQueueBuffers, and alSourceUnqueueBuffers).
Remarks
f the requested number of buffers cannot be created, an error will be generated which
can be detected with alGetError. If an error occurs, no buffers will be generated. If n
equals zero, alGenBuffers does nothing and does not return an error.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck May 11, 2013

Contributor

When looking over what I had done I forgot to test it with valgrind, bit of a goof on my part. I panicked after I realized algenbuffer was a bad decision (Mind you it did somehow fix the bug), and am unsure of where to take this now. I know of an ugly fix, but it will require the destructor being called on the buffer on every reload. Is a fix like that acceptable? Otherwise I fear the structure of soundbuffer may need to be changed, I could spend a couple weeks toying with the structure to see if there is a cleaner way to solve this.

I used the following code for testing.

#include <SFML/Audio.hpp>
#include <SFML/System.hpp>

#include <string>
#include <cstdio>

class FileStream : public sf::InputStream
{
public :

    FileStream();

    ~FileStream();

    bool open(const std::string& filename);

    virtual sf::Int64 read(void* data, sf::Int64 size);

    virtual sf::Int64 seek(sf::Int64 position);

    virtual sf::Int64 tell();

    virtual sf::Int64 getSize();

private :

    std::FILE* m_file;
};

FileStream::FileStream() :
m_file(NULL)
{
}

FileStream::~FileStream()
{
    if (m_file)
        std::fclose(m_file);
}

bool FileStream::open(const std::string& filename)
{
    if (m_file)
        std::fclose(m_file);

    m_file = std::fopen(filename.c_str(), "rb");

    return m_file != NULL;
}

sf::Int64 FileStream::read(void* data, sf::Int64 size)
{
    if (m_file)
        return std::fread(data, 1, static_cast<std::size_t>(size), m_file);
    else
        return -1;
}

sf::Int64 FileStream::seek(sf::Int64 position)
{
    if (m_file)
    {
        std::fseek(m_file, static_cast<std::size_t>(position), SEEK_SET);
        return tell();
    }
    else
    {
        return -1;
    }
}

sf::Int64 FileStream::tell()
{
    if (m_file)
        return std::ftell(m_file);
    else
        return -1;
}

sf::Int64 FileStream::getSize()
{
    if (m_file)
    {
        sf::Int64 position = tell();
        std::fseek(m_file, 0, SEEK_END);
        sf::Int64 size = tell();
        seek(position);
        return size;
    }
    else
    {
        return -1;
    }
}


int main(int, char**)
{
    sf::SoundBuffer buffer, buffer2;
    sf::Sound sound;

    FileStream stream;



    buffer.loadFromFile("cars001.wav");
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer.loadFromFile("cars002.wav");
    sound.setBuffer(buffer);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    stream.open("cars001.wav");
    buffer.loadFromStream(stream);
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer2);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    stream.open("cars002.wav");
    buffer.loadFromStream(stream);
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer2);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();





    return 0;
}
Contributor

kd7tck commented May 11, 2013

When looking over what I had done I forgot to test it with valgrind, bit of a goof on my part. I panicked after I realized algenbuffer was a bad decision (Mind you it did somehow fix the bug), and am unsure of where to take this now. I know of an ugly fix, but it will require the destructor being called on the buffer on every reload. Is a fix like that acceptable? Otherwise I fear the structure of soundbuffer may need to be changed, I could spend a couple weeks toying with the structure to see if there is a cleaner way to solve this.

I used the following code for testing.

#include <SFML/Audio.hpp>
#include <SFML/System.hpp>

#include <string>
#include <cstdio>

class FileStream : public sf::InputStream
{
public :

    FileStream();

    ~FileStream();

    bool open(const std::string& filename);

    virtual sf::Int64 read(void* data, sf::Int64 size);

    virtual sf::Int64 seek(sf::Int64 position);

    virtual sf::Int64 tell();

    virtual sf::Int64 getSize();

private :

    std::FILE* m_file;
};

FileStream::FileStream() :
m_file(NULL)
{
}

FileStream::~FileStream()
{
    if (m_file)
        std::fclose(m_file);
}

bool FileStream::open(const std::string& filename)
{
    if (m_file)
        std::fclose(m_file);

    m_file = std::fopen(filename.c_str(), "rb");

    return m_file != NULL;
}

sf::Int64 FileStream::read(void* data, sf::Int64 size)
{
    if (m_file)
        return std::fread(data, 1, static_cast<std::size_t>(size), m_file);
    else
        return -1;
}

sf::Int64 FileStream::seek(sf::Int64 position)
{
    if (m_file)
    {
        std::fseek(m_file, static_cast<std::size_t>(position), SEEK_SET);
        return tell();
    }
    else
    {
        return -1;
    }
}

sf::Int64 FileStream::tell()
{
    if (m_file)
        return std::ftell(m_file);
    else
        return -1;
}

sf::Int64 FileStream::getSize()
{
    if (m_file)
    {
        sf::Int64 position = tell();
        std::fseek(m_file, 0, SEEK_END);
        sf::Int64 size = tell();
        seek(position);
        return size;
    }
    else
    {
        return -1;
    }
}


int main(int, char**)
{
    sf::SoundBuffer buffer, buffer2;
    sf::Sound sound;

    FileStream stream;



    buffer.loadFromFile("cars001.wav");
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer.loadFromFile("cars002.wav");
    sound.setBuffer(buffer);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    stream.open("cars001.wav");
    buffer.loadFromStream(stream);
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer2);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    stream.open("cars002.wav");
    buffer.loadFromStream(stream);
    sound.setBuffer(buffer);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();

    buffer2.loadFromSamples(buffer.getSamples(), buffer.getSampleCount(), buffer.getChannelCount(), buffer.getSampleRate());
    sound.setBuffer(buffer2);
    sound.setVolume(100);
    sound.play();
    sf::sleep(sf::seconds(5));
    sound.stop();





    return 0;
}
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 11, 2013

Member

What was wrong with your first pull request? It looked like the right thing to do, with just a few details to adjust.

Member

LaurentGomila commented May 11, 2013

What was wrong with your first pull request? It looked like the right thing to do, with just a few details to adjust.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 11, 2013

Member

I'm still a bit confused on why you'd have to recreate the buffer rather than reusing it.

Did some testing and it seems the culprit is having some source attached to the buffer. If I destroy all sf::Sound objects playing the sf::SoundBuffer this issue doesn't appear. Maybe recreating the buffer could be added as some kind of error handling while mentioning in the documentation that you can't reload buffers while they're playing/being in use?

Member

MarioLiebisch commented May 11, 2013

I'm still a bit confused on why you'd have to recreate the buffer rather than reusing it.

Did some testing and it seems the culprit is having some source attached to the buffer. If I destroy all sf::Sound objects playing the sf::SoundBuffer this issue doesn't appear. Maybe recreating the buffer could be added as some kind of error handling while mentioning in the documentation that you can't reload buffers while they're playing/being in use?

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck May 11, 2013

Contributor

I wanted to avoid what was done in the first pull request, I did not want to alter the api. The following addition to loadfromfile seems to fix it.

for (SoundList::const_iterator it = m_sounds.begin(); it != m_sounds.end(); ++it)
        (*it)->resetBuffer();

My earlier decision in patch number 1 to copy the destructor and rename it unload was a lazy decision.

Laurant, if you want to I can just reload my old patch in a matter of minutes. However try this new fix I just posted with the code I used to test it. It really fixes it and no memory leaks like last time.

Contributor

kd7tck commented May 11, 2013

I wanted to avoid what was done in the first pull request, I did not want to alter the api. The following addition to loadfromfile seems to fix it.

for (SoundList::const_iterator it = m_sounds.begin(); it != m_sounds.end(); ++it)
        (*it)->resetBuffer();

My earlier decision in patch number 1 to copy the destructor and rename it unload was a lazy decision.

Laurant, if you want to I can just reload my old patch in a matter of minutes. However try this new fix I just posted with the code I used to test it. It really fixes it and no memory leaks like last time.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck May 11, 2013

Contributor

@MarioLiebisch, I agree now. My decision to recreate the buffer was one of laziness.

I will add a detachsoundfunction to allow what you suggested with error handling.

Contributor

kd7tck commented May 11, 2013

@MarioLiebisch, I agree now. My decision to recreate the buffer was one of laziness.

I will add a detachsoundfunction to allow what you suggested with error handling.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 11, 2013

Member

That looks much better.

But do the attached sounds really need to be detached? Can't they just be stopped instead? Or at least reattached once the buffer has finished reloading?

Member

LaurentGomila commented May 11, 2013

That looks much better.

But do the attached sounds really need to be detached? Can't they just be stopped instead? Or at least reattached once the buffer has finished reloading?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 11, 2013

Member

Stopping isn't enough (tried that first). Reattaching sounds reasonable to me and might be considered the expected behavior.

Member

MarioLiebisch commented May 11, 2013

Stopping isn't enough (tried that first). Reattaching sounds reasonable to me and might be considered the expected behavior.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck May 12, 2013

Contributor

@MarioLiebisch , Your right it is the expected behaviour.

Contributor

kd7tck commented May 12, 2013

@MarioLiebisch , Your right it is the expected behaviour.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck May 13, 2013

Contributor

@LaurentGomila , I added a reatachConnectedSounds function, it reconnects all prior connected sound objects.

Contributor

kd7tck commented May 13, 2013

@LaurentGomila , I added a reatachConnectedSounds function, it reconnects all prior connected sound objects.

@kd7tck

This comment has been minimized.

Show comment
Hide comment
@kd7tck

kd7tck May 18, 2013

Contributor

@LaurentGomila , I spent the last couple of days tinkering, and I have failed to find any other way of fixing this?

Contributor

kd7tck commented May 18, 2013

@LaurentGomila , I spent the last couple of days tinkering, and I have failed to find any other way of fixing this?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

I assume this has been resolved through the issue #354 and pull request #589 which got merged in 0375d75. Reopen if this is a wrong assumption.

Member

eXpl0it3r commented May 26, 2014

I assume this has been resolved through the issue #354 and pull request #589 which got merged in 0375d75. Reopen if this is a wrong assumption.

@eXpl0it3r eXpl0it3r closed this May 26, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.2, 2.x May 26, 2014

@eXpl0it3r eXpl0it3r added resolved and removed confirmed labels May 26, 2014

@eXpl0it3r eXpl0it3r added s:superseded and removed s:accepted labels Jul 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment